* [dpdk-dev] [PATCH 1/2] examples/ipsec-secgw: fix missing ingress flow attribute
@ 2017-11-23 15:12 Nelio Laranjeiro
2017-11-23 15:12 ` [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro
0 siblings, 1 reply; 56+ messages in thread
From: Nelio Laranjeiro @ 2017-11-23 15:12 UTC (permalink / raw)
To: Sergio Gonzalez Monroy, Radu Nicolau; +Cc: dev, akhil.goyal
Generic flow API have both direction bits, ingress and egress for rules
which may work on both sides.
Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
Cc: akhil.goyal@nxp.com
Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
examples/ipsec-secgw/ipsec.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
index ec8bf95e1..17bd7620d 100644
--- a/examples/ipsec-secgw/ipsec.c
+++ b/examples/ipsec-secgw/ipsec.c
@@ -205,6 +205,8 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa)
sa->attr.egress = (sa->direction ==
RTE_SECURITY_IPSEC_SA_DIR_EGRESS);
+ sa->attr.ingress = (sa->direction ==
+ RTE_SECURITY_IPSEC_SA_DIR_INGRESS);
sa->flow = rte_flow_create(sa->portid,
&sa->attr, sa->pattern, sa->action, &err);
if (sa->flow == NULL) {
--
2.11.0
^ permalink raw reply [flat|nested] 56+ messages in thread* [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-11-23 15:12 [dpdk-dev] [PATCH 1/2] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro @ 2017-11-23 15:12 ` Nelio Laranjeiro 2017-11-29 12:30 ` Anoob ` (2 more replies) 0 siblings, 3 replies; 56+ messages in thread From: Nelio Laranjeiro @ 2017-11-23 15:12 UTC (permalink / raw) To: Sergio Gonzalez Monroy, Radu Nicolau; +Cc: dev Mellanox INNOVA NIC needs to have final target queue actions to perform inline crypto. Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> --- examples/ipsec-secgw/ipsec.c | 27 ++++++++++++++++++++++++++- examples/ipsec-secgw/ipsec.h | 2 +- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index 17bd7620d..e967f88b3 100644 --- a/examples/ipsec-secgw/ipsec.c +++ b/examples/ipsec-secgw/ipsec.c @@ -142,6 +142,22 @@ 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; + 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->sec_session = rte_security_session_create(ctx, &sess_conf, ipsec_ctx->session_pool); @@ -201,7 +217,16 @@ 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->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; + sa->action[2].type = RTE_FLOW_ACTION_TYPE_END; sa->attr.egress = (sa->direction == RTE_SECURITY_IPSEC_SA_DIR_EGRESS); diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h index 775b316ff..82ffc1c6d 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 (4) struct rte_flow_item pattern[MAX_RTE_FLOW_PATTERN]; struct rte_flow_action action[MAX_RTE_FLOW_ACTIONS]; struct rte_flow_attr attr; -- 2.11.0 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-11-23 15:12 ` [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro @ 2017-11-29 12:30 ` Anoob 2017-11-29 12:50 ` Nelio Laranjeiro 2017-12-04 14:11 ` [dpdk-dev] [PATCH v2 1/2] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro 2017-12-04 14:11 ` [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2 siblings, 1 reply; 56+ messages in thread From: Anoob @ 2017-11-29 12:30 UTC (permalink / raw) To: Nelio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau; +Cc: dev Hi Nelio, Since support of RSS with inline crypto/protocol is hardware implementation dependent, it would be better if there is some sort of capability check before setting the flow parameters in the application. If the hardware doesn't support RSS with inline processing, then the RSS flow action will have to be ignored in the driver. This wouldn't look right from application's point of view. And also the PMD would need application-specific logic to handle such cases, which may not scale well. Thanks, Anoob On 11/23/2017 08:42 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@6wind.com> > --- > examples/ipsec-secgw/ipsec.c | 27 ++++++++++++++++++++++++++- > examples/ipsec-secgw/ipsec.h | 2 +- > 2 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c > index 17bd7620d..e967f88b3 100644 > --- a/examples/ipsec-secgw/ipsec.c > +++ b/examples/ipsec-secgw/ipsec.c > @@ -142,6 +142,22 @@ 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; > + 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->sec_session = rte_security_session_create(ctx, > &sess_conf, ipsec_ctx->session_pool); > @@ -201,7 +217,16 @@ 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->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; > + sa->action[2].type = RTE_FLOW_ACTION_TYPE_END; > > sa->attr.egress = (sa->direction == > RTE_SECURITY_IPSEC_SA_DIR_EGRESS); > diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h > index 775b316ff..82ffc1c6d 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 (4) > struct rte_flow_item pattern[MAX_RTE_FLOW_PATTERN]; > struct rte_flow_action action[MAX_RTE_FLOW_ACTIONS]; > struct rte_flow_attr attr; ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-11-29 12:30 ` Anoob @ 2017-11-29 12:50 ` Nelio Laranjeiro 2017-11-30 10:46 ` Anoob 0 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-11-29 12:50 UTC (permalink / raw) To: Anoob; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev Hi Anoob, On Wed, Nov 29, 2017 at 06:00:38PM +0530, Anoob wrote: > Hi Nelio, > > Since support of RSS with inline crypto/protocol is hardware > implementation dependent, it would be better if there is some sort of > capability check before setting the flow parameters in the application. > > If the hardware doesn't support RSS with inline processing, then the RSS > flow action will have to be ignored in the driver. This wouldn't look > right from application's point of view. And also the PMD would need > application-specific logic to handle such cases, which may not scale well. There is a real issue here, RTE_FLOW API needs a terminal action, security is not one [1] you must have one of the followings: QUEUE, DROP, RSS, PF, VF or PASSTHRU. Flow API does not work with "capabilities" as the application can verify the rule using the validate(). If it cannot be validated the application can test another kind of rule until the PMD returns a success. Here, I am proposing the RSS as RSS with a single queue is equivalent to queue. On Mellanox NIC we need the RSS or QUEUE in ingress and for Egress PASSTHRU is good. What are your needs? Regards, > Thanks, > Anoob > > On 11/23/2017 08:42 PM, Nelio Laranjeiro wrote: > > Mellanox INNOVA NIC needs to have final target queue actions to perform > inline crypto. > > Signed-off-by: Nelio Laranjeiro [1]<nelio.laranjeiro@6wind.com> > --- > examples/ipsec-secgw/ipsec.c | 27 ++++++++++++++++++++++++++- > examples/ipsec-secgw/ipsec.h | 2 +- > 2 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c > index 17bd7620d..e967f88b3 100644 > --- a/examples/ipsec-secgw/ipsec.c > +++ b/examples/ipsec-secgw/ipsec.c > @@ -142,6 +142,22 @@ 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; > + 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->sec_session = rte_security_session_create(ctx, > &sess_conf, ipsec_ctx->session_pool); > @@ -201,7 +217,16 @@ 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->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; > + sa->action[2].type = RTE_FLOW_ACTION_TYPE_END; > > sa->attr.egress = (sa->direction == > RTE_SECURITY_IPSEC_SA_DIR_EGRESS); > diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h > index 775b316ff..82ffc1c6d 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 (4) > struct rte_flow_item pattern[MAX_RTE_FLOW_PATTERN]; > struct rte_flow_action action[MAX_RTE_FLOW_ACTIONS]; > struct rte_flow_attr attr; > > References > > Visible links > 1. mailto:nelio.laranjeiro@6wind.com [1] http://dpdk.org/doc/guides/prog_guide/rte_flow.html?highlight=rte_flow#actions -- Nélio Laranjeiro 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-11-29 12:50 ` Nelio Laranjeiro @ 2017-11-30 10:46 ` Anoob 2017-11-30 12:28 ` Nelio Laranjeiro 0 siblings, 1 reply; 56+ messages in thread From: Anoob @ 2017-11-30 10:46 UTC (permalink / raw) To: Nelio Laranjeiro Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev, Narayana Prasad, Jerin Jacob Hi Nelio, Please see inline. Thanks, Anoob On 11/29/2017 06:20 PM, Nelio Laranjeiro wrote: > Hi Anoob, > > On Wed, Nov 29, 2017 at 06:00:38PM +0530, Anoob wrote: >> Hi Nelio, >> >> Since support of RSS with inline crypto/protocol is hardware >> implementation dependent, it would be better if there is some sort of >> capability check before setting the flow parameters in the application. >> >> If the hardware doesn't support RSS with inline processing, then the RSS >> flow action will have to be ignored in the driver. This wouldn't look >> right from application's point of view. And also the PMD would need >> application-specific logic to handle such cases, which may not scale well. > There is a real issue here, RTE_FLOW API needs a terminal action, security is > not one [1] you must have one of the followings: QUEUE, DROP, RSS, PF, > VF or PASSTHRU. > > Flow API does not work with "capabilities" as the application can verify > the rule using the validate(). If it cannot be validated the > application can test another kind of rule until the PMD returns a > success. > > Here, I am proposing the RSS as RSS with a single queue is equivalent to queue. > > On Mellanox NIC we need the RSS or QUEUE in ingress and for Egress PASSTHRU > is good. > > What are your needs? Thanks for the clarification. Understood the issue here. On Cavium hardware SECURITY will be terminating. So a better approach would be to first check from the application (using rte_flow_verify()) if SECURITY is terminating action. If it fails, then application can do RSS/QUEUE. That should solve the issue. > > Regards, > >> Thanks, >> Anoob >> >> On 11/23/2017 08:42 PM, Nelio Laranjeiro wrote: >> >> Mellanox INNOVA NIC needs to have final target queue actions to perform >> inline crypto. >> >> Signed-off-by: Nelio Laranjeiro [1]<nelio.laranjeiro@6wind.com> >> --- >> examples/ipsec-secgw/ipsec.c | 27 ++++++++++++++++++++++++++- >> examples/ipsec-secgw/ipsec.h | 2 +- >> 2 files changed, 27 insertions(+), 2 deletions(-) >> >> diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c >> index 17bd7620d..e967f88b3 100644 >> --- a/examples/ipsec-secgw/ipsec.c >> +++ b/examples/ipsec-secgw/ipsec.c >> @@ -142,6 +142,22 @@ 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; >> + 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->sec_session = rte_security_session_create(ctx, >> &sess_conf, ipsec_ctx->session_pool); >> @@ -201,7 +217,16 @@ 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->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; >> + sa->action[2].type = RTE_FLOW_ACTION_TYPE_END; >> >> sa->attr.egress = (sa->direction == >> RTE_SECURITY_IPSEC_SA_DIR_EGRESS); >> diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h >> index 775b316ff..82ffc1c6d 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 (4) >> struct rte_flow_item pattern[MAX_RTE_FLOW_PATTERN]; >> struct rte_flow_action action[MAX_RTE_FLOW_ACTIONS]; >> struct rte_flow_attr attr; >> >> References >> >> Visible links >> 1. mailto:nelio.laranjeiro@6wind.com > [1] http://dpdk.org/doc/guides/prog_guide/rte_flow.html?highlight=rte_flow#actions > ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-11-30 10:46 ` Anoob @ 2017-11-30 12:28 ` Nelio Laranjeiro 2017-12-01 15:04 ` Anoob Joseph 0 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-11-30 12:28 UTC (permalink / raw) To: Anoob Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev, Narayana Prasad, Jerin Jacob Hi Annob, On Thu, Nov 30, 2017 at 04:16:23PM +0530, Anoob wrote: > On 11/29/2017 06:20 PM, Nelio Laranjeiro wrote: > > Hi Anoob, > > > > On Wed, Nov 29, 2017 at 06:00:38PM +0530, Anoob wrote: > > > Hi Nelio, > > > > > > Since support of RSS with inline crypto/protocol is hardware > > > implementation dependent, it would be better if there is some sort of > > > capability check before setting the flow parameters in the application. > > > > > > If the hardware doesn't support RSS with inline processing, then the RSS > > > flow action will have to be ignored in the driver. This wouldn't look > > > right from application's point of view. And also the PMD would need > > > application-specific logic to handle such cases, which may not scale well. > > There is a real issue here, RTE_FLOW API needs a terminal action, security is > > not one [1] you must have one of the followings: QUEUE, DROP, RSS, PF, > > VF or PASSTHRU. > > > > Flow API does not work with "capabilities" as the application can verify > > the rule using the validate(). If it cannot be validated the > > application can test another kind of rule until the PMD returns a > > success. > > > > Here, I am proposing the RSS as RSS with a single queue is equivalent to queue. > > > > On Mellanox NIC we need the RSS or QUEUE in ingress and for Egress PASSTHRU > > is good. > > > > What are your needs? > Thanks for the clarification. Understood the issue here. On Cavium hardware > SECURITY will be terminating. You should finalise with PASSTHRU to be compliant with the API, otherwise application makers won't understand why it does not work according to the API implementation. > So a better approach would be to first check from the application > (using rte_flow_verify()) if SECURITY is terminating action. If it > fails, then application can do RSS/QUEUE. That should solve > the issue. <snip> I think we have an agreement here, in order the final action to be tested: 1. PASSTHRU 2. RSS 3. QUEUE If those 3 fails, the functions fails to create the rule, the first succeeding is the one applied. Do you agree? Thanks, -- Nélio Laranjeiro 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-11-30 12:28 ` Nelio Laranjeiro @ 2017-12-01 15:04 ` Anoob Joseph 2017-12-01 16:26 ` Nelio Laranjeiro 0 siblings, 1 reply; 56+ messages in thread From: Anoob Joseph @ 2017-12-01 15:04 UTC (permalink / raw) To: Nelio Laranjeiro Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev, Narayana Prasad, Jerin Jacob Hi Nelio, On 30-11-2017 17:58, Nelio Laranjeiro wrote: > Hi Annob, > > On Thu, Nov 30, 2017 at 04:16:23PM +0530, Anoob wrote: >> On 11/29/2017 06:20 PM, Nelio Laranjeiro wrote: >>> Hi Anoob, >>> >>> On Wed, Nov 29, 2017 at 06:00:38PM +0530, Anoob wrote: >>>> Hi Nelio, >>>> >>>> Since support of RSS with inline crypto/protocol is hardware >>>> implementation dependent, it would be better if there is some sort of >>>> capability check before setting the flow parameters in the application. >>>> >>>> If the hardware doesn't support RSS with inline processing, then the RSS >>>> flow action will have to be ignored in the driver. This wouldn't look >>>> right from application's point of view. And also the PMD would need >>>> application-specific logic to handle such cases, which may not scale well. >>> There is a real issue here, RTE_FLOW API needs a terminal action, security is >>> not one [1] you must have one of the followings: QUEUE, DROP, RSS, PF, >>> VF or PASSTHRU. >>> >>> Flow API does not work with "capabilities" as the application can verify >>> the rule using the validate(). If it cannot be validated the >>> application can test another kind of rule until the PMD returns a >>> success. >>> >>> Here, I am proposing the RSS as RSS with a single queue is equivalent to queue. >>> >>> On Mellanox NIC we need the RSS or QUEUE in ingress and for Egress PASSTHRU >>> is good. >>> >>> What are your needs? >> Thanks for the clarification. Understood the issue here. On Cavium hardware >> SECURITY will be terminating. > You should finalise with PASSTHRU to be compliant with the API, > otherwise application makers won't understand why it does not work > according to the API implementation. Cavium hardware would be supporting only terminating actions. So PASSTHRU will not be supported. >> So a better approach would be to first check from the application >> (using rte_flow_verify()) if SECURITY is terminating action. If it >> fails, then application can do RSS/QUEUE. That should solve >> the issue. > <snip> > > I think we have an agreement here, in order the final action to be > tested: > > 1. PASSTHRU > 2. RSS > 3. QUEUE > > If those 3 fails, the functions fails to create the rule, the first > succeeding is the one applied. PASSTHRU itself is non-terminating, right? So I'm not sure, how a check with PASSTHRU would help us. SECURITY will be terminating action on Cavium hardware. So, the first check could be without any addition. If that fails, RSS. And then QUEUE. That should be fine. Any thoughts? Anoob > > Do you agree? > > Thanks, > ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-01 15:04 ` Anoob Joseph @ 2017-12-01 16:26 ` Nelio Laranjeiro 0 siblings, 0 replies; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-01 16:26 UTC (permalink / raw) To: Anoob Joseph Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev, Narayana Prasad, Jerin Jacob Hi Annob On Fri, Dec 01, 2017 at 08:34:04PM +0530, Anoob Joseph wrote: <snip> > > > > I think we have an agreement here, in order the final action to be > > tested: > > > > 1. PASSTHRU > > 2. RSS > > 3. QUEUE > > > > If those 3 fails, the functions fails to create the rule, the first > > succeeding is the one applied. > PASSTHRU itself is non-terminating, right? So I'm not sure, how a check with > PASSTHRU would help us. SECURITY will be terminating action on Cavium > hardware. So, the first check could be without any addition. If that fails, > RSS. And then QUEUE. That should be fine. Agreed, assuming PASSTHRU [1] is not mandatory (i.e. explicit) when the rule action list is not terminal. A small suggestion, you should accept/ignore such action in your PMD, the API also allows applications to add it. Thanks, [1] http://dpdk.org/doc/guides/prog_guide/rte_flow.html#action-types -- Nélio Laranjeiro 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* [dpdk-dev] [PATCH v2 1/2] examples/ipsec-secgw: fix missing ingress flow attribute 2017-11-23 15:12 ` [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-11-29 12:30 ` Anoob @ 2017-12-04 14:11 ` Nelio Laranjeiro 2017-12-11 11:50 ` Radu Nicolau 2017-12-04 14:11 ` [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-04 14:11 UTC (permalink / raw) To: Sergio Gonzalez Monroy, Radu Nicolau, Anoob Joseph; +Cc: dev, akhil.goyal Generic flow API have both direction bits, ingress and egress for rules which may work on both sides. Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload") Cc: akhil.goyal@nxp.com Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> --- examples/ipsec-secgw/ipsec.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index ec8bf95e1..17bd7620d 100644 --- a/examples/ipsec-secgw/ipsec.c +++ b/examples/ipsec-secgw/ipsec.c @@ -205,6 +205,8 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa) sa->attr.egress = (sa->direction == RTE_SECURITY_IPSEC_SA_DIR_EGRESS); + sa->attr.ingress = (sa->direction == + RTE_SECURITY_IPSEC_SA_DIR_INGRESS); sa->flow = rte_flow_create(sa->portid, &sa->attr, sa->pattern, sa->action, &err); if (sa->flow == NULL) { -- 2.11.0 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v2 1/2] examples/ipsec-secgw: fix missing ingress flow attribute 2017-12-04 14:11 ` [dpdk-dev] [PATCH v2 1/2] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro @ 2017-12-11 11:50 ` Radu Nicolau 0 siblings, 0 replies; 56+ messages in thread From: Radu Nicolau @ 2017-12-11 11:50 UTC (permalink / raw) To: Nelio Laranjeiro, Sergio Gonzalez Monroy, Anoob Joseph; +Cc: dev, akhil.goyal On 12/4/2017 2:11 PM, Nelio Laranjeiro wrote: > Generic flow API have both direction bits, ingress and egress for rules > which may work on both sides. > > Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload") > Cc: akhil.goyal@nxp.com > > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> > --- > Acked-by: Radu Nicolau <radu.nicolau@intel.com> ^ permalink raw reply [flat|nested] 56+ messages in thread
* [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-11-23 15:12 ` [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-11-29 12:30 ` Anoob 2017-12-04 14:11 ` [dpdk-dev] [PATCH v2 1/2] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro @ 2017-12-04 14:11 ` Nelio Laranjeiro 2017-12-07 9:47 ` Anoob ` (3 more replies) 2 siblings, 4 replies; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-04 14:11 UTC (permalink / raw) To: Sergio Gonzalez Monroy, Radu Nicolau, Anoob Joseph; +Cc: dev Mellanox INNOVA NIC needs to have final target queue actions to perform inline crypto. Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> --- Changes in v2: * Test the rule by PASSTHRU/RSS/QUEUE and apply the first one validated. --- examples/ipsec-secgw/ipsec.c | 81 ++++++++++++++++++++++++++++++++++++++++---- examples/ipsec-secgw/ipsec.h | 2 +- 2 files changed, 76 insertions(+), 7 deletions(-) diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index 17bd7620d..f8823fb94 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); @@ -173,6 +174,10 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa) return -1; } + sa->attr.egress = (sa->direction == + RTE_SECURITY_IPSEC_SA_DIR_EGRESS); + sa->attr.ingress = (sa->direction == + RTE_SECURITY_IPSEC_SA_DIR_INGRESS); sa->ol_flags = sec_cap->ol_flags; sa->security_ctx = ctx; sa->pattern[0].type = RTE_FLOW_ITEM_TYPE_ETH; @@ -201,15 +206,79 @@ 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 implicitly PASSTHRU, it can also be + * explicit. + */ + sa->action[1].type = RTE_FLOW_ACTION_TYPE_END; + ret = rte_flow_validate(sa->portid, &sa->attr, + sa->pattern, sa->action, + &err); + if (!ret) + goto flow_create; + /* 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. */ + for (i = 0; + i < eth_dev->data->nb_rx_queues; ++i) + if (eth_dev->data->rx_queues[i]) + break; + if (i != eth_dev->data->nb_rx_queues) + return -1; + sa->action[1].type = RTE_FLOW_ACTION_TYPE_QUEUE; + sa->action[1].conf = + &(struct rte_flow_action_queue){ + .index = i, + }; + 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; + } +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..82ffc1c6d 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 (4) struct rte_flow_item pattern[MAX_RTE_FLOW_PATTERN]; struct rte_flow_action action[MAX_RTE_FLOW_ACTIONS]; struct rte_flow_attr attr; -- 2.11.0 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-04 14:11 ` [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro @ 2017-12-07 9:47 ` Anoob 2017-12-07 12:22 ` Nelio Laranjeiro 2017-12-08 14:00 ` Anoob ` (2 subsequent siblings) 3 siblings, 1 reply; 56+ messages in thread From: Anoob @ 2017-12-07 9:47 UTC (permalink / raw) To: Nelio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau Cc: dev, Narayana Prasad, Jerin Jacob Hi Nelio, On 12/04/2017 07:41 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@6wind.com> > > --- > > Changes in v2: > > * Test the rule by PASSTHRU/RSS/QUEUE and apply the first one validated. > --- > examples/ipsec-secgw/ipsec.c | 81 ++++++++++++++++++++++++++++++++++++++++---- > examples/ipsec-secgw/ipsec.h | 2 +- > 2 files changed, 76 insertions(+), 7 deletions(-) > > diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c > index 17bd7620d..f8823fb94 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); > @@ -173,6 +174,10 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa) > return -1; > } > > + sa->attr.egress = (sa->direction == > + RTE_SECURITY_IPSEC_SA_DIR_EGRESS); > + sa->attr.ingress = (sa->direction == > + RTE_SECURITY_IPSEC_SA_DIR_INGRESS); > sa->ol_flags = sec_cap->ol_flags; > sa->security_ctx = ctx; > sa->pattern[0].type = RTE_FLOW_ITEM_TYPE_ETH; > @@ -201,15 +206,79 @@ 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 implicitly PASSTHRU, it can also be > + * explicit. > + */ May be we can get rid of this check. You can do the check with RSS and then QUEUE. That should be fine. SECURITY is terminating on Cavium hardware, but according to the spec it is a non-terminating meta action. We can stick to that. For Cavium hardware the PMD will give success to SECURITY+QUEUE. That should resolve the issue. > + sa->action[1].type = RTE_FLOW_ACTION_TYPE_END; > + ret = rte_flow_validate(sa->portid, &sa->attr, > + sa->pattern, sa->action, > + &err); > + if (!ret) > + goto flow_create; > + /* 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. */ > + for (i = 0; > + i < eth_dev->data->nb_rx_queues; ++i) > + if (eth_dev->data->rx_queues[i]) > + break; > + if (i != eth_dev->data->nb_rx_queues) > + return -1; > + sa->action[1].type = RTE_FLOW_ACTION_TYPE_QUEUE; > + sa->action[1].conf = > + &(struct rte_flow_action_queue){ > + .index = i, > + }; > + 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; > + } > +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..82ffc1c6d 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 (4) > struct rte_flow_item pattern[MAX_RTE_FLOW_PATTERN]; > struct rte_flow_action action[MAX_RTE_FLOW_ACTIONS]; > struct rte_flow_attr attr; ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-07 9:47 ` Anoob @ 2017-12-07 12:22 ` Nelio Laranjeiro 0 siblings, 0 replies; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-07 12:22 UTC (permalink / raw) To: Anoob Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev, Narayana Prasad, Jerin Jacob Hi Anoob, On Thu, Dec 07, 2017 at 03:17:40PM +0530, Anoob wrote: > Hi Nelio, > > > On 12/04/2017 07:41 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@6wind.com> > > > > --- > > > > Changes in v2: > > > > * Test the rule by PASSTHRU/RSS/QUEUE and apply the first one validated. > > --- > > examples/ipsec-secgw/ipsec.c | 81 ++++++++++++++++++++++++++++++++++++++++---- > > examples/ipsec-secgw/ipsec.h | 2 +- > > 2 files changed, 76 insertions(+), 7 deletions(-) > > > > diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c > > index 17bd7620d..f8823fb94 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); > > @@ -173,6 +174,10 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa) > > return -1; > > } > > + sa->attr.egress = (sa->direction == > > + RTE_SECURITY_IPSEC_SA_DIR_EGRESS); > > + sa->attr.ingress = (sa->direction == > > + RTE_SECURITY_IPSEC_SA_DIR_INGRESS); > > sa->ol_flags = sec_cap->ol_flags; > > sa->security_ctx = ctx; > > sa->pattern[0].type = RTE_FLOW_ITEM_TYPE_ETH; > > @@ -201,15 +206,79 @@ 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 implicitly PASSTHRU, it can also be > > + * explicit. > > + */ > May be we can get rid of this check. You can do the check with RSS and then > QUEUE. That should be fine. SECURITY is terminating on Cavium hardware, but > according to the spec it is a non-terminating meta action. We can stick to > that. For Cavium hardware the PMD will give success to SECURITY+QUEUE. That > should resolve the issue. <snip> I'll remove it in a v3, I will send it tomorrow to let a little more time for other people to review. Thanks, -- Nélio Laranjeiro 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-04 14:11 ` [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-12-07 9:47 ` Anoob @ 2017-12-08 14:00 ` Anoob 2017-12-08 14:40 ` Nelio Laranjeiro 2017-12-11 14:04 ` [dpdk-dev] [PATCH v3 1/2] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro 2017-12-11 14:04 ` [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 3 siblings, 1 reply; 56+ messages in thread From: Anoob @ 2017-12-08 14:00 UTC (permalink / raw) To: Nelio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau; +Cc: dev Hi Nelio, On 12/04/2017 07:41 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@6wind.com> > > --- > > Changes in v2: > > * Test the rule by PASSTHRU/RSS/QUEUE and apply the first one validated. > --- > examples/ipsec-secgw/ipsec.c | 81 ++++++++++++++++++++++++++++++++++++++++---- > examples/ipsec-secgw/ipsec.h | 2 +- > 2 files changed, 76 insertions(+), 7 deletions(-) > > diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c > index 17bd7620d..f8823fb94 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); > @@ -173,6 +174,10 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa) > return -1; > } > > + sa->attr.egress = (sa->direction == > + RTE_SECURITY_IPSEC_SA_DIR_EGRESS); > + sa->attr.ingress = (sa->direction == > + RTE_SECURITY_IPSEC_SA_DIR_INGRESS); > sa->ol_flags = sec_cap->ol_flags; > sa->security_ctx = ctx; > sa->pattern[0].type = RTE_FLOW_ITEM_TYPE_ETH; > @@ -201,15 +206,79 @@ 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 implicitly PASSTHRU, it can also be > + * explicit. > + */ > + sa->action[1].type = RTE_FLOW_ACTION_TYPE_END; > + ret = rte_flow_validate(sa->portid, &sa->attr, > + sa->pattern, sa->action, > + &err); > + if (!ret) > + goto flow_create; > + /* 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. */ > + for (i = 0; > + i < eth_dev->data->nb_rx_queues; ++i) > + if (eth_dev->data->rx_queues[i]) > + break; Is the following check correct? > + if (i != eth_dev->data->nb_rx_queues) > + return -1; > + sa->action[1].type = RTE_FLOW_ACTION_TYPE_QUEUE; > + sa->action[1].conf = > + &(struct rte_flow_action_queue){ > + .index = i, > + }; > + 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; > + } > +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..82ffc1c6d 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 (4) > struct rte_flow_item pattern[MAX_RTE_FLOW_PATTERN]; > struct rte_flow_action action[MAX_RTE_FLOW_ACTIONS]; > struct rte_flow_attr attr; ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-08 14:00 ` Anoob @ 2017-12-08 14:40 ` Nelio Laranjeiro 2017-12-08 16:40 ` Anoob Joseph 0 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-08 14:40 UTC (permalink / raw) To: Anoob; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev On Fri, Dec 08, 2017 at 07:30:03PM +0530, Anoob wrote: > Hi Nelio, > > [...] > > + goto flow_create; > > + /* Try Queue. */ > > + for (i = 0; > > + i < eth_dev->data->nb_rx_queues; ++i) > > + if (eth_dev->data->rx_queues[i]) > > + break; > Is the following check correct? [...] For an application, it seems not necessary. The application knows which queues are configured in the drivers has it has made the configuration. Removing it in the v3. Thanks, -- Nélio Laranjeiro 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-08 14:40 ` Nelio Laranjeiro @ 2017-12-08 16:40 ` Anoob Joseph 2017-12-11 8:21 ` Nelio Laranjeiro 0 siblings, 1 reply; 56+ messages in thread From: Anoob Joseph @ 2017-12-08 16:40 UTC (permalink / raw) To: Nelio Laranjeiro; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev HI Nelio, On 08-12-2017 20:10, Nelio Laranjeiro wrote: > On Fri, Dec 08, 2017 at 07:30:03PM +0530, Anoob wrote: >> Hi Nelio, >> >> > [...] >>> + goto flow_create; >>> + /* Try Queue. */ >>> + for (i = 0; >>> + i < eth_dev->data->nb_rx_queues; ++i) >>> + if (eth_dev->data->rx_queues[i]) >>> + break; >> Is the following check correct? > [...] > > For an application, it seems not necessary. The application knows which > queues are configured in the drivers has it has made the configuration. > > Removing it in the v3. I think you misunderstood me here. I was talking about the following line. + if (i != eth_dev->data->nb_rx_queues) + return -1; Shouldn't it be? + if (i == eth_dev->data->nb_rx_queues) + return -1; Thanks, Anoob ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-08 16:40 ` Anoob Joseph @ 2017-12-11 8:21 ` Nelio Laranjeiro 2017-12-11 9:00 ` Anoob 0 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-11 8:21 UTC (permalink / raw) To: Anoob Joseph; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev Hi Anoob, On Fri, Dec 08, 2017 at 10:10:28PM +0530, Anoob Joseph wrote: > HI Nelio, > > > On 08-12-2017 20:10, Nelio Laranjeiro wrote: > > On Fri, Dec 08, 2017 at 07:30:03PM +0530, Anoob wrote: > > > Hi Nelio, > > > > > > > > [...] > > > > + goto flow_create; > > > > + /* Try Queue. */ > > > > + for (i = 0; > > > > + i < eth_dev->data->nb_rx_queues; ++i) > > > > + if (eth_dev->data->rx_queues[i]) > > > > + break; > > > Is the following check correct? > > [...] > > > > For an application, it seems not necessary. The application knows which > > queues are configured in the drivers has it has made the configuration. > > > > Removing it in the v3. > I think you misunderstood me here. Indeed, I misunderstood, > I was talking about the following line. > > + if (i != eth_dev->data->nb_rx_queues) > + return -1; > > Shouldn't it be? > > + if (i == eth_dev->data->nb_rx_queues) > + return -1; Yes it should. Anyway, I don't thing it is necessary to keep this check, from what I saw in the application source code, it initialise all Rx queues up to nb_rx_queues without leaving any hole. According to this, I'll just remove this verification, is it okay for you? Thanks, -- Nélio Laranjeiro 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-11 8:21 ` Nelio Laranjeiro @ 2017-12-11 9:00 ` Anoob 0 siblings, 0 replies; 56+ messages in thread From: Anoob @ 2017-12-11 9:00 UTC (permalink / raw) To: Nelio Laranjeiro Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev, Jerin Jacob, Narayana Prasad Hi Nelio, On 12/11/2017 01:51 PM, Nelio Laranjeiro wrote: > Hi Anoob, > > On Fri, Dec 08, 2017 at 10:10:28PM +0530, Anoob Joseph wrote: >> HI Nelio, >> >> >> On 08-12-2017 20:10, Nelio Laranjeiro wrote: >>> On Fri, Dec 08, 2017 at 07:30:03PM +0530, Anoob wrote: >>>> Hi Nelio, >>>> >>>> >>> [...] >>>>> + goto flow_create; >>>>> + /* Try Queue. */ >>>>> + for (i = 0; >>>>> + i < eth_dev->data->nb_rx_queues; ++i) >>>>> + if (eth_dev->data->rx_queues[i]) >>>>> + break; >>>> Is the following check correct? >>> [...] >>> >>> For an application, it seems not necessary. The application knows which >>> queues are configured in the drivers has it has made the configuration. >>> >>> Removing it in the v3. >> I think you misunderstood me here. > Indeed, I misunderstood, > >> I was talking about the following line. >> >> + if (i != eth_dev->data->nb_rx_queues) >> + return -1; >> >> Shouldn't it be? >> >> + if (i == eth_dev->data->nb_rx_queues) >> + return -1; > Yes it should. > > Anyway, I don't thing it is necessary to keep this check, from what I > saw in the application source code, it initialise all Rx queues up to > nb_rx_queues without leaving any hole. > According to this, I'll just remove this verification, is it okay for > you? I think you can just use Queue 0 here. So you can get rid of the checks etc. For real applications, we should have an entry in SA structure which will determine the Queue to be used. Even for RSS, something like that would be required. Thanks, Anoob ^ permalink raw reply [flat|nested] 56+ messages in thread
* [dpdk-dev] [PATCH v3 1/2] examples/ipsec-secgw: fix missing ingress flow attribute 2017-12-04 14:11 ` [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-12-07 9:47 ` Anoob 2017-12-08 14:00 ` Anoob @ 2017-12-11 14:04 ` Nelio Laranjeiro 2017-12-12 7:14 ` Anoob Joseph 2017-12-11 14:04 ` [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 3 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-11 14:04 UTC (permalink / raw) To: Sergio Gonzalez Monroy, Radu Nicolau, Anoob Joseph; +Cc: dev, akhil.goyal Generic flow API have both direction bits, ingress and egress for rules which may work on both sides. Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload") Cc: akhil.goyal@nxp.com Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> Acked-by: Radu Nicolau <radu.nicolau@intel.com> --- examples/ipsec-secgw/ipsec.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index ec8bf95e1..17bd7620d 100644 --- a/examples/ipsec-secgw/ipsec.c +++ b/examples/ipsec-secgw/ipsec.c @@ -205,6 +205,8 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa) sa->attr.egress = (sa->direction == RTE_SECURITY_IPSEC_SA_DIR_EGRESS); + sa->attr.ingress = (sa->direction == + RTE_SECURITY_IPSEC_SA_DIR_INGRESS); sa->flow = rte_flow_create(sa->portid, &sa->attr, sa->pattern, sa->action, &err); if (sa->flow == NULL) { -- 2.11.0 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 1/2] examples/ipsec-secgw: fix missing ingress flow attribute 2017-12-11 14:04 ` [dpdk-dev] [PATCH v3 1/2] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro @ 2017-12-12 7:14 ` Anoob Joseph 0 siblings, 0 replies; 56+ messages in thread From: Anoob Joseph @ 2017-12-12 7:14 UTC (permalink / raw) To: Nelio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau Cc: anoob.joseph, dev, akhil.goyal On 12/11/2017 07:34 PM, Nelio Laranjeiro wrote: > Generic flow API have both direction bits, ingress and egress for rules > which may work on both sides. > > Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload") > Cc: akhil.goyal@nxp.com > > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> > Acked-by: Radu Nicolau <radu.nicolau@intel.com> Acked-by: Anoob Joseph <anoob.joseph@caviumnetworks.com> > --- > examples/ipsec-secgw/ipsec.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c > index ec8bf95e1..17bd7620d 100644 > --- a/examples/ipsec-secgw/ipsec.c > +++ b/examples/ipsec-secgw/ipsec.c > @@ -205,6 +205,8 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa) > > sa->attr.egress = (sa->direction == > RTE_SECURITY_IPSEC_SA_DIR_EGRESS); > + sa->attr.ingress = (sa->direction == > + RTE_SECURITY_IPSEC_SA_DIR_INGRESS); > sa->flow = rte_flow_create(sa->portid, > &sa->attr, sa->pattern, sa->action, &err); > if (sa->flow == NULL) { ^ permalink raw reply [flat|nested] 56+ messages in thread
* [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-04 14:11 ` [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro ` (2 preceding siblings ...) 2017-12-11 14:04 ` [dpdk-dev] [PATCH v3 1/2] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro @ 2017-12-11 14:04 ` Nelio Laranjeiro 2017-12-12 12:43 ` Anoob Joseph ` (3 more replies) 3 siblings, 4 replies; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-11 14:04 UTC (permalink / raw) To: Sergio Gonzalez Monroy, Radu Nicolau, Anoob Joseph; +Cc: dev Mellanox INNOVA NIC needs to have final target queue actions to perform inline crypto. Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@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; + } +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; -- 2.11.0 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-11 14:04 ` [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro @ 2017-12-12 12:43 ` Anoob Joseph 2017-12-12 13:44 ` Nelio Laranjeiro 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 1/3] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro ` (2 subsequent siblings) 3 siblings, 1 reply; 56+ messages in thread From: Anoob Joseph @ 2017-12-12 12:43 UTC (permalink / raw) To: Nelio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau; +Cc: anoob.joseph, dev 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@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). If the driver doesn't need metadata and the flow create fails, then the create session should fail. Any thoughts? > + } > +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; ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-12 12:43 ` Anoob Joseph @ 2017-12-12 13:44 ` Nelio Laranjeiro 2017-12-12 14:04 ` Anoob Joseph 0 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-12 13:44 UTC (permalink / raw) To: Anoob Joseph; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev 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@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. 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? > > + } > > +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, -- Nélio Laranjeiro 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-12 13:44 ` Nelio Laranjeiro @ 2017-12-12 14:04 ` Anoob Joseph 2017-12-12 14:38 ` Nelio Laranjeiro 0 siblings, 1 reply; 56+ messages in thread From: Anoob Joseph @ 2017-12-12 14:04 UTC (permalink / raw) To: Nelio Laranjeiro; +Cc: anoob.joseph, Sergio Gonzalez Monroy, Radu Nicolau, 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 <nelio.laranjeiro@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. > 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, > ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-12 14:04 ` Anoob Joseph @ 2017-12-12 14:38 ` Nelio Laranjeiro 2017-12-13 6:41 ` Anoob Joseph 0 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-12 14:38 UTC (permalink / raw) To: Anoob Joseph; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev 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@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. > > 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. > 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, -- Nélio Laranjeiro 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-12 14:38 ` Nelio Laranjeiro @ 2017-12-13 6:41 ` Anoob Joseph 2017-12-13 10:02 ` Nelio Laranjeiro 0 siblings, 1 reply; 56+ messages in thread From: Anoob Joseph @ 2017-12-13 6:41 UTC (permalink / raw) To: Nelio Laranjeiro; +Cc: anoob.joseph, Sergio Gonzalez Monroy, Radu Nicolau, dev 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@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. > >> 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, > ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-13 6:41 ` Anoob Joseph @ 2017-12-13 10:02 ` Nelio Laranjeiro 2017-12-13 11:38 ` Anoob Joseph 0 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-13 10:02 UTC (permalink / raw) To: Anoob Joseph; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev 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@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"? > > > 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, > > > -- Nélio Laranjeiro 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-13 10:02 ` Nelio Laranjeiro @ 2017-12-13 11:38 ` Anoob Joseph 2017-12-13 12:53 ` Nelio Laranjeiro 0 siblings, 1 reply; 56+ messages in thread From: Anoob Joseph @ 2017-12-13 11:38 UTC (permalink / raw) To: Nelio Laranjeiro; +Cc: anoob.joseph, Sergio Gonzalez Monroy, Radu Nicolau, dev 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@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. 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. > >>>> 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, >>> ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-13 11:38 ` Anoob Joseph @ 2017-12-13 12:53 ` Nelio Laranjeiro 2017-12-13 13:53 ` Anoob Joseph 0 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-13 12:53 UTC (permalink / raw) To: Anoob Joseph; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev 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@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. > 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. 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 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-13 12:53 ` Nelio Laranjeiro @ 2017-12-13 13:53 ` Anoob Joseph 2017-12-13 14:47 ` Nelio Laranjeiro 0 siblings, 1 reply; 56+ messages in thread From: Anoob Joseph @ 2017-12-13 13:53 UTC (permalink / raw) To: Nelio Laranjeiro; +Cc: anoob.joseph, Sergio Gonzalez Monroy, Radu Nicolau, dev 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@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. > > 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. > >>>>>> 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, > ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-13 13:53 ` Anoob Joseph @ 2017-12-13 14:47 ` Nelio Laranjeiro 2017-12-20 16:19 ` Boris Pismenny 0 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-13 14:47 UTC (permalink / raw) To: Anoob Joseph, Sergio Gonzalez Monroy, Radu Nicolau; +Cc: dev 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@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. > > > > > > > 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-13 14:47 ` Nelio Laranjeiro @ 2017-12-20 16:19 ` Boris Pismenny 2017-12-21 8:06 ` Anoob Joseph 0 siblings, 1 reply; 56+ messages in thread From: Boris Pismenny @ 2017-12-20 16:19 UTC (permalink / raw) To: Nélio Laranjeiro, Anoob Joseph, Sergio Gonzalez Monroy, Radu Nicolau, Aviad Yehezkel, Liran Liss Cc: dev 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@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). Currently, the SPI is provided in the rte_security_session_conf. I think this is a mistake. The rte_security session is not using this field and it should be provided only via rte_flow. This field should be provided only when protocol offload is used. Moreover, according to the IPsec RFC, the SPI can be used to identify the SA in conjunction with the destination IP and the source IP. Both are provided in rte_flow. If you decide to use only the information from rte_security_session_conf to identify the SA, then you are limiting yourself. Best, Boris. ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-20 16:19 ` Boris Pismenny @ 2017-12-21 8:06 ` Anoob Joseph 2017-12-21 10:12 ` Boris Pismenny 0 siblings, 1 reply; 56+ messages in thread From: Anoob Joseph @ 2017-12-21 8:06 UTC (permalink / raw) To: Boris Pismenny, Nélio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau, Aviad Yehezkel, Liran Liss Cc: anoob.joseph, dev 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@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? Thanks, Anoob > > Currently, the SPI is provided in the rte_security_session_conf. I think this is a mistake. > The rte_security session is not using this field and it should be provided only via rte_flow. > This field should be provided only when protocol offload is used. > Moreover, according to the IPsec RFC, the SPI can be used to identify the SA in conjunction with > the destination IP and the source IP. Both are provided in rte_flow. If you decide to use only the > information from rte_security_session_conf to identify the SA, then you are limiting yourself. > > Best, > Boris. > ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-21 8:06 ` Anoob Joseph @ 2017-12-21 10:12 ` Boris Pismenny 2017-12-21 14:22 ` Adrien Mazarguil 2018-01-05 5:52 ` Anoob Joseph 0 siblings, 2 replies; 56+ messages in thread From: Boris Pismenny @ 2017-12-21 10:12 UTC (permalink / raw) To: Anoob Joseph, Nélio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau, Aviad Yehezkel, Liran Liss Cc: dev 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@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? 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! 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. ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-21 10:12 ` Boris Pismenny @ 2017-12-21 14:22 ` Adrien Mazarguil 2018-01-05 6:18 ` Anoob Joseph 2018-01-05 5:52 ` Anoob Joseph 1 sibling, 1 reply; 56+ messages in thread From: Adrien Mazarguil @ 2017-12-21 14:22 UTC (permalink / raw) To: Boris Pismenny Cc: Anoob Joseph, Nélio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau, Aviad Yehezkel, Liran Liss, dev On Thu, Dec 21, 2017 at 12:12:29PM +0200, Boris Pismenny wrote: <snip> > On 12/21/2017 10:06 AM, Anoob Joseph wrote: <snip> > > 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? > 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! > > 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. I think there's already a way to satisfy everyone regarding context requirements on TX without the huge penalty of SW parsing in case HW doesn't support matching on egress. While seldom used at the moment, rte_flow patterns can match packet meta-data (see meta pattern items); for instance RTE_FLOW_ITEM_TYPE_PORT matches a physical port of the underlying device. This works both with ingress and egress. For ingress, RTE_FLOW_ACTION_TYPE_MARK can be used to add meta data to selected packets. While for egress such an action doesn't make much sense at the moment, the converse meta pattern item (RTE_FLOW_ITEM_TYPE_MARK) could be useful to let PMD know what needs to be done with packets submitted by the application and containing a given mark. That way, PMDs that do not support egress packet matching wouldn't need to lie and would reject rules such as: flow create X egress pattern ip / udp / end actions whatever / end But would accept: flow create X egress pattern mark is 42 / end actions whatever / end PMDs could also support combinations (remember the position of meta items within a pattern is not significant) to only perform whatever for UDPv4 packets marked with 42. Non-marked packets would go through unmodified: flow create X egress pattern ip / udp / mark is 42 / end actions whatever / end This is just to point out how leveraging meta pattern items on egress is one possibility using MARK as an example. -- Adrien Mazarguil 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-21 14:22 ` Adrien Mazarguil @ 2018-01-05 6:18 ` Anoob Joseph 2018-01-09 12:48 ` Nelio Laranjeiro 0 siblings, 1 reply; 56+ messages in thread From: Anoob Joseph @ 2018-01-05 6:18 UTC (permalink / raw) To: Adrien Mazarguil, Boris Pismenny Cc: anoob.joseph, Nélio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau, Aviad Yehezkel, Liran Liss, dev, Jerin Jacob, Narayana Prasad Hi Adrien, On 12/21/2017 07:52 PM, Adrien Mazarguil wrote: > On Thu, Dec 21, 2017 at 12:12:29PM +0200, Boris Pismenny wrote: > <snip> >> On 12/21/2017 10:06 AM, Anoob Joseph wrote: > <snip> >>> 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? >> 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! >> >> 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. > I think there's already a way to satisfy everyone regarding context > requirements on TX without the huge penalty of SW parsing in case HW doesn't > support matching on egress. > > While seldom used at the moment, rte_flow patterns can match packet > meta-data (see meta pattern items); for instance RTE_FLOW_ITEM_TYPE_PORT > matches a physical port of the underlying device. This works both with > ingress and egress. > > For ingress, RTE_FLOW_ACTION_TYPE_MARK can be used to add meta data to > selected packets. While for egress such an action doesn't make much sense at > the moment, the converse meta pattern item (RTE_FLOW_ITEM_TYPE_MARK) could > be useful to let PMD know what needs to be done with packets submitted by > the application and containing a given mark. Good suggestion. For ingress this would help. The only problem with this is the size of ID. Since it is 32 bit, this particular mark cannot be used to save and retrieve application pointers. If it was 64 bit, this could've solved all the metadata problems. For egress, PMD would still need to do a lookup as, the mark ID would be 32 bit and has to be matched with the same in various flows. Though this is better than what we have right now(with flows), this may still be more costlier than "set_pkt_metadata" method. > > That way, PMDs that do not support egress packet matching wouldn't need to > lie and would reject rules such as: > > flow create X egress pattern ip / udp / end actions whatever / end > > But would accept: > > flow create X egress pattern mark is 42 / end actions whatever / end > > PMDs could also support combinations (remember the position of meta items > within a pattern is not significant) to only perform whatever for UDPv4 > packets marked with 42. Non-marked packets would go through unmodified: > > flow create X egress pattern ip / udp / mark is 42 / end actions whatever / end > > This is just to point out how leveraging meta pattern items on egress is one > possibility using MARK as an example. > ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2018-01-05 6:18 ` Anoob Joseph @ 2018-01-09 12:48 ` Nelio Laranjeiro 2018-01-10 6:21 ` Anoob Joseph 0 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2018-01-09 12:48 UTC (permalink / raw) To: Anoob Joseph Cc: Adrien Mazarguil, Boris Pismenny, Sergio Gonzalez Monroy, Radu Nicolau, Aviad Yehezkel, Liran Liss, dev, Jerin Jacob, Narayana Prasad Hi Anoob, On Fri, Jan 05, 2018 at 11:48:50AM +0530, Anoob Joseph wrote: > Hi Adrien, > > > On 12/21/2017 07:52 PM, Adrien Mazarguil wrote: > > On Thu, Dec 21, 2017 at 12:12:29PM +0200, Boris Pismenny wrote: > > <snip> > > > On 12/21/2017 10:06 AM, Anoob Joseph wrote: > > <snip> > > > > 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? > > > 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! > > > > > > 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. > > I think there's already a way to satisfy everyone regarding context > > requirements on TX without the huge penalty of SW parsing in case HW doesn't > > support matching on egress. > > > > While seldom used at the moment, rte_flow patterns can match packet > > meta-data (see meta pattern items); for instance RTE_FLOW_ITEM_TYPE_PORT > > matches a physical port of the underlying device. This works both with > > ingress and egress. > > > > For ingress, RTE_FLOW_ACTION_TYPE_MARK can be used to add meta data to > > selected packets. While for egress such an action doesn't make much sense at > > the moment, the converse meta pattern item (RTE_FLOW_ITEM_TYPE_MARK) could > > be useful to let PMD know what needs to be done with packets submitted by > > the application and containing a given mark. > Good suggestion. For ingress this would help. The only problem with this is > the size of ID. Since it is 32 bit, this particular mark cannot be used to > save and retrieve application pointers. If it was 64 bit, this could've > solved all the metadata problems. > > For egress, PMD would still need to do a lookup as, the mark ID would be 32 > bit and has to be matched with the same in various flows. Though this is > better than what we have right now(with flows), this may still be more > costlier than "set_pkt_metadata" method. It depends on how you implement it, it can be a simple table using it as an index, it can be a simple hash based on the tunnel informations, etc... set_pkt_metada() has two issues: 1. it is device specific [1] as described in the API. For an application it is unusable. 2. it is a function called per packet which is costly and don't give any benefit for an application which can directly store the pointer in the mbuf without calling such function. > > That way, PMDs that do not support egress packet matching wouldn't need to > > lie and would reject rules such as: > > > > flow create X egress pattern ip / udp / end actions whatever / end > > > > But would accept: > > > > flow create X egress pattern mark is 42 / end actions whatever / end > > > > PMDs could also support combinations (remember the position of meta items > > within a pattern is not significant) to only perform whatever for UDPv4 > > packets marked with 42. Non-marked packets would go through unmodified: > > > > flow create X egress pattern ip / udp / mark is 42 / end actions whatever / end > > > > This is just to point out how leveraging meta pattern items on egress is one > > possibility using MARK as an example. > > Regards, [1] https://dpdk.org/browse/next/dpdk-next-crypto/tree/lib/librte_security/rte_security.h#n331 -- Nélio Laranjeiro 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2018-01-09 12:48 ` Nelio Laranjeiro @ 2018-01-10 6:21 ` Anoob Joseph 0 siblings, 0 replies; 56+ messages in thread From: Anoob Joseph @ 2018-01-10 6:21 UTC (permalink / raw) To: Nelio Laranjeiro Cc: anoob.joseph, Adrien Mazarguil, Boris Pismenny, Sergio Gonzalez Monroy, Radu Nicolau, Aviad Yehezkel, Liran Liss, dev, Jerin Jacob, Narayana Prasad Hi Nelio, On 01/09/2018 06:18 PM, Nelio Laranjeiro wrote: > Hi Anoob, > > On Fri, Jan 05, 2018 at 11:48:50AM +0530, Anoob Joseph wrote: >> Hi Adrien, >> >> >> On 12/21/2017 07:52 PM, Adrien Mazarguil wrote: >>> On Thu, Dec 21, 2017 at 12:12:29PM +0200, Boris Pismenny wrote: >>> <snip> >>>> On 12/21/2017 10:06 AM, Anoob Joseph wrote: >>> <snip> >>>>> 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? >>>> 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! >>>> >>>> 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. >>> I think there's already a way to satisfy everyone regarding context >>> requirements on TX without the huge penalty of SW parsing in case HW doesn't >>> support matching on egress. >>> >>> While seldom used at the moment, rte_flow patterns can match packet >>> meta-data (see meta pattern items); for instance RTE_FLOW_ITEM_TYPE_PORT >>> matches a physical port of the underlying device. This works both with >>> ingress and egress. >>> >>> For ingress, RTE_FLOW_ACTION_TYPE_MARK can be used to add meta data to >>> selected packets. While for egress such an action doesn't make much sense at >>> the moment, the converse meta pattern item (RTE_FLOW_ITEM_TYPE_MARK) could >>> be useful to let PMD know what needs to be done with packets submitted by >>> the application and containing a given mark. >> Good suggestion. For ingress this would help. The only problem with this is >> the size of ID. Since it is 32 bit, this particular mark cannot be used to >> save and retrieve application pointers. If it was 64 bit, this could've >> solved all the metadata problems. >> >> For egress, PMD would still need to do a lookup as, the mark ID would be 32 >> bit and has to be matched with the same in various flows. Though this is >> better than what we have right now(with flows), this may still be more >> costlier than "set_pkt_metadata" method. > It depends on how you implement it, it can be a simple table using it as > an index, it can be a simple hash based on the tunnel informations, > etc... Yeah. I see your point. It can be an index that is set per packet. This index will be obtained from the driver as the flow is created, and will be saved along with SA. This can be used to uniquely associate a packet with a flow. > > set_pkt_metada() has two issues: > > 1. it is device specific [1] as described in the API. For an > application it is unusable. > > 2. it is a function called per packet which is costly and don't give > any benefit for an application which can directly store the pointer > in the mbuf without calling such function. For inline crypto mode, the only thing that set_pkt_metadata does is associate a packet with a particular security session. The above discussed model could replace that. But for inline protocol, that may not be the case. In fact I'm in the middle of drafting a patch which will enable the application to set & retrieve sequence number & IV used while protocol offload. For such cases, there will be more use cases for set_pkt_metadata than just storing a pointer. The idea is, in such cases application would need to send per packet information. In inline crypto, it wasn't necessarily per packet (setting security session pointer, which was per SA), but for inline protocol, there will be per packet variables that the application would need to communicate. For inline crypto, the "MARK" idea should be fine, but for inline protocol that may not suffice the requirement. >>> That way, PMDs that do not support egress packet matching wouldn't need to >>> lie and would reject rules such as: >>> >>> flow create X egress pattern ip / udp / end actions whatever / end >>> >>> But would accept: >>> >>> flow create X egress pattern mark is 42 / end actions whatever / end >>> >>> PMDs could also support combinations (remember the position of meta items >>> within a pattern is not significant) to only perform whatever for UDPv4 >>> packets marked with 42. Non-marked packets would go through unmodified: >>> >>> flow create X egress pattern ip / udp / mark is 42 / end actions whatever / end >>> >>> This is just to point out how leveraging meta pattern items on egress is one >>> possibility using MARK as an example. >>> > Regards, > > [1] https://dpdk.org/browse/next/dpdk-next-crypto/tree/lib/librte_security/rte_security.h#n331 > ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions 2017-12-21 10:12 ` Boris Pismenny 2017-12-21 14:22 ` Adrien Mazarguil @ 2018-01-05 5:52 ` Anoob Joseph 1 sibling, 0 replies; 56+ messages in thread From: Anoob Joseph @ 2018-01-05 5:52 UTC (permalink / raw) To: Boris Pismenny, Nélio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau, Aviad Yehezkel, Liran Liss Cc: anoob.joseph, dev, Jerin Jacob, Narayana Prasad 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@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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* [dpdk-dev] [PATCH v4 1/3] examples/ipsec-secgw: fix missing ingress flow attribute 2017-12-11 14:04 ` [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-12-12 12:43 ` Anoob Joseph @ 2017-12-14 15:14 ` Nelio Laranjeiro 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 2/3] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress " Nelio Laranjeiro 3 siblings, 0 replies; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-14 15:14 UTC (permalink / raw) To: Sergio Gonzalez Monroy, Radu Nicolau, Anoob Joseph; +Cc: dev, akhil.goyal Generic flow API have both direction bits, ingress and egress for rules which may work on both sides. Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload") Cc: akhil.goyal@nxp.com Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> Acked-by: Radu Nicolau <radu.nicolau@intel.com> Acked-by: Anoob Joseph <anoob.joseph@caviumnetworks.com> --- examples/ipsec-secgw/ipsec.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index ec8bf95e1..17bd7620d 100644 --- a/examples/ipsec-secgw/ipsec.c +++ b/examples/ipsec-secgw/ipsec.c @@ -205,6 +205,8 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa) sa->attr.egress = (sa->direction == RTE_SECURITY_IPSEC_SA_DIR_EGRESS); + sa->attr.ingress = (sa->direction == + RTE_SECURITY_IPSEC_SA_DIR_INGRESS); sa->flow = rte_flow_create(sa->portid, &sa->attr, sa->pattern, sa->action, &err); if (sa->flow == NULL) { -- 2.11.0 ^ permalink raw reply [flat|nested] 56+ messages in thread
* [dpdk-dev] [PATCH v4 2/3] examples/ipsec-secgw: add target queues in flow actions 2017-12-11 14:04 ` [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-12-12 12:43 ` Anoob Joseph 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 1/3] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro @ 2017-12-14 15:14 ` Nelio Laranjeiro 2017-12-18 8:23 ` Anoob Joseph 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress " Nelio Laranjeiro 3 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-14 15:14 UTC (permalink / raw) To: Sergio Gonzalez Monroy, Radu Nicolau, Anoob Joseph; +Cc: dev Mellanox INNOVA NIC needs to have final target queue actions to perform inline crypto. Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> --- Changes in v4: * remove Egress code. 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 | 53 ++++++++++++++++++++++++++++++++++++++++++-- examples/ipsec-secgw/ipsec.h | 2 +- 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index 17bd7620d..8e8dc6df7 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,63 @@ 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; + } +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; -- 2.11.0 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v4 2/3] examples/ipsec-secgw: add target queues in flow actions 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 2/3] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro @ 2017-12-18 8:23 ` Anoob Joseph 2017-12-18 9:57 ` Nélio Laranjeiro 0 siblings, 1 reply; 56+ messages in thread From: Anoob Joseph @ 2017-12-18 8:23 UTC (permalink / raw) To: Nelio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau; +Cc: anoob.joseph, dev Hi Nelio, On 12/14/2017 08:44 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@6wind.com> > > --- > > Changes in v4: > > * remove Egress code. > > 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 | 53 ++++++++++++++++++++++++++++++++++++++++++-- > examples/ipsec-secgw/ipsec.h | 2 +- > 2 files changed, 52 insertions(+), 3 deletions(-) > > diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c > index 17bd7620d..8e8dc6df7 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,63 @@ 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; This is reverted in the second patch, right? Are you planning to revise the patch fixing this? I'm fine with the changes, otherwise. > - > 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; > + } > +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; ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v4 2/3] examples/ipsec-secgw: add target queues in flow actions 2017-12-18 8:23 ` Anoob Joseph @ 2017-12-18 9:57 ` Nélio Laranjeiro 0 siblings, 0 replies; 56+ messages in thread From: Nélio Laranjeiro @ 2017-12-18 9:57 UTC (permalink / raw) To: Anoob Joseph; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev Hi Anoob, On Mon, Dec 18, 2017 at 01:53:50PM +0530, Anoob Joseph wrote: > Hi Nelio, > > > On 12/14/2017 08:44 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@6wind.com> > > > > --- > > > > Changes in v4: > > > > * remove Egress code. > > > > 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 | 53 ++++++++++++++++++++++++++++++++++++++++++-- > > examples/ipsec-secgw/ipsec.h | 2 +- > > 2 files changed, 52 insertions(+), 3 deletions(-) > > > > diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c > > index 17bd7620d..8e8dc6df7 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,63 @@ 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; > This is reverted in the second patch, right? Are you planning to revise the > patch fixing this? I'm fine with the changes, otherwise. <snip> Yes I will send a v5 fixing this point. Thanks, -- Nélio Laranjeiro 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress flow actions 2017-12-11 14:04 ` [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro ` (2 preceding siblings ...) 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 2/3] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro @ 2017-12-14 15:14 ` Nelio Laranjeiro 2017-12-15 9:05 ` Anoob Joseph ` (3 more replies) 3 siblings, 4 replies; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-14 15:14 UTC (permalink / raw) To: Sergio Gonzalez Monroy, Radu Nicolau, Anoob Joseph; +Cc: dev 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)) { + 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, -- 2.11.0 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress flow actions 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress " Nelio Laranjeiro @ 2017-12-15 9:05 ` Anoob Joseph 2017-12-15 13:53 ` Nelio Laranjeiro 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 1/3] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro ` (2 subsequent siblings) 3 siblings, 1 reply; 56+ messages in thread From: Anoob Joseph @ 2017-12-15 9:05 UTC (permalink / raw) To: Nelio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau; +Cc: anoob.joseph, dev 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. And if the flow create fails in that case, then create_session would fail. 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! > + 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, ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress flow actions 2017-12-15 9:05 ` Anoob Joseph @ 2017-12-15 13:53 ` Nelio Laranjeiro 2017-12-15 15:39 ` Anoob Joseph 0 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-15 13:53 UTC (permalink / raw) To: Anoob Joseph; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev 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 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? > > + 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, -- Nélio Laranjeiro 6WIND ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress flow actions 2017-12-15 13:53 ` Nelio Laranjeiro @ 2017-12-15 15:39 ` Anoob Joseph 2017-12-15 16:53 ` Nelio Laranjeiro 0 siblings, 1 reply; 56+ messages in thread From: Anoob Joseph @ 2017-12-15 15:39 UTC (permalink / raw) To: Nelio Laranjeiro; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev 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. > >> 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, 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; 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; } > >>> + 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, > ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress flow actions 2017-12-15 15:39 ` Anoob Joseph @ 2017-12-15 16:53 ` Nelio Laranjeiro 2017-12-15 17:01 ` Anoob Joseph 0 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-15 16:53 UTC (permalink / raw) To: Anoob Joseph; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress flow actions 2017-12-15 16:53 ` Nelio Laranjeiro @ 2017-12-15 17:01 ` Anoob Joseph 0 siblings, 0 replies; 56+ messages in thread From: Anoob Joseph @ 2017-12-15 17:01 UTC (permalink / raw) To: Nelio Laranjeiro; +Cc: Sergio Gonzalez Monroy, Radu Nicolau, dev Hi Nelio, Agreed. Those can be addressed in other patches. I just wanted your opinion on how the present patch can be finalized keeping in mind the situations that we should address. And yeah, with the change in commit log, this should be enough. Thanks, Anoob On 15-12-2017 22:23, Nelio Laranjeiro wrote: > 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 > ^ permalink raw reply [flat|nested] 56+ messages in thread
* [dpdk-dev] [PATCH v5 1/3] examples/ipsec-secgw: fix missing ingress flow attribute 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress " Nelio Laranjeiro 2017-12-15 9:05 ` Anoob Joseph @ 2017-12-18 10:24 ` Nelio Laranjeiro 2018-01-18 14:50 ` De Lara Guarch, Pablo 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 2/3] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 3/3] examples/ipsec-secgw: add Egress " Nelio Laranjeiro 3 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-18 10:24 UTC (permalink / raw) To: Sergio Gonzalez Monroy, Radu Nicolau, Anoob Joseph; +Cc: dev, akhil.goyal Generic flow API have both direction bits, ingress and egress for rules which may work on both sides. Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload") Cc: akhil.goyal@nxp.com Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> Acked-by: Radu Nicolau <radu.nicolau@intel.com> Acked-by: Anoob Joseph <anoob.joseph@caviumnetworks.com> --- examples/ipsec-secgw/ipsec.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index ec8bf95e1..17bd7620d 100644 --- a/examples/ipsec-secgw/ipsec.c +++ b/examples/ipsec-secgw/ipsec.c @@ -205,6 +205,8 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa) sa->attr.egress = (sa->direction == RTE_SECURITY_IPSEC_SA_DIR_EGRESS); + sa->attr.ingress = (sa->direction == + RTE_SECURITY_IPSEC_SA_DIR_INGRESS); sa->flow = rte_flow_create(sa->portid, &sa->attr, sa->pattern, sa->action, &err); if (sa->flow == NULL) { -- 2.11.0 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v5 1/3] examples/ipsec-secgw: fix missing ingress flow attribute 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 1/3] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro @ 2018-01-18 14:50 ` De Lara Guarch, Pablo 0 siblings, 0 replies; 56+ messages in thread From: De Lara Guarch, Pablo @ 2018-01-18 14:50 UTC (permalink / raw) To: Nelio Laranjeiro, Gonzalez Monroy, Sergio, Nicolau, Radu, Anoob Joseph Cc: dev, akhil.goyal > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Nelio Laranjeiro > Sent: Monday, December 18, 2017 10:25 AM > To: Gonzalez Monroy, Sergio <sergio.gonzalez.monroy@intel.com>; > Nicolau, Radu <radu.nicolau@intel.com>; Anoob Joseph > <anoob.joseph@caviumnetworks.com> > Cc: dev@dpdk.org; akhil.goyal@nxp.com > Subject: [dpdk-dev] [PATCH v5 1/3] examples/ipsec-secgw: fix missing > ingress flow attribute > > Generic flow API have both direction bits, ingress and egress for rules which > may work on both sides. > > Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload") > Cc: akhil.goyal@nxp.com > > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> > Acked-by: Radu Nicolau <radu.nicolau@intel.com> > Acked-by: Anoob Joseph <anoob.joseph@caviumnetworks.com> Series applied to dpdk-next-crypto. Thanks, Pablo ^ permalink raw reply [flat|nested] 56+ messages in thread
* [dpdk-dev] [PATCH v5 2/3] examples/ipsec-secgw: add target queues in flow actions 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress " Nelio Laranjeiro 2017-12-15 9:05 ` Anoob Joseph 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 1/3] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro @ 2017-12-18 10:24 ` Nelio Laranjeiro 2017-12-19 6:22 ` Anoob Joseph 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 3/3] examples/ipsec-secgw: add Egress " Nelio Laranjeiro 3 siblings, 1 reply; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-18 10:24 UTC (permalink / raw) To: Sergio Gonzalez Monroy, Radu Nicolau, Anoob Joseph; +Cc: dev Mellanox INNOVA NIC needs to have final target queue actions to perform inline crypto. Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> --- Changes in v5: * Add back default second action. Changes in v4: * remove Egress code. 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 | 51 ++++++++++++++++++++++++++++++++++++++++++++ examples/ipsec-secgw/ipsec.h | 2 +- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index 17bd7620d..37a6416ed 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); @@ -207,9 +208,59 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa) 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; + } +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; -- 2.11.0 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v5 2/3] examples/ipsec-secgw: add target queues in flow actions 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 2/3] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro @ 2017-12-19 6:22 ` Anoob Joseph 0 siblings, 0 replies; 56+ messages in thread From: Anoob Joseph @ 2017-12-19 6:22 UTC (permalink / raw) To: Nelio Laranjeiro, Sergio Gonzalez Monroy, Radu Nicolau; +Cc: anoob.joseph, dev On 12/18/2017 03:54 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@6wind.com> Acked-by: Anoob Joseph <anoob.joseph@caviumnetworks.com> > > --- > > Changes in v5: > > * Add back default second action. > > Changes in v4: > > * remove Egress code. > > 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 | 51 ++++++++++++++++++++++++++++++++++++++++++++ > examples/ipsec-secgw/ipsec.h | 2 +- > 2 files changed, 52 insertions(+), 1 deletion(-) > > diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c > index 17bd7620d..37a6416ed 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); > @@ -207,9 +208,59 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa) > 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; > + } > +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; ^ permalink raw reply [flat|nested] 56+ messages in thread
* [dpdk-dev] [PATCH v5 3/3] examples/ipsec-secgw: add Egress flow actions 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress " Nelio Laranjeiro ` (2 preceding siblings ...) 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 2/3] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro @ 2017-12-18 10:24 ` Nelio Laranjeiro 2018-01-08 16:13 ` De Lara Guarch, Pablo 2018-01-16 16:12 ` Nicolau, Radu 3 siblings, 2 replies; 56+ messages in thread From: Nelio Laranjeiro @ 2017-12-18 10:24 UTC (permalink / raw) To: Sergio Gonzalez Monroy, Radu Nicolau, Anoob Joseph; +Cc: dev Add Egress flow create for devices supporting RTE_SECURITY_TX_HW_TRAILER_OFFLOAD. Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> -- Changes in v5: * removed default second end actions wrongly added in this patch. --- examples/ipsec-secgw/ipsec.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c index 37a6416ed..580e09a3a 100644 --- a/examples/ipsec-secgw/ipsec.c +++ b/examples/ipsec-secgw/ipsec.c @@ -255,6 +255,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)) { + 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, -- 2.11.0 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v5 3/3] examples/ipsec-secgw: add Egress flow actions 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 3/3] examples/ipsec-secgw: add Egress " Nelio Laranjeiro @ 2018-01-08 16:13 ` De Lara Guarch, Pablo 2018-01-16 16:12 ` Nicolau, Radu 1 sibling, 0 replies; 56+ messages in thread From: De Lara Guarch, Pablo @ 2018-01-08 16:13 UTC (permalink / raw) To: Nelio Laranjeiro, Gonzalez Monroy, Sergio, Nicolau, Radu, Anoob Joseph Cc: dev > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Nelio Laranjeiro > Sent: Monday, December 18, 2017 10:25 AM > To: Gonzalez Monroy, Sergio <sergio.gonzalez.monroy@intel.com>; > Nicolau, Radu <radu.nicolau@intel.com>; Anoob Joseph > <anoob.joseph@caviumnetworks.com> > Cc: dev@dpdk.org > Subject: [dpdk-dev] [PATCH v5 3/3] examples/ipsec-secgw: add Egress flow > actions > > Add Egress flow create for devices supporting > RTE_SECURITY_TX_HW_TRAILER_OFFLOAD. > > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> Akhil, Radu, could you review this patch? Thanks, Pablo ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [dpdk-dev] [PATCH v5 3/3] examples/ipsec-secgw: add Egress flow actions 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 3/3] examples/ipsec-secgw: add Egress " Nelio Laranjeiro 2018-01-08 16:13 ` De Lara Guarch, Pablo @ 2018-01-16 16:12 ` Nicolau, Radu 1 sibling, 0 replies; 56+ messages in thread From: Nicolau, Radu @ 2018-01-16 16:12 UTC (permalink / raw) To: Nelio Laranjeiro, Gonzalez Monroy, Sergio, Anoob Joseph; +Cc: dev > -----Original Message----- > From: Nelio Laranjeiro [mailto:nelio.laranjeiro@6wind.com] > Sent: Monday, December 18, 2017 10:25 AM > To: Gonzalez Monroy, Sergio <sergio.gonzalez.monroy@intel.com>; Nicolau, > Radu <radu.nicolau@intel.com>; Anoob Joseph > <anoob.joseph@caviumnetworks.com> > Cc: dev@dpdk.org > Subject: [PATCH v5 3/3] examples/ipsec-secgw: add Egress flow actions > > Add Egress flow create for devices supporting > RTE_SECURITY_TX_HW_TRAILER_OFFLOAD. > > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com> > Acked-by: Radu Nicolau <radu.nicolau@intel.com> ^ permalink raw reply [flat|nested] 56+ messages in thread
end of thread, other threads:[~2018-01-18 14:50 UTC | newest] Thread overview: 56+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-23 15:12 [dpdk-dev] [PATCH 1/2] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro 2017-11-23 15:12 ` [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-11-29 12:30 ` Anoob 2017-11-29 12:50 ` Nelio Laranjeiro 2017-11-30 10:46 ` Anoob 2017-11-30 12:28 ` Nelio Laranjeiro 2017-12-01 15:04 ` Anoob Joseph 2017-12-01 16:26 ` Nelio Laranjeiro 2017-12-04 14:11 ` [dpdk-dev] [PATCH v2 1/2] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro 2017-12-11 11:50 ` Radu Nicolau 2017-12-04 14:11 ` [dpdk-dev] [PATCH v2 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-12-07 9:47 ` Anoob 2017-12-07 12:22 ` Nelio Laranjeiro 2017-12-08 14:00 ` Anoob 2017-12-08 14:40 ` Nelio Laranjeiro 2017-12-08 16:40 ` Anoob Joseph 2017-12-11 8:21 ` Nelio Laranjeiro 2017-12-11 9:00 ` Anoob 2017-12-11 14:04 ` [dpdk-dev] [PATCH v3 1/2] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro 2017-12-12 7:14 ` Anoob Joseph 2017-12-11 14:04 ` [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-12-12 12:43 ` Anoob Joseph 2017-12-12 13:44 ` Nelio Laranjeiro 2017-12-12 14:04 ` Anoob Joseph 2017-12-12 14:38 ` Nelio Laranjeiro 2017-12-13 6:41 ` Anoob Joseph 2017-12-13 10:02 ` Nelio Laranjeiro 2017-12-13 11:38 ` Anoob Joseph 2017-12-13 12:53 ` Nelio Laranjeiro 2017-12-13 13:53 ` Anoob Joseph 2017-12-13 14:47 ` Nelio Laranjeiro 2017-12-20 16:19 ` Boris Pismenny 2017-12-21 8:06 ` Anoob Joseph 2017-12-21 10:12 ` Boris Pismenny 2017-12-21 14:22 ` Adrien Mazarguil 2018-01-05 6:18 ` Anoob Joseph 2018-01-09 12:48 ` Nelio Laranjeiro 2018-01-10 6:21 ` Anoob Joseph 2018-01-05 5:52 ` Anoob Joseph 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 1/3] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 2/3] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-12-18 8:23 ` Anoob Joseph 2017-12-18 9:57 ` Nélio Laranjeiro 2017-12-14 15:14 ` [dpdk-dev] [PATCH v4 3/3] examples/ipsec-secgw: add Egress " Nelio Laranjeiro 2017-12-15 9:05 ` Anoob Joseph 2017-12-15 13:53 ` Nelio Laranjeiro 2017-12-15 15:39 ` Anoob Joseph 2017-12-15 16:53 ` Nelio Laranjeiro 2017-12-15 17:01 ` Anoob Joseph 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 1/3] examples/ipsec-secgw: fix missing ingress flow attribute Nelio Laranjeiro 2018-01-18 14:50 ` De Lara Guarch, Pablo 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 2/3] examples/ipsec-secgw: add target queues in flow actions Nelio Laranjeiro 2017-12-19 6:22 ` Anoob Joseph 2017-12-18 10:24 ` [dpdk-dev] [PATCH v5 3/3] examples/ipsec-secgw: add Egress " Nelio Laranjeiro 2018-01-08 16:13 ` De Lara Guarch, Pablo 2018-01-16 16:12 ` Nicolau, Radu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).