[dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress flow actions

Nelio Laranjeiro nelio.laranjeiro at 6wind.com
Fri Dec 15 17:53:52 CET 2017


Hi Anoob,

Seems you want to address a lot of stuff where is should be done in
a different series, please see below,

On Fri, Dec 15, 2017 at 09:09:00PM +0530, Anoob Joseph wrote:
> Hi Nelio,
> 
> 
> On 15-12-2017 19:23, Nelio Laranjeiro wrote:
> > Hi Anoob,
> > 
> > On Fri, Dec 15, 2017 at 02:35:12PM +0530, Anoob Joseph wrote:
> > > Hi Nelio,
> > > 
> > > On 12/14/2017 08:44 PM, Nelio Laranjeiro wrote:
> > > > Add Egress flow create for devices supporting
> > > > RTE_SECURITY_TX_HW_TRAILER_OFFLOAD.
> > > > 
> > > > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro at 6wind.com>
> > > > ---
> > > >    examples/ipsec-secgw/ipsec.c | 8 ++++++++
> > > >    1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
> > > > index 8e8dc6df7..d49970ad8 100644
> > > > --- a/examples/ipsec-secgw/ipsec.c
> > > > +++ b/examples/ipsec-secgw/ipsec.c
> > > > @@ -201,6 +201,7 @@ 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);
> > > > @@ -253,6 +254,13 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa)
> > > >    							&err);
> > > >    				if (ret)
> > > >    					goto flow_create_failure;
> > > > +			} else if (sa->attr.egress &&
> > > > +				   (sa->ol_flags &
> > > > +				    RTE_SECURITY_TX_HW_TRAILER_OFFLOAD)) {
> > > If this flag is not set, the following code won't be executed, but it would
> > > still try to create the flow.
> > Right, with actions Security + END as the original code.
> > 
> > > And if the flow create fails in that case then create_session would fail.
> > Do you mean the original code is also wrong?
> I would say it's not handling all the cases. Just like how we finalized the
> ingress, egress might also need some work. Or may be we can retain the
> original behavior with this patch and take up this issue separately.

It is better to not mix everything, the final work can be done after.

> > > I would suggest moving the flow_create also into the block (for
> > > ingress and egress). Or may be initialize the flow with
> > > actions END+END+END, and add SECURITY+<RSS/QUEUE/PASSTHRU>+END as it hits
> > > various conditions. I'm not sure what the flow_create would do for such an
> > > action. This would look ugly in any case. See if you get any better ideas!
> > I think this comment is related to second patch where the
> > "sa->action[1].type = RTE_FLOW_ACTION_TYPE_END;" is wrongly removed.
> > 
> > Can you confirm before I send a new revision?
> No. I was suggesting an alternate algorithm to handle the situation when
> egress may/may not create flow while ingress would need flow by default.
> What I suggested is something like this,

The default behavior of this function for
RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO is to call a flow in both side
Ingress and Egress.  Default behavior coded in main repository in this
file [1].

This series is only adding final actions to respect the generic flow
API.

> sa->action[0].type = RTE_FLOW_ACTION_TYPE_END;
> sa->action[1].type = RTE_FLOW_ACTION_TYPE_END;
> sa->action[2].type = RTE_FLOW_ACTION_TYPE_END;

An END is final independently of what is after, as the extra actions are
only handled in their respective if branches, no need to initialize
everything to END.

> if (ingress) {
>     sa->action[0].type = RTE_FLOW_ACTION_TYPE_SECURITY;
>     ...
> } else if (egress && FLAG_ENABLED) {
>     sa->action[0].type = RTE_FLOW_ACTION_TYPE_SECURITY;
>     ...
> }
> 
> flow_create();
> 
> On second thought, this may not work well. Another suggestion is,
> 
> if (ingress) {
>     sa->action[0].type = RTE_FLOW_ACTION_TYPE_SECURITY;
>     ...
>     flow_create();
> } else if (egress && FLAG_ENABLED) {
>     sa->action[0].type = RTE_FLOW_ACTION_TYPE_SECURITY;
>     ...
>     flow_create();
> }
> // Here if flow_create fails, create_session should fail.
> // Either flow or metadata flag is required
> if (sa->flow == NULL && !(NEEDS_METADATA)) {
>     return -1;
> }

This patch scope is done to respect generic flow API calls for
RTE_SECURITY_TX_HW_TRAILER_OFFLOAD devices as written in the commit log.
For RTE_SECURITY_TX_OLOAD_NEED_MDATA devices, it should be handled in a
separate patch/series, the default behavior is maintained for them.

> > > > +					sa->action[1].type =
> > > > +						RTE_FLOW_ACTION_TYPE_PASSTHRU;
> > > > +					sa->action[2].type =
> > > > +						RTE_FLOW_ACTION_TYPE_END;
> > > >    			}
> > > >    flow_create:
> > > >    			sa->flow = rte_flow_create(sa->portid,
> > Thanks,
> > 
> 

Thanks,

[1] https://dpdk.org/browse/dpdk/tree/examples/ipsec-secgw/ipsec.c#n132

-- 
Nélio Laranjeiro
6WIND


More information about the dev mailing list