DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] app/testpmd: register metadata dynfield on modify field
@ 2022-03-01 11:51 Dariusz Sosnowski
  2022-03-03 12:20 ` Ferruh Yigit
  0 siblings, 1 reply; 4+ messages in thread
From: Dariusz Sosnowski @ 2022-03-01 11:51 UTC (permalink / raw)
  To: Ori Kam, Xiaoyun Li, Aman Singh, Yuying Zhang; +Cc: dev, Viacheslav Ovsiienko

This patch adds implicit registration of metadata dynamic field and flag
whenever a modify_field action with META as source and/or destination
field is used.

Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
Reviewed-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 4f7a9f17f9..dd38a635b0 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -8347,6 +8347,7 @@ parse_vc_modify_field_id(struct context *ctx, const struct token *token,
 {
 	struct rte_flow_action_modify_field *action_modify_field;
 	unsigned int i;
+	int ret;
 
 	(void)token;
 	(void)buf;
@@ -8362,9 +8363,15 @@ parse_vc_modify_field_id(struct context *ctx, const struct token *token,
 	if (!ctx->object)
 		return len;
 	action_modify_field = ctx->object;
-	if (ctx->curr == ACTION_MODIFY_FIELD_DST_TYPE_VALUE)
+	if (ctx->curr == ACTION_MODIFY_FIELD_DST_TYPE_VALUE) {
 		action_modify_field->dst.field = (enum rte_flow_field_id)i;
-	else
+		if (action_modify_field->dst.field == RTE_FLOW_FIELD_META) {
+			ret = rte_flow_dynf_metadata_register();
+			if (ret < 0)
+				return -1;
+		}
+
+	} else
 		action_modify_field->src.field = (enum rte_flow_field_id)i;
 	return len;
 }
-- 
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] app/testpmd: register metadata dynfield on modify field
  2022-03-01 11:51 [PATCH] app/testpmd: register metadata dynfield on modify field Dariusz Sosnowski
@ 2022-03-03 12:20 ` Ferruh Yigit
  2022-03-09 11:50   ` Dariusz Sosnowski
  0 siblings, 1 reply; 4+ messages in thread
From: Ferruh Yigit @ 2022-03-03 12:20 UTC (permalink / raw)
  To: Dariusz Sosnowski, Ori Kam, Xiaoyun Li, Aman Singh, Yuying Zhang
  Cc: dev, Viacheslav Ovsiienko

On 3/1/2022 11:51 AM, Dariusz Sosnowski wrote:
> This patch adds implicit registration of metadata dynamic field and flag

Hi Dariusz,

metaday dynamic field is explicitly registered when testpmd
command used to enable tx metadata, or rte flow rule created
with "set_meta" action.

Can you please document more when this implicit enablement
is required? And why that case doesn't cover above explicit
enable cases?

> whenever a modify_field action with META as source and/or destination
> field is used.
> 

According below code it is only registered in the DST_TYPE block,
not is 'else' (which seems src) leg, is this OK?

> Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
> Reviewed-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
>   app/test-pmd/cmdline_flow.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 4f7a9f17f9..dd38a635b0 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -8347,6 +8347,7 @@ parse_vc_modify_field_id(struct context *ctx, const struct token *token,
>   {
>   	struct rte_flow_action_modify_field *action_modify_field;
>   	unsigned int i;
> +	int ret;
>   
>   	(void)token;
>   	(void)buf;
> @@ -8362,9 +8363,15 @@ parse_vc_modify_field_id(struct context *ctx, const struct token *token,
>   	if (!ctx->object)
>   		return len;
>   	action_modify_field = ctx->object;
> -	if (ctx->curr == ACTION_MODIFY_FIELD_DST_TYPE_VALUE)
> +	if (ctx->curr == ACTION_MODIFY_FIELD_DST_TYPE_VALUE) {
>   		action_modify_field->dst.field = (enum rte_flow_field_id)i;
> -	else
> +		if (action_modify_field->dst.field == RTE_FLOW_FIELD_META) {
> +			ret = rte_flow_dynf_metadata_register();
> +			if (ret < 0)
> +				return -1;
> +		}
> +
> +	} else
>   		action_modify_field->src.field = (enum rte_flow_field_id)i;
>   	return len;
>   }


^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] app/testpmd: register metadata dynfield on modify field
  2022-03-03 12:20 ` Ferruh Yigit
@ 2022-03-09 11:50   ` Dariusz Sosnowski
  2022-03-14 20:42     ` Thomas Monjalon
  0 siblings, 1 reply; 4+ messages in thread
