From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f54.google.com (mail-wm0-f54.google.com [74.125.82.54]) by dpdk.org (Postfix) with ESMTP id 337B07D19 for ; Wed, 13 Dec 2017 15:46:40 +0100 (CET) Received: by mail-wm0-f54.google.com with SMTP id t8so5501195wmc.3 for ; Wed, 13 Dec 2017 06:46:40 -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=q95715p8LocSY66BfQmjZHg/H++YadxrG7nPe6eh+C0=; b=1Yo23KI/RKrRSOeQdI9Z928ZWZ7LRzzGtpJ7XSdvzV/HhzVAQBCPGEMDbxEJ9uH0n9 V+mTDEX+WYkwXmIgBaoGKEMoF1F9WmL7TPuaiV8rKBVzy2kNUew092vO9IMCfiaHPuEa qeGRcRjM8r8IrnX+YS2aHtYaacDS5KqEYd69ie8qzZ1bK8F+st7uCsoMR1pyJ/b3USB5 iV6meSN2BBnXAeBpZIvvVx667WR3NQUIY2TXxSGSuTYQiismosCHrc9slKaHlJb0VqhC jQyThhmc70oOPYSt53TT6YgfGlhwLYiumcUQlWaqHksz/CR8PiQrbg9p+7JdMinZWn28 Ek5Q== 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=q95715p8LocSY66BfQmjZHg/H++YadxrG7nPe6eh+C0=; b=OQSlzPRm6hAwujb0zfzL23VFVVbwAue1w7AahBCE0DPo72TJ4TH3XUfn4nKYNJfEA/ Ot4bsE8O9GMFC+db0F/mr0/TKwJWjmSPEWUyvedqTOKUMAcCCPAUyq6vdcSBs34rmE6e OmnYjXy5x6D/EbOZdzYhbyx6drq7LUJUcpJ24+XWg3xkpJ5gkBLdpeyOk3doY+Mi8hYV jsFLlz6M+LAN4cZ2EhfNk3iLH8VgBVsJI5lNQ8mpW3eQj9Odg2SaM2sr7x2Yu3qB2o8u cYWzo0IGSLfqvSjk6tfNWosQ68jV9F2zWifJ4TU6+m/mubnVZgto7OY5qlnyI1J1i0zH zJhg== X-Gm-Message-State: AKGB3mKxPzC9nnVtyj8cnMnIyadBi61ijns11MCw419dYIJyQF5VCd6R Wxu7csmXHu23LTCJl4KhwL3g X-Google-Smtp-Source: ACJfBosIbeVDOaTrzuIcqM+srGrTGo57ZzC+XrWdjaesbqtqaCpgL4LPT0ubXTYbQfiKdkY/NrAEXA== X-Received: by 10.80.203.13 with SMTP id g13mr8175139edi.14.1513176399752; Wed, 13 Dec 2017 06:46:39 -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 t23sm1592268edb.70.2017.12.13.06.46.38 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 13 Dec 2017 06:46:38 -0800 (PST) Date: Wed, 13 Dec 2017 15:47:10 +0100 From: Nelio Laranjeiro To: Anoob Joseph , Sergio Gonzalez Monroy , Radu Nicolau Cc: dev@dpdk.org Message-ID: <20171213144710.tyqvcvpkrymzqriv@laranjeiro-vm.dev.6wind.com> References: <5d3fdd0c05d5f8afd3f8e38ca03eaf25187d5c98.1513000931.git.nelio.laranjeiro@6wind.com> <5777791b-3dd6-f746-aa37-d572c108f042@caviumnetworks.com> <20171212134456.4x3uaus2poovddlf@laranjeiro-vm.dev.6wind.com> <20171212143800.ggdtdfnbknttr45g@laranjeiro-vm.dev.6wind.com> <047cadcf-13dc-5368-4ad5-a27ff25c42f8@caviumnetworks.com> <20171213100237.uvpbg2qewhxqgaln@laranjeiro-vm.dev.6wind.com> <817bec1f-4bff-ebdc-07b4-f8f24ec2084a@caviumnetworks.com> <20171213125353.2zyllxk7pwkncm76@laranjeiro-vm.dev.6wind.com> <577d100a-d021-1219-89e3-721e02b77b90@caviumnetworks.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <577d100a-d021-1219-89e3-721e02b77b90@caviumnetworks.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Dec 2017 14:46:40 -0000 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 > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > I do agree that this is outside the scope of this patch, but I was just > > > > > > > curious about the behavior since you touched the topic. > > > > > > > > > > + } > > > > > > > > > > +flow_create: > > > > > > > > > > sa->flow = rte_flow_create(sa->portid, > > > > > > > > > > &sa->attr, sa->pattern, sa->action, &err); > > > > > > > > > > if (sa->flow == NULL) { > > > > > > > > > > +flow_create_failure: > > > > > > > > > > RTE_LOG(ERR, IPSEC, > > > > > > > > > > "Failed to create ipsec flow msg: %s\n", > > > > > > > > > > err.message); > > > > > > > > > > diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h > > > > > > > > > > index 775b316ff..3c367d392 100644 > > > > > > > > > > --- a/examples/ipsec-secgw/ipsec.h > > > > > > > > > > +++ b/examples/ipsec-secgw/ipsec.h > > > > > > > > > > @@ -133,7 +133,7 @@ struct ipsec_sa { > > > > > > > > > > uint32_t ol_flags; > > > > > > > > > > #define MAX_RTE_FLOW_PATTERN (4) > > > > > > > > > > -#define MAX_RTE_FLOW_ACTIONS (2) > > > > > > > > > > +#define MAX_RTE_FLOW_ACTIONS (3) > > > > > > > > > > struct rte_flow_item pattern[MAX_RTE_FLOW_PATTERN]; > > > > > > > > > > struct rte_flow_action action[MAX_RTE_FLOW_ACTIONS]; > > > > > > > > > > struct rte_flow_attr attr; > > > > > > > > Thanks, > > > > > > Regards, > > > > > > > > Regards, > > > -- Nélio Laranjeiro 6WIND