From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anoob Joseph Subject: Re: [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions Date: Tue, 12 Dec 2017 19:34:31 +0530 Message-ID: References: <5d3fdd0c05d5f8afd3f8e38ca03eaf25187d5c98.1513000931.git.nelio.laranjeiro@6wind.com> <5777791b-3dd6-f746-aa37-d572c108f042@caviumnetworks.com> <20171212134456.4x3uaus2poovddlf@laranjeiro-vm.dev.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: anoob.joseph@caviumnetworks.com, Sergio Gonzalez Monroy , Radu Nicolau , dev@dpdk.org To: Nelio Laranjeiro Return-path: Received: from NAM02-BL2-obe.outbound.protection.outlook.com (mail-bl2nam02on0062.outbound.protection.outlook.com [104.47.38.62]) by dpdk.org (Postfix) with ESMTP id 37B627CE5 for ; Tue, 12 Dec 2017 15:04:41 +0100 (CET) In-Reply-To: <20171212134456.4x3uaus2poovddlf@laranjeiro-vm.dev.6wind.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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. > 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. 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, >