From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <nelio.laranjeiro@6wind.com>
Received: from mail-wm0-f51.google.com (mail-wm0-f51.google.com [74.125.82.51])
by dpdk.org (Postfix) with ESMTP id B345E1D7
for <dev@dpdk.org>; Fri, 15 Dec 2017 17:53:17 +0100 (CET)
Received: by mail-wm0-f51.google.com with SMTP id g130so31497118wme.0
for <dev@dpdk.org>; Fri, 15 Dec 2017 08:53:17 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=6wind-com.20150623.gappssmtp.com; s=20150623;
h=date:from:to:cc:subject:message-id:references:mime-version
:content-disposition:content-transfer-encoding:in-reply-to
:user-agent; bh=Y1HVVaeolLw8wf3cSiQV1Uwqp9VSZx4ZczNzNGV0HoQ=;
b=ozXjZNMZ2k0bjQlCeSBWWatbnjmn297TfXzSu1mjJ8FMQRtkG1GByJ3yz7M5ePZnWK
qmRWwiIzy1B/kjERZAy6Kxnhr5N13YnxvItQ328KGlIunJUlHxbZy+Pmw9uRlTK3VKyj
xZfSRR4/j/v+hpriyOwfpXTfWwC4Q5AzEycCsQpe5/c6hzsdYhxNvQbd2bXY12dOmJWO
XXVTRTJkVpi7JEErYlp9EX/Gw2I6raRacMvyBaeXlCNHQQ98Ku2OB8V/XdK/7Zgz52K5
nvRN2fIHaTADBEaJUePW/MFd13ws2tIueYod+O4cebx236exL9HigOk02/SuglZExk2n
CUcg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:date:from:to:cc:subject:message-id:references
:mime-version:content-disposition:content-transfer-encoding
:in-reply-to:user-agent;
bh=Y1HVVaeolLw8wf3cSiQV1Uwqp9VSZx4ZczNzNGV0HoQ=;
b=r94jj0kjICDjoy1/WRFyNVhb0zy1sj/c/5GynJVBSCaOxLDSpTPBO1lpoeAReVZUgR
LI6D65NtE70CB7jifyV1REcBrOj1jHXzh6TBYO4Zu9GOK6fD6E96pI3GyyE2jwe3QLrC
KmRXrtALKU4kWNt6OuiUkuxHQ+q7fINXnIvZpJvV2kfKUMJs32Zht+G+oBSj+u8iDlVs
hi05EYQqAmNncHp1EdokuGJVOk0fkiV/+QaJvGzCVyQVeS/aai+nCUHde1kIkpGHhRXO
EmmW+dvF31rHIc0PUn7UB8TTrLBep6h4vYlm79pwOjj+c+gT5wG1pGiZbWNTcGM1Ze+O
Lrsg==
X-Gm-Message-State: AKGB3mInr6mlryRmfkQL3WnRBrAIuuDn/y0SEQn6MLcScGr/FBzYwIqp
S5wB1aFCVi2oai3sK+6a6iu2
X-Google-Smtp-Source: ACJfBoss7gGk6ATnJG4E2YgtY99hsqH06MhjbXlDZvHaPGo6Xcx7iuUgacGYLGQLsNqF+vP8FxlL+g==
X-Received: by 10.80.136.113 with SMTP id c46mr17824043edc.112.1513356797239;
Fri, 15 Dec 2017 08:53:17 -0800 (PST)
Received: from laranjeiro-vm.dev.6wind.com
(host.78.145.23.62.rev.coltfrance.com. [62.23.145.78])
by smtp.gmail.com with ESMTPSA id t23sm6149143edb.70.2017.12.15.08.53.15
(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);
Fri, 15 Dec 2017 08:53:16 -0800 (PST)
Date: Fri, 15 Dec 2017 17:53:52 +0100
From: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
To: Anoob Joseph <anoob.joseph@caviumnetworks.com>
Cc: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>,
Radu Nicolau <radu.nicolau@intel.com>, dev@dpdk.org
Message-ID: <20171215165352.htocozxulv74e6xc@laranjeiro-vm.dev.6wind.com>
References: <5d3fdd0c05d5f8afd3f8e38ca03eaf25187d5c98.1513000931.git.nelio.laranjeiro@6wind.com>
<89add3272024fefe644a9e636a476c85d39e398b.1513264386.git.nelio.laranjeiro@6wind.com>
<78f97959-bf6c-33cc-e758-d232013ea159@caviumnetworks.com>
<20171215135300.zm6ubao24qqxstpl@laranjeiro-vm.dev.6wind.com>
<f65fbef7-afd5-6f2c-b1b9-f7764d62a2e3@caviumnetworks.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <f65fbef7-afd5-6f2c-b1b9-f7764d62a2e3@caviumnetworks.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Subject: Re: [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress flow
actions
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Fri, 15 Dec 2017 16:53:17 -0000
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@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