[dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions

Anoob Joseph anoob.joseph at caviumnetworks.com
Fri Jan 5 06:52:05 CET 2018


Hi Boris,


On 12/21/2017 03:42 PM, Boris Pismenny wrote:
> Hi Anoob,
>
>
> On 12/21/2017 10:06 AM, Anoob Joseph wrote:
>> HI Boris,
>>
>>
>> On 12/20/2017 09:49 PM, Boris Pismenny wrote:
>>> Hi,
>>>
>>>> On Wed, Dec 13, 2017 at 07:14:19PM, Nelio Laranjeiro wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Wed, Dec 13, 2017 at 07:23:19PM +0530, Anoob Joseph wrote:
>>>>> Hi Nelio,
>>>>>
>>>>>
>>>>> On 12/13/2017 06:23 PM, Nelio Laranjeiro wrote:
>>>>>> Hi Anoob,
>>>>>>
>>>>>> On Wed, Dec 13, 2017 at 05:08:19PM +0530, Anoob Joseph wrote:
>>>>>>> Hi Nelio,
>>>>>>>
>>>>>>>
>>>>>>> On 12/13/2017 03:32 PM, Nelio Laranjeiro wrote:
>>>>>>>> Hi Anoob,
>>>>>>>>
>>>>>>>> On Wed, Dec 13, 2017 at 12:11:18PM +0530, Anoob Joseph wrote:
>>>>>>>>> Hi Nelio,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 12/12/2017 08:08 PM, Nelio Laranjeiro wrote:
>>>>>>>>>> Hi Anoob,
>>>>>>>>>>
>>>>>>>>>> On Tue, Dec 12, 2017 at 07:34:31PM +0530, Anoob Joseph wrote:
>>>>>>>>>>> Hi Nelio,
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 12/12/2017 07:14 PM, Nelio Laranjeiro wrote:
>>>>>>>>>>>> Hi Anoob,
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Dec 12, 2017 at 06:13:08PM +0530, Anoob Joseph
>>>> wrote:
>>>>>>>>>>>>> Hi Nelio,
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 12/11/2017 07:34 PM, Nelio Laranjeiro wrote:
>>>>>>>>>>>>>> Mellanox INNOVA NIC needs to have final target queue
>>>>>>>>>>>>>> actions to perform inline crypto.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Nelio Laranjeiro
>>>>>>>>>>>>>> <nelio.laranjeiro at 6wind.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>        * removed PASSTHRU test for ingress.
>>>>>>>>>>>>>>        * removed check on configured queues for the queue
>>>> action.
>>>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>        * Test the rule by PASSTHRU/RSS/QUEUE and apply the
>>>> first one validated.
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>        examples/ipsec-secgw/ipsec.c | 57
>>>> ++++++++++++++++++++++++++++++++++++++++++--
>>>>>>>>>>>>>> examples/ipsec-secgw/ipsec.h |  2 +-
>>>>>>>>>>>>>>        2 files changed, 56 insertions(+), 3
>>>>>>>>>>>>>> deletions(-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git a/examples/ipsec-secgw/ipsec.c
>>>>>>>>>>>>>> b/examples/ipsec-secgw/ipsec.c index
>>>>>>>>>>>>>> 17bd7620d..1b8b251c8 100644
>>>>>>>>>>>>>> --- a/examples/ipsec-secgw/ipsec.c
>>>>>>>>>>>>>> +++ b/examples/ipsec-secgw/ipsec.c
>>>>>>>>>>>>>> @@ -142,6 +142,7 @@ create_session(struct ipsec_ctx
>>>> *ipsec_ctx, struct ipsec_sa *sa)
>>>>     rte_eth_dev_get_sec_ctx(
>>>>>>>>>>>>>> sa-
>>>>> portid);
>>>>>>>>>>>>>> const struct rte_security_capability
>>>>>>>>>>>>>> *sec_cap;
>>>>>>>>>>>>>> +            int ret = 0;
>>>>>>>>>>>>>>                    sa->sec_session =
>>>> rte_security_session_create(ctx,
>>>>>>>>>>>>>> &sess_conf,
>>>> ipsec_ctx->session_pool); @@
>>>>>>>>>>>>>> -201,15 +202,67 @@ create_session(struct ipsec_ctx
>>>> *ipsec_ctx, struct ipsec_sa *sa)
>>>>>>>>>>>>>> sa->action[0].type =
>>>> RTE_FLOW_ACTION_TYPE_SECURITY;
>>>>>>>>>>>>>> sa->action[0].conf = sa->sec_session;
>>>>>>>>>>>>>> -            sa->action[1].type =
>>>> RTE_FLOW_ACTION_TYPE_END;
>>>>>>>>>>>>>> -
>>>>>>>>>>>>>>                    sa->attr.egress = (sa->direction ==
>>>>>>>>>>>>>>
>>>>     RTE_SECURITY_IPSEC_SA_DIR_EGRESS);
>>>>>>>>>>>>>> sa->attr.ingress = (sa->direction ==
>>>>>>>>>>>>>>
>>>>     RTE_SECURITY_IPSEC_SA_DIR_INGRESS);
>>>>>>>>>>>>>> +            if (sa->attr.ingress) {
>>>>>>>>>>>>>> +                uint8_t rss_key[40];
>>>>>>>>>>>>>> +                struct rte_eth_rss_conf
>>>> rss_conf = {
>>>>>>>>>>>>>> + .rss_key = rss_key,
>>>>>>>>>>>>>> +                    .rss_key_len = 40,
>>>>>>>>>>>>>> +                };
>>>>>>>>>>>>>> +                struct rte_eth_dev
>>>> *eth_dev;
>>>>>>>>>>>>>> + union {
>>>>>>>>>>>>>> +                    struct
>>>> rte_flow_action_rss rss;
>>>>>>>>>>>>>> + struct {
>>>>>>>>>>>>>> +                    const struct
>>>> rte_eth_rss_conf *rss_conf;
>>>>>>>>>>>>>> + uint16_t num;
>>>>>>>>>>>>>> +                    uint16_t
>>>> queue[RTE_MAX_QUEUES_PER_PORT];
>>>>>>>>>>>>>> + } local;
>>>>>>>>>>>>>> +                } action_rss;
>>>>>>>>>>>>>> +                unsigned int i;
>>>>>>>>>>>>>> +                unsigned int j;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +                sa->action[2].type =
>>>> RTE_FLOW_ACTION_TYPE_END;
>>>>>>>>>>>>>> + /* Try RSS. */
>>>>>>>>>>>>>> +                sa->action[1].type =
>>>> RTE_FLOW_ACTION_TYPE_RSS;
>>>>>>>>>>>>>> + sa->action[1].conf =
>>>> &action_rss;
>>>>>>>>>>>>>> + eth_dev = ctx->device;
>>>>>>>>>>>>>> +
>>>>     rte_eth_dev_rss_hash_conf_get(sa->portid,
>>>>>>>>>>>>>> +
>>>> &rss_conf);
>>>>>>>>>>>>>> + for (i = 0, j = 0;
>>>>>>>>>>>>>> +                     i < eth_dev->data-
>>>>> nb_rx_queues; ++i)
>>>>>>>>>>>>>> + if (eth_dev->data-
>>>>> rx_queues[i])
>>>>>>>>>>>>>> +
>>>>     action_rss.local.queue[j++] = i;
>>>>>>>>>>>>>> + action_rss.local.num = j;
>>>>>>>>>>>>>> + action_rss.local.rss_conf =
>>>> &rss_conf;
>>>>>>>>>>>>>> + ret = rte_flow_validate(sa-
>>>>> portid, &sa->attr,
>>>>>>>>>>>>>> + sa-
>>>>> pattern, sa->action,
>>>>>>>>>>>>>> + &err);
>>>>>>>>>>>>>> +                if (!ret)
>>>>>>>>>>>>>> +                    goto flow_create;
>>>>>>>>>>>>>> +                /* Try Queue. */
>>>>>>>>>>>>>> +                sa->action[1].type =
>>>> RTE_FLOW_ACTION_TYPE_QUEUE;
>>>>>>>>>>>>>> + sa->action[1].conf =
>>>>>>>>>>>>>> +                    &(struct
>>>> rte_flow_action_queue){
>>>>>>>>>>>>>> + .index = 0,
>>>>>>>>>>>>>> +                };
>>>>>>>>>>>>>> +                ret = rte_flow_validate(sa-
>>>>> portid, &sa->attr,
>>>>>>>>>>>>>> + sa-
>>>>> pattern, sa->action,
>>>>>>>>>>>>>> + &err);
>>>>>>>>>>>>>> +                if (ret)
>>>>>>>>>>>>>> +                    goto
>>>> flow_create_failure;
>>>>>>>>>>>>>> +            } else {
>>>>>>>>>>>>>> +                sa->action[1].type =
>>>>>>>>>>>>>> +
>>>>     RTE_FLOW_ACTION_TYPE_PASSTHRU;
>>>>>>>>>>>>>> + sa->action[2].type =
>>>> RTE_FLOW_ACTION_TYPE_END;
>>>>>>>>>>>>> We would need flow validate here also. And, for
>>>>>>>>>>>>> egress, the application will be able to set metadata
>>>>>>>>>>>>> (set_pkt_metadata API) per packet. So flow may not be
>>>>>>>>>>>>> required for such cases. But if the flow create fails,
>>>>>>>>>>>>> the session create would also fail. It might be better if we
>>>> check whether the PMD would need metadata (part of the sec_cap-
>>>>> ol_flags).
>>>>>>>>>>>> Seems what you are describing is outside of this scope
>>>>>>>>>>>> which is only related to correctly implement the generic
>>>>>>>>>>>> flow API with terminal actions.
>>>>>>>>>>> Since SECURITY+PASSTHRU won't be terminal, this code
>>>>>>>>>>> segment might be misleading.
>>>>>>>>>> Well, I don't mind adding an extra verification even if the
>>>>>>>>>> create should fail if the validate fails, as there is no
>>>>>>>>>> other option it is just like adding another if statement
>>>>>>>>>> considering  the validate() cannot guarantee the flow will
>>>>>>>>>> be created(), other errors like ENOMEM are still possible in the
>>>> creation stage.
>>>>>>>>> Good point. I was thinking of a scenario when flow for egress
>>>>>>>>> itself would be optional.
>>>>>>>>>>>> I'll suggest to add it in another patch.
>>>>>>>>>>>>
>>>>>>>>>>>> Anyway, the flow validate is useful in the ingress to
>>>>>>>>>>>> select the best behavior RSS/Queue, if the flow validate
>>>>>>>>>>>> may fail, the flow create should also fail for the same 
>>>>>>>>>>>> reasons.
>>>>>>>>>>>>
>>>>>>>>>>>>> If the driver doesn't need metadata and the flow
>>>>>>>>>>>>> create fails, then the create session should fail. Any
>>>> thoughts?
>>>>>>>>>>>> How the create_session() can fail without having all the
>>>>>>>>>>>> informations (pattern, metadata, ...) the application wants to
>>>> offload?
>>>>>>>>>>> Is flow mandatory for the egress traffic? My understanding is,
>>>> it's not.
>>>>>>>>>>> "set_pkt_metadata" API gives application the ability to do
>>>>>>>>>>> the lookup and pass the info along with the packet. In
>>>>>>>>>>> such cases, flow creation is not necessary.
>>>>>>>>>> Some NIC need to apply a flow rule for Egress and they don't
>>>>>>>>>> need metadata for the packet.
>>>>>>>>> Understood. In that case, what I proposed could be a separate
>>>>>>>>> patch. The ingress path is proper with this patch, but we can
>>>>>>>>> keep egress open for improvements.
>>>>>>>> What do you mean with "keep egrees open for improvements"?
>>>>>>> In egress side, this set of flow actions won't be having any
>>>>>>> terminating action. And addition of PASSTHRU won't be required, 
>>>>>>> as it
>>>> will be implicit.
>>>>>> Flow API does not define any behavior on Egress.  We have to 
>>>>>> define it.
>>>>> Understood.
>>>>>>> Creating flow for egress would allow hardware to perform the SA
>>>>>>> lookup. But we cannot remove the lookup in application, as it's
>>>>>>> the SA which has the information whether the packet need to be
>>>>>>> completely offloaded. Once this lookup is done, this information
>>>>>>> could be communicated to hardware using the set_pkt_metadata.
>>>>>>>
>>>>>>> This will eliminate the second lookup in the hardware. So the flow
>>>>>>> could be optional. The current patch assumes flow is mandatory for
>>>>>>> egress as well.
>>>>>>> For Cavium hardware, egress side flow is not required and we will
>>>>>>> be using "set_pkt_metadata" API. May be Radu can give his thoughts
>>>> on this.
>>>>>> Got it, what is missing here is a verification on the sa->ol_flags
>>>>>> and only use the rte_flow for RTE_SECURITY_TX_HW_TRAILER_OFFLOAD
>>>> as
>>>>>> other NICs are using the RTE_SECURITY_TX_OLOAD_NEED_MDATA.
>>>>> Precisely.
>>>> I'll make the changes discussed here, splitting this patch into 
>>>> ingress/Egress
>>>> and use the of_flags.
>>>>
>>>>>> Do you know why such difference is not hidden by the library?  It
>>>>>> won't help application which will need to have different
>>>>>> configuration path depending on the NIC capabilities.
>>>>> I can only speculate the reasons. I think, application will have to
>>>>> know the NIC capabilities as the usage of rte_flow would be a one 
>>>>> time
>>>>> configuration while using set_pkt_metadata would be per packet API
>>>>> call. If library is to hide this, it would incur unwanted checks 
>>>>> in the data
>>>> path.
>>>>
>>>> Why not embedding all the configuration part necessary for all PMD 
>>>> (as this
>>>> example is making though this modifications) inside rte_security 
>>>> library and
>>>> in et_dev add a new Tx burst API with another array containing the 
>>>> metadata
>>>> for the each packet.
>>>>
>>>> PMD who needs such metadata have it along with the packet they are
>>>> processing, others can ignore it.
>>>>
>>>>  From an application point of view, this become transparent and 
>>>> friendly, one
>>>> function to set the offload request, one function to send packets and
>>>> another one in Rx for the symmetry if necessary.
>>>>
>>> The call to rte_flow is necessary both on Tx and Rx.
>>>
>>> The reason for using setting the flow on Tx is to provide the packet 
>>> header fields
>>> which are required to identify the SA. This use enables 
>>> extensibility. For instance,
>>> we could describe ESP over VXLAN simply be providing the appropriate 
>>> rte_flow
>>> configuration. If the flow creation fails, then this is not supported.
>>> A similar use-case is the use of VLANs with ESP traffic.
>>>
>>> I understand that it is possible to use the metadata to provide 
>>> applications with all
>>> information. But, a user is better served by an API that indicates 
>>> an error when the
>>> control operation is executed (e.g. rte_flow) and not during the 
>>> data-path (e.g. metadata).
>> I can see the benefits of using rte_flow in both rx & tx, but this 
>> unnecessarily introduces hardware requirements for supporting inline. 
>> Rte_flow would do two operations:
>> 1) Pattern matching
>> 2) Perform some operation with the above matched packets
>>
>> Issue is with pattern matching, not about performing the operations 
>> specified. If we need rte_flow in the tx, the PMD should support 
>> pattern matching in software or hardware. Since the application will 
>> have to do a lookup in it's space to determine the SA, the secondary 
>> lookup in the PMD may not be necessary. But making rte_flow mandatory 
>> would make tx side pattern matching mandatory, which may not be 
>> supported on all hardware (with inline crypto/protocol). Also the 
>> pattern matching hardware module should be able to submit to inline 
>> performing module for this to work.
>>
>> May be the right approach is to decouple pattern matching from 
>> actions to be performed for the flow. In other words, add a new API 
>> to allow application to submit a packet to a flow. In such case, 
>> application could do the lookup and submit packet to a flow. The 
>> packet submitted could be validated to see if it is matching the flow 
>> properties. If it is matching, then the actions specified for the 
>> flow would be performed. Adding such an API will allow rte_flow to be 
>> used with hardware which doesn't have packet filtering features.
>>
>> The flow could have a "pattern item" which would say whether 
>> application can submit packets to the flow. Submit would be allowed 
>> only for those flows. flow_validate would give PMD the option to 
>> accept or reject such a model. This may need some thought before we 
>> can start implementing, like, whether we should support "submit" for 
>> flows which doesn't have terminating action.
>>
>> Any thoughts?
>
> I think that your suggested API is more or less the intended use of 
> rte_flow egress actions with rte_security.
>
> Would it be wrong to say that you could use rte_flow without doing 
> pattern matching in HW or in the PMD in the data-path?
But the documentation says pattern matching is mandatory.
> Suppose that your HW doesn't support pattern matching on tx. But, you 
> do support IPsec inline crypto on tx according to user provided 
> pointers that you set in the set_pkt_metadata callback. The user will 
> call rte_create_flow with some pattern, in response you check that the 
> driver's set_pkt_metadata could handle such patterns and actions on 
> tx. If yes, then return success, otherwise return false. The 
> successful creation of the flow will indicate to the user that packets 
> with this format will be offloaded. Packets with other formats will 
> not get offload and set_pkt_metadata for such packets shouldn't be 
> called!
This would be more misleading to the application. If the flow_create is 
successful, the application would expect pattern matching in 
PMD/hardware. And in this case, PMD should lie about it's behavior when 
the upper application is doing inline. That also might not scale well.
>
> When using rte_flow with IPsec, it is used not to indicate that HW 
> must do this pattern matching. But rather to indicate that software 
> will send packets that match a pattern with proper metadata and expect 
> an action to be applied. Software cannot expect this action to be 
> applied unless the packet matches the pattern and the proper metadata 
> is provided. For example, packets with IPv6 extension headers should 
> not go through IPsec inline crypto offload if the pattern is IPv6/ESP 
> because the next IP protocol must be ESP.

Following are the two options I could think of
1) In the control path,
         if (capability & HW_SUPPORTS_FLOW)
               sa->flow = create_flow();

         if (sa->flow == NULL && !(capability & HW_NEEDS_METADATA))
               /* error */

2) Add a new submit API (submit a packet to a flow). This effectively 
decouples pattern matching from performing actions on a flow. If we have 
submit API, we could make flow mandatory for Rx & Tx.

Thanks,
Anoob



More information about the dev mailing list