From: Dariusz Sosnowski @ 2022-03-09 11:50 UTC (permalink / raw)
  To: Ferruh Yigit, Ori Kam, Xiaoyun Li, Aman Singh, Yuying Zhang
  Cc: dev, Slava Ovsiienko

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, March 3, 2022 13:21
> To: Dariusz Sosnowski <dsosnowski@nvidia.com>; Ori Kam
> <orika@nvidia.com>; Xiaoyun Li <xiaoyun.li@intel.com>; Aman Singh
> <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>
> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>
> Subject: Re: [PATCH] app/testpmd: register metadata dynfield on modify
> field
> 
> External email: Use caution opening links or attachments
> 
> 
> On 3/1/2022 11:51 AM, Dariusz Sosnowski wrote:
> > This patch adds implicit registration of metadata dynamic field and
> > flag
> 
> Hi Dariusz,
> 
> metaday dynamic field is explicitly registered when testpmd command used
> to enable tx metadata, or rte flow rule created with "set_meta" action.
> 
> Can you please document more when this implicit enablement is required?
> And why that case doesn't cover above explicit enable cases?

Before this patch, when a user inserted a flow rule with MODIFY_FIELD action,
which modified packet metadata, the metadata dynamic field was not registered, as opposed to
what happened with SET_META action. Goal of this patch is to make the behavior consistent
between these two actions.

Maybe using "implicit" in the commit message was misleading here. 
What do you think about rewording the commit message to something like the one below?

"This patch adds registration of metadata dynamic field and flag
whenever a MODIFY_FIELD action with META as source and/or destination
field is used. It makes the behavior consistent with SET_META action, where
metadata dynamic field and flag is registered on flow rule creation." 

> > whenever a modify_field action with META as source and/or destination
> > field is used.
> >
> 
> According below code it is only registered in the DST_TYPE block, not is 'else'
> (which seems src) leg, is this OK?
> 
> > Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
> > Reviewed-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> > ---
> >   app/test-pmd/cmdline_flow.c | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > index 4f7a9f17f9..dd38a635b0 100644
> > --- a/app/test-pmd/cmdline_flow.c
> > +++ b/app/test-pmd/cmdline_flow.c
> > @@ -8347,6 +8347,7 @@ parse_vc_modify_field_id(struct context *ctx,
> const struct token *token,
> >   {
> >       struct rte_flow_action_modify_field *action_modify_field;
> >       unsigned int i;
> > +     int ret;
> >
> >       (void)token;
> >       (void)buf;
> > @@ -8362,9 +8363,15 @@ parse_vc_modify_field_id(struct context *ctx,
> const struct token *token,
> >       if (!ctx->object)
> >               return len;
> >       action_modify_field = ctx->object;
> > -     if (ctx->curr == ACTION_MODIFY_FIELD_DST_TYPE_VALUE)
> > +     if (ctx->curr == ACTION_MODIFY_FIELD_DST_TYPE_VALUE) {
> >               action_modify_field->dst.field = (enum rte_flow_field_id)i;
> > -     else
> > +             if (action_modify_field->dst.field == RTE_FLOW_FIELD_META) {
> > +                     ret = rte_flow_dynf_metadata_register();
> > +                     if (ret < 0)
> > +                             return -1;
> > +             }
> > +
> > +     } else
> >               action_modify_field->src.field = (enum rte_flow_field_id)i;
> >       return len;
> >   }

No, I should add registering for source field as well.

Best regards,
Dariusz Sosnowski

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] app/testpmd: register metadata dynfield on modify field
  2022-03-09 11:50   ` Dariusz Sosnowski
@ 2022-03-14 20:42     ` Thomas Monjalon
  0 siblings, 0 replies; 4+ messages in thread
From: Thomas Monjalon @ 2022-03-14 20:42 UTC (permalink / raw)
  To: Dariusz Sosnowski
  Cc: Ferruh Yigit, Ori Kam, Xiaoyun Li, Aman Singh, Yuying Zhang, dev,
	Slava Ovsiienko

09/03/2022 12:50, Dariusz Sosnowski:
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > On 3/1/2022 11:51 AM, Dariusz Sosnowski wrote:
> > > This patch adds implicit registration of metadata dynamic field and
> > > flag
> > 
> > Hi Dariusz,
> > 
> > metaday dynamic field is explicitly registered when testpmd command used
> > to enable tx metadata, or rte flow rule created with "set_meta" action.
> > 
> > Can you please document more when this implicit enablement is required?
> > And why that case doesn't cover above explicit enable cases?
> 
> Before this patch, when a user inserted a flow rule with MODIFY_FIELD action,
> which modified packet metadata, the metadata dynamic field was not registered, as opposed to
> what happened with SET_META action. Goal of this patch is to make the behavior consistent
> between these two actions.

I think consistency should be ensured by the PMD.
Why not registering the field in the PMD?




^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-03-14 20:42 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-01 11:51 [PATCH] app/testpmd: register metadata dynfield on modify field Dariusz Sosnowski
2022-03-03 12:20 ` Ferruh Yigit
2022-03-09 11:50   ` Dariusz Sosnowski
2022-03-14 20:42     ` Thomas Monjalon

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).