[PATCH v1 0/2] Testpmd flow update/destroy fixes

[PATCH v1 0/2] Testpmd flow update/destroy fixes

[Danylo Vodopianov]

These patches provide next fixes:
	1. The testpmd command “flow update…“ provides a nullptr as the
	   context variable.
	2. Avoid removal of additional flows after requested number of flows has
	   been already removed.

Danylo Vodopianov (2):
  app/testpmd: fix flow update
  app/testpmd: fix flow destroy

 app/test-pmd/config.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

-- 
2.43.5

[PATCH v1 1/2] app/testpmd: fix flow update

[Danylo Vodopianov]

The testpmd command “flow update…“ provides a nullptr as the
context variable.
Thus "flow aged <port_id> destroy" command can not
execute successfully.

Fix was done with next steps
1. Generate new port flow entry to add/replace action(s).
2. Set age contest if exists.
3. Replace flow in the flow list.

Fixes: 2d9c7e56e52c ("app/testpmd: support updating flow rule actions")

Signed-off-by: Danylo Vodopianov <dvo-plv@napatech.com>
---
 app/test-pmd/config.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 88770b4dfc..bf50f6adef 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3882,7 +3882,8 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
 		 const struct rte_flow_action *actions, bool is_user_id)
 {
 	struct rte_port *port;
-	struct port_flow **flow_list;
+	struct port_flow **flow_list, *uf;
+	struct rte_flow_action_age *age = age_action_get(actions);
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
 	    port_id == (portid_t)RTE_PORT_ALL)
@@ -3897,6 +3898,16 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
 			flow_list = &flow->next;
 			continue;
 		}
+
+		/* Update flow action(s) with new action(s) */
+		uf = port_flow_new(flow->rule.attr_ro, flow->rule.pattern_ro, actions, &error);
+		if (!uf)
+			return port_flow_complain(&error);
+		if (age) {
+			flow->age_type = ACTION_AGE_CONTEXT_TYPE_FLOW;
+			age->context = &flow->age_type;
+		}
+
 		/*
 		 * Poisoning to make sure PMDs update it in case
 		 * of error.
@@ -3913,6 +3924,13 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
 			printf("Flow rule #%"PRIu64
 			       " updated with new actions\n",
 			       flow->id);
+
+		uf->next = flow->next;
+		uf->id = flow->id;
+		uf->flow = flow->flow;
+		*flow_list = uf;
+
+		free(flow);
 		return 0;
 	}
 	printf("Failed to find flow %"PRIu32"\n", rule_id);
-- 
2.43.5

RE: [PATCH v1 1/2] app/testpmd: fix flow update

[Dariusz Sosnowski]

Hi, 

> -----Original Message-----
> From: Danylo Vodopianov <dvo-plv@napatech.com>
> Sent: Thursday, October 31, 2024 16:00
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> aman.deep.singh@intel.com; yuying.zhang@intel.com; Ori Kam
> <orika@nvidia.com>; mko-plv@napatech.com; ckm@napatech.com; sil-
> plv@napatech.com
> Cc: dev@dpdk.org; ferruh.yigit@amd.com
> Subject: [PATCH v1 1/2] app/testpmd: fix flow update
> 
> The testpmd command “flow update…“ provides a nullptr as the context variable.

I would propose the following phrasing, which I think would be more accurate:

	If actions provided to “flow update…“ command contained an age action,
	Then testpmd did not update the age action context accordingly.

> Thus "flow aged <port_id> destroy" command can not execute successfully.
> 
> Fix was done with next steps
> 1. Generate new port flow entry to add/replace action(s).
> 2. Set age contest if exists.

s/contest if exists/context if age action is present/

> 3. Replace flow in the flow list.
> 
> Fixes: 2d9c7e56e52c ("app/testpmd: support updating flow rule actions")
> 
> Signed-off-by: Danylo Vodopianov <dvo-plv@napatech.com>
> ---
>  app/test-pmd/config.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 88770b4dfc..bf50f6adef 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -3882,7 +3882,8 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
>                  const struct rte_flow_action *actions, bool is_user_id)  {
>         struct rte_port *port;
> -       struct port_flow **flow_list;
> +       struct port_flow **flow_list, *uf;
> +       struct rte_flow_action_age *age = age_action_get(actions);
> 
>         if (port_id_is_invalid(port_id, ENABLED_WARN) ||
>             port_id == (portid_t)RTE_PORT_ALL) @@ -3897,6 +3898,16 @@
> port_flow_update(portid_t port_id, uint32_t rule_id,
>                         flow_list = &flow->next;
>                         continue;
>                 }
> +
> +               /* Update flow action(s) with new action(s) */
> +               uf = port_flow_new(flow->rule.attr_ro, flow->rule.pattern_ro, actions,
> &error);
> +               if (!uf)
> +                       return port_flow_complain(&error);
> +               if (age) {
> +                       flow->age_type = ACTION_AGE_CONTEXT_TYPE_FLOW;
> +                       age->context = &flow->age_type;
> +               }
> +
>                 /*
>                  * Poisoning to make sure PMDs update it in case
>                  * of error.
> @@ -3913,6 +3924,13 @@ port_flow_update(portid_t port_id, uint32_t
> rule_id,
>                         printf("Flow rule #%"PRIu64
>                                " updated with new actions\n",
>                                flow->id);
> +
> +               uf->next = flow->next;
> +               uf->id = flow->id;

Please copy user_id as well.

> +               uf->flow = flow->flow;
> +               *flow_list = uf;
> +
> +               free(flow);
>                 return 0;
>         }
>         printf("Failed to find flow %"PRIu32"\n", rule_id);
> --
> 2.43.5

Best regards,
Dariusz Sosnowski

[PATCH v1 2/2] app/testpmd: fix flow destroy

[Danylo Vodopianov]

Avoid removal of additional flows after requested number of flows has
been already removed.

Issue with removal of multiple flows is internal testpmd bug at
port_flow_destroy(). This function goes through all flows and compares
given flow ‘id’ with them. However in some cases it can advance pointer
with “given ID” and thus remove additional flow.

Fixes: de956d5ecf08 ("app/testpmd: support age shared action context")

Signed-off-by: Danylo Vodopianov <dvo-plv@napatech.com>
---
 app/test-pmd/config.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index bf50f6adef..50c4b018c1 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -4170,8 +4170,12 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
 			       ctx.pf->rule.attr->ingress ? 'i' : '-',
 			       ctx.pf->rule.attr->egress ? 'e' : '-',
 			       ctx.pf->rule.attr->transfer ? 't' : '-');
+			/* use local copy of id as ctx.pf is freed by
+			 * port_flow_destroy() during processing
+			 */
+			uint64_t flow_id = ctx.pf->id;
 			if (destroy && !port_flow_destroy(port_id, 1,
-							  &ctx.pf->id, false))
+							  &flow_id, false))
 				total++;
 			break;
 		case ACTION_AGE_CONTEXT_TYPE_INDIRECT_ACTION:
-- 
2.43.5

RE: [PATCH v1 2/2] app/testpmd: fix flow destroy

[Dariusz Sosnowski]

Hi,

> -----Original Message-----
> From: Danylo Vodopianov <dvo-plv@napatech.com>
> Sent: Thursday, October 31, 2024 16:00
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> aman.deep.singh@intel.com; yuying.zhang@intel.com; Ori Kam
> <orika@nvidia.com>; mko-plv@napatech.com; ckm@napatech.com; sil-
> plv@napatech.com
> Cc: dev@dpdk.org; ferruh.yigit@amd.com
> Subject: [PATCH v1 2/2] app/testpmd: fix flow destroy

I think it would be better to rename the commit to: "app/testpmd: fix aged flow destroy"

> 
> Avoid removal of additional flows after requested number of flows has been
> already removed.
> 
> Issue with removal of multiple flows is internal testpmd bug at
> port_flow_destroy(). This function goes through all flows and compares given
> flow ‘id’ with them. However in some cases it can advance pointer with “given ID”
> and thus remove additional flow.

I'm not sure that the issue with port_flow_destroy() is really a bug.
port_flow_destroy() function never assumed that rule array can be freed when it's executing,
and port_flow_aged() just violated that assumption.

Could you please rephrase the commit message to include that info?

> 
> Fixes: de956d5ecf08 ("app/testpmd: support age shared action context")

This fix will have to be taken into LTS releases. Please add "Cc: stable@dpdk.org"

> 
> Signed-off-by: Danylo Vodopianov <dvo-plv@napatech.com>
> ---
>  app/test-pmd/config.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> bf50f6adef..50c4b018c1 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -4170,8 +4170,12 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
>                                ctx.pf->rule.attr->ingress ? 'i' : '-',
>                                ctx.pf->rule.attr->egress ? 'e' : '-',
>                                ctx.pf->rule.attr->transfer ? 't' : '-');
> +                       /* use local copy of id as ctx.pf is freed by
> +                        * port_flow_destroy() during processing
> +                        */

After the commit message is rephrased, I don't think this comment will be needed.

> +                       uint64_t flow_id = ctx.pf->id;

Please move the flow_id variable declaration to the beginning of the case.
Also, please enclose the case's body in braces, so that flow_id declaration does not leak to the following cases.

>                         if (destroy && !port_flow_destroy(port_id, 1,
> -                                                         &ctx.pf->id, false))
> +                                                         &flow_id,
> + false))
>                                 total++;
>                         break;
>                 case ACTION_AGE_CONTEXT_TYPE_INDIRECT_ACTION:
> --
> 2.43.5

Best regards,
Dariusz Sosnowski

Re: [PATCH v1 0/2] Testpmd flow update/destroy fixes

[Ferruh Yigit]

On 10/31/2024 3:00 PM, Danylo Vodopianov wrote:
> These patches provide next fixes:
> 	1. The testpmd command “flow update…“ provides a nullptr as the
> 	   context variable.
> 	2. Avoid removal of additional flows after requested number of flows has
> 	   been already removed.
> 
> Danylo Vodopianov (2):
>   app/testpmd: fix flow update
>   app/testpmd: fix flow destroy
>

Hi Aman, Ori,

Can you please help reviewing this patch series?

Re: [PATCH v1 0/2] Testpmd flow update/destroy fixes

[Serhii Iliushyk]

>On 11.11.2024, 15:35, "Ferruh Yigit" wrote:
>
>
>On 10/31/2024 3:00 PM, Danylo Vodopianov wrote:
>> These patches provide next fixes:
>> 1. The testpmd command “flow update…“ provides a nullptr as the
>> context variable.
>> 2. Avoid removal of additional flows after requested number of flows has
>> been already removed.
>> 
>> Danylo Vodopianov (2):
>> app/testpmd: fix flow update
>> app/testpmd: fix flow destroy
>>
>
>
>Hi Aman, Ori,
>
>
>Can you please help reviewing this patch series?
>

Hi everyone 

We kindly ask you to review the patches and fixes for the application testpmd.
The primary purpose of these patches is to provide stable behavior of the feature `flow update` together with `aging`.

We are ready to answer any questions about changes in the patches.

Thanks,
Serhii

Re: [PATCH v1 0/2] Testpmd flow update/destroy fixes

[Thomas Monjalon]

14/11/2024 15:12, Serhii Iliushyk:
> >On 11.11.2024, 15:35, "Ferruh Yigit" wrote:
> >
> >
> >On 10/31/2024 3:00 PM, Danylo Vodopianov wrote:
> >> These patches provide next fixes:
> >> 1. The testpmd command “flow update…“ provides a nullptr as the
> >> context variable.
> >> 2. Avoid removal of additional flows after requested number of flows has
> >> been already removed.
> >> 
> >> Danylo Vodopianov (2):
> >> app/testpmd: fix flow update
> >> app/testpmd: fix flow destroy
> >>
> >
> >
> >Hi Aman, Ori,
> >
> >
> >Can you please help reviewing this patch series?
> >
> 
> Hi everyone 
> 
> We kindly ask you to review the patches and fixes for the application testpmd.
> The primary purpose of these patches is to provide stable behavior of the feature `flow update` together with `aging`.
> 
> We are ready to answer any questions about changes in the patches.

+Cc more NVIDIA devs

RE: [PATCH v2 1/2] app/testpmd: fix flow update

[Dariusz Sosnowski]

> -----Original Message-----
> From: Danylo Vodopianov <dvo-plv@napatech.com>
> Sent: Monday, November 18, 2024 11:25
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> aman.deep.singh@intel.com; yuying.zhang@intel.com; Ori Kam
> <orika@nvidia.com>; mko-plv@napatech.com; ckm@napatech.com; Dariusz
> Sosnowski <dsosnowski@nvidia.com>; sil-plv@napatech.com
> Cc: Gregory Etelson <getelson@nvidia.com>; Alexander Kozyrev
> <akozyrev@nvidia.com>; dev@dpdk.org; stable@dpdk.org;
> ferruh.yigit@amd.com
> Subject: [PATCH v2 1/2] app/testpmd: fix flow update
> 
> External email: Use caution opening links or attachments
> 
> 
> If actions provided to “flow update…“ command contained an age action, then
> testpmd did not update the age action context accordingly.
> 
> Thus "flow aged <port_id> destroy" command can not execute successfully.
> 
> Fix was done with next steps
> 1. Generate new port flow entry to add/replace action(s).
> 2. Set age context if age action is present.
> 3. Replace flow in the flow list.
> 
> Fixes: 2d9c7e56e52c ("app/testpmd: support updating flow rule actions")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Danylo Vodopianov <dvo-plv@napatech.com>
> ---
>  app/test-pmd/config.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 88770b4dfc..c831166431 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -3882,7 +3882,8 @@ port_flow_update(portid_t port_id, uint32_t
> rule_id,
>                  const struct rte_flow_action *actions, bool is_user_id)  {
>         struct rte_port *port;
> -       struct port_flow **flow_list;
> +       struct port_flow **flow_list, *uf;
> +       struct rte_flow_action_age *age = age_action_get(actions);
> 
>         if (port_id_is_invalid(port_id, ENABLED_WARN) ||
>             port_id == (portid_t)RTE_PORT_ALL) @@ -3897,6 +3898,16 @@
> port_flow_update(portid_t port_id, uint32_t rule_id,
>                         flow_list = &flow->next;
>                         continue;
>                 }
> +
> +               /* Update flow action(s) with new action(s) */
> +               uf = port_flow_new(flow->rule.attr_ro, flow->rule.pattern_ro,
> actions, &error);
> +               if (!uf)
> +                       return port_flow_complain(&error);
> +               if (age) {
> +                       flow->age_type = ACTION_AGE_CONTEXT_TYPE_FLOW;
> +                       age->context = &flow->age_type;
> +               }
> +
>                 /*
>                  * Poisoning to make sure PMDs update it in case
>                  * of error.
> @@ -3913,6 +3924,14 @@ port_flow_update(portid_t port_id, uint32_t
> rule_id,
>                         printf("Flow rule #%"PRIu64
>                                " updated with new actions\n",
>                                flow->id);
> +
> +               uf->next = flow->next;
> +               uf->id = flow->id;
> +               uf->user_id = flow->user_id;
> +               uf->flow = flow->flow;
> +               *flow_list = uf;
> +
> +               free(flow);
>                 return 0;
>         }
>         printf("Failed to find flow %"PRIu32"\n", rule_id);
> --
> 2.43.5

Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com>

Best regards,
Dariusz Sosnowski

RE: [PATCH v2 0/2] Testpmd flow update/destroy fixes

[Dariusz Sosnowski]

Hi Danylo,

Could you please resend the v2 of the patchset? I'm not sure why, but for some reason the patches are missing in patchwork. I also cannot find them on http://inbox.dpdk.org/dev/

Best regards,
Dariusz Sosnowski

> -----Original Message-----
> From: Danylo Vodopianov <dvo-plv@napatech.com>
> Sent: Monday, November 18, 2024 11:25
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> aman.deep.singh@intel.com; yuying.zhang@intel.com; Ori Kam
> <orika@nvidia.com>; mko-plv@napatech.com; ckm@napatech.com; Dariusz
> Sosnowski <dsosnowski@nvidia.com>; sil-plv@napatech.com
> Cc: Gregory Etelson <getelson@nvidia.com>; Alexander Kozyrev
> <akozyrev@nvidia.com>; dev@dpdk.org; stable@dpdk.org;
> ferruh.yigit@amd.com
> Subject: [PATCH v2 0/2] Testpmd flow update/destroy fixes
> 
> External email: Use caution opening links or attachments
> 
> 
> These patches provide next fixes:
>         1. The testpmd command “flow update…“ provides a nullptr as the
>            context variable.
>         2. Avoid removal of additional flows after requested number of flows has
>            been already removed.
> v2:
>         1. Rephase commit messages.
>         2. Copy user_id to the flow list for flow_update command.
>         3. Enclose the case's body for flow_destroy command in braces.
> 
> Danylo Vodopianov (2):
>   app/testpmd: fix flow update
>   app/testpmd: fix aged flow destroy
> 
>  app/test-pmd/config.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> --
> 2.43.5
> 
> Disclaimer: This email and any files transmitted with it may contain
> confidential information intended for the addressee(s) only. The information is
> not to be surrendered or copied to unauthorized persons. If you have received
> this communication in error, please notify the sender immediately and delete
> this e-mail from your system.

RE: [PATCH v2 0/2] Testpmd flow update/destroy fixes

[Dariusz Sosnowski]

It seems that you're not registered to stable mailing list.
Could you please register on that list and try again?
You can use this link to register: https://mails.dpdk.org/listinfo/stable

> From: Danylo Vodopianov <dvo-plv@napatech.com> 
> Sent: Monday, November 18, 2024 11:48
> To: Dariusz Sosnowski <dsosnowski@nvidia.com>; NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>; aman.deep.singh@intel.com; yuying.zhang@intel.com; Ori Kam <orika@nvidia.com>; Mykola Kostenok <mko-plv@napatech.com>; Christian Koue Muf <ckm@napatech.com>; Serhii Iliushyk <sil-plv@napatech.com>
> Cc: Gregory Etelson <getelson@nvidia.com>; Alexander Kozyrev <akozyrev@nvidia.com>; dev@dpdk.org; stable@dpdk.org; ferruh.yigit@amd.com
> Subject: RE: [PATCH v2 0/2] Testpmd flow update/destroy fixes
> 
> External email: Use caution opening links or attachments 
> 
> Hi, Dariusz
> 
> I think the reason is that email:
> 
> 
> I also get it for v1. Maybe you could advise, to who to contact I should ?
> ________________________________________
> От: Dariusz Sosnowski <mailto:dsosnowski@nvidia.com>
> Отправлено: 18 ноября 2024 г. 12:43
> Кому: Danylo Vodopianov <mailto:dvo-plv@napatech.com>; NBU-Contact-Thomas Monjalon (EXTERNAL) <mailto:thomas@monjalon.net>; mailto:aman.deep.singh@intel.com <mailto:aman.deep.singh@intel.com>; mailto:yuying.zhang@intel.com <mailto:yuying.zhang@intel.com>; Ori Kam <mailto:orika@nvidia.com>; Mykola Kostenok <mailto:mko-plv@napatech.com>; Christian Koue Muf <mailto:ckm@napatech.com>; Serhii Iliushyk <mailto:sil-plv@napatech.com>
> Копия: Gregory Etelson <mailto:getelson@nvidia.com>; Alexander Kozyrev <mailto:akozyrev@nvidia.com>; mailto:dev@dpdk.org <mailto:dev@dpdk.org>; mailto:stable@dpdk.org <mailto:stable@dpdk.org>; mailto:ferruh.yigit@amd.com <mailto:ferruh.yigit@amd.com>
> Тема: RE: [PATCH v2 0/2] Testpmd flow update/destroy fixes 
>  
> Hi Danylo,
> 
> Could you please resend the v2 of the patchset? I'm not sure why, but for some reason the patches are missing in patchwork. I also cannot find them on https://linkprotect.cudasvc.com/url?a=http%3a%2f%2finbox.dpdk.org%2fdev%2f&c=E,1,dcZxFWB-eo1ewbHY7A168mmU8SGWF_I8DZ4pWHzAoDaMq5fBfu4xHL1kYRZJjDsBCqtKWlkfnn7_fKtrD8shz7BfbAmgFnTgi61I6AAPFseq9ppG&typo=1
> 
> Best regards,
> Dariusz Sosnowski
> 
> > -----Original Message-----
> > From: Danylo Vodopianov <mailto:dvo-plv@napatech.com>
> > Sent: Monday, November 18, 2024 11:25
> > To: NBU-Contact-Thomas Monjalon (EXTERNAL) <mailto:thomas@monjalon.net>;
> > mailto:aman.deep.singh@intel.com; mailto:yuying.zhang@intel.com; Ori Kam
> > <mailto:orika@nvidia.com>; mailto:mko-plv@napatech.com; mailto:ckm@napatech.com; Dariusz
> > Sosnowski <mailto:dsosnowski@nvidia.com>; mailto:sil-plv@napatech.com
> > Cc: Gregory Etelson <mailto:getelson@nvidia.com>; Alexander Kozyrev
> > <mailto:akozyrev@nvidia.com>; mailto:dev@dpdk.org; mailto:stable@dpdk.org;
> > mailto:ferruh.yigit@amd.com
> > Subject: [PATCH v2 0/2] Testpmd flow update/destroy fixes
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > These patches provide next fixes:
> >         1. The testpmd command "flow update:" provides a nullptr as the
> >            context variable.
> >         2. Avoid removal of additional flows after requested number of flows has
> >            been already removed.
> > v2:
> >         1. Rephase commit messages.
> >         2. Copy user_id to the flow list for flow_update command.
> >         3. Enclose the case's body for flow_destroy command in braces.
> > 
> > Danylo Vodopianov (2):
> >   app/testpmd: fix flow update
> >   app/testpmd: fix aged flow destroy
> > 
> >  app/test-pmd/config.c | 27 ++++++++++++++++++++++++---
> >  1 file changed, 24 insertions(+), 3 deletions(-)
> > 
> > --
> > 2.43.5
> > 
> > Disclaimer: This email and any files transmitted with it may contain
> > confidential information intended for the addressee(s) only. The information is
> > not to be surrendered or copied to unauthorized persons. If you have received
> > this communication in error, please notify the sender immediately and delete
> > this e-mail from your system.
> Disclaimer: This email and any files transmitted with it may contain confidential information intended for the addressee(s) only. The information is not to be surrendered or copied to unauthorized persons. If you have received this communication in error, please notify the sender immediately and delete this e-mail from your system. 
>

[PATCH v2 0/2] Testpmd flow update/destroy fixes

[Danylo Vodopianov]

These patches provide next fixes:
	1. The testpmd command “flow update…“ provides a nullptr as the
	   context variable.
	2. Avoid removal of additional flows after requested number of flows has
	   been already removed.
v2:
	1. Rephase commit messages.
	2. Copy user_id to the flow list for flow_update command.
	3. Enclose the case's body for flow_destroy command in braces.

Danylo Vodopianov (2):
  app/testpmd: fix flow update
  app/testpmd: fix aged flow destroy

 app/test-pmd/config.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

-- 
2.43.5

[PATCH v2 1/2] app/testpmd: fix flow update

[Danylo Vodopianov]

If actions provided to “flow update…“ command contained an age
action, then testpmd did not update the age action context
accordingly.

Thus "flow aged <port_id> destroy" command can not
execute successfully.

Fix was done with next steps
1. Generate new port flow entry to add/replace action(s).
2. Set age context if age action is present.
3. Replace flow in the flow list.

Fixes: 2d9c7e56e52c ("app/testpmd: support updating flow rule actions")
Cc: stable@dpdk.org

Signed-off-by: Danylo Vodopianov <dvo-plv@napatech.com>
---
 app/test-pmd/config.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 88770b4dfc..c831166431 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3882,7 +3882,8 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
 		 const struct rte_flow_action *actions, bool is_user_id)
 {
 	struct rte_port *port;
-	struct port_flow **flow_list;
+	struct port_flow **flow_list, *uf;
+	struct rte_flow_action_age *age = age_action_get(actions);
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN) ||
 	    port_id == (portid_t)RTE_PORT_ALL)
@@ -3897,6 +3898,16 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
 			flow_list = &flow->next;
 			continue;
 		}
+
+		/* Update flow action(s) with new action(s) */
+		uf = port_flow_new(flow->rule.attr_ro, flow->rule.pattern_ro, actions, &error);
+		if (!uf)
+			return port_flow_complain(&error);
+		if (age) {
+			flow->age_type = ACTION_AGE_CONTEXT_TYPE_FLOW;
+			age->context = &flow->age_type;
+		}
+
 		/*
 		 * Poisoning to make sure PMDs update it in case
 		 * of error.
@@ -3913,6 +3924,14 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
 			printf("Flow rule #%"PRIu64
 			       " updated with new actions\n",
 			       flow->id);
+
+		uf->next = flow->next;
+		uf->id = flow->id;
+		uf->user_id = flow->user_id;
+		uf->flow = flow->flow;
+		*flow_list = uf;
+
+		free(flow);
 		return 0;
 	}
 	printf("Failed to find flow %"PRIu32"\n", rule_id);
-- 
2.43.5

RE: [PATCH v2 1/2] app/testpmd: fix flow update

[Dariusz Sosnowski]

> -----Original Message-----
> From: Danylo Vodopianov <dvo-plv@napatech.com>
> Sent: Monday, November 18, 2024 12:26
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> aman.deep.singh@intel.com; yuying.zhang@intel.com; Ori Kam
> <orika@nvidia.com>; mko-plv@napatech.com; ckm@napatech.com; Dariusz
> Sosnowski <dsosnowski@nvidia.com>; sil-plv@napatech.com
> Cc: Gregory Etelson <getelson@nvidia.com>; Alexander Kozyrev
> <akozyrev@nvidia.com>; dev@dpdk.org; stable@dpdk.org;
> ferruh.yigit@amd.com
> Subject: [PATCH v2 1/2] app/testpmd: fix flow update
> 
> If actions provided to “flow update…“ command contained an age action, then
> testpmd did not update the age action context accordingly.
> 
> Thus "flow aged <port_id> destroy" command can not execute successfully.
> 
> Fix was done with next steps
> 1. Generate new port flow entry to add/replace action(s).
> 2. Set age context if age action is present.
> 3. Replace flow in the flow list.
> 
> Fixes: 2d9c7e56e52c ("app/testpmd: support updating flow rule actions")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Danylo Vodopianov <dvo-plv@napatech.com>
> ---
>  app/test-pmd/config.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 88770b4dfc..c831166431 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -3882,7 +3882,8 @@ port_flow_update(portid_t port_id, uint32_t rule_id,
>                  const struct rte_flow_action *actions, bool is_user_id)  {
>         struct rte_port *port;
> -       struct port_flow **flow_list;
> +       struct port_flow **flow_list, *uf;
> +       struct rte_flow_action_age *age = age_action_get(actions);
> 
>         if (port_id_is_invalid(port_id, ENABLED_WARN) ||
>             port_id == (portid_t)RTE_PORT_ALL) @@ -3897,6 +3898,16 @@
> port_flow_update(portid_t port_id, uint32_t rule_id,
>                         flow_list = &flow->next;
>                         continue;
>                 }
> +
> +               /* Update flow action(s) with new action(s) */
> +               uf = port_flow_new(flow->rule.attr_ro, flow->rule.pattern_ro, actions,
> &error);
> +               if (!uf)
> +                       return port_flow_complain(&error);
> +               if (age) {
> +                       flow->age_type = ACTION_AGE_CONTEXT_TYPE_FLOW;
> +                       age->context = &flow->age_type;
> +               }
> +
>                 /*
>                  * Poisoning to make sure PMDs update it in case
>                  * of error.
> @@ -3913,6 +3924,14 @@ port_flow_update(portid_t port_id, uint32_t
> rule_id,
>                         printf("Flow rule #%"PRIu64
>                                " updated with new actions\n",
>                                flow->id);
> +
> +               uf->next = flow->next;
> +               uf->id = flow->id;
> +               uf->user_id = flow->user_id;
> +               uf->flow = flow->flow;
> +               *flow_list = uf;
> +
> +               free(flow);
>                 return 0;
>         }
>         printf("Failed to find flow %"PRIu32"\n", rule_id);
> --
> 2.43.5

Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com>

Best regards,
Dariusz Sosnowski

[PATCH v2 2/2] app/testpmd: fix aged flow destroy

[Danylo Vodopianov]

Avoid removal of additional flows after requested number of flows has
been already removed.

port_flow_destroy() function goes through all flows and compares
given flow ‘id’ with them. However in some cases it can advance pointer
with “given ID” and thus remove additional flow.

port_flow_destroy() function never assumed that rule array can be freed
when it's executing, and port_flow_aged() just violated that assumption.

Fixes: de956d5ecf08 ("app/testpmd: support age shared action context")
Cc: stable@dpdk.org

Signed-off-by: Danylo Vodopianov <dvo-plv@napatech.com>
---
 app/test-pmd/config.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index c831166431..04de2fe59d 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -4160,8 +4160,9 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
 		}
 		type = (enum age_action_context_type *)contexts[idx];
 		switch (*type) {
-		case ACTION_AGE_CONTEXT_TYPE_FLOW:
+		case ACTION_AGE_CONTEXT_TYPE_FLOW: {
 			ctx.pf = container_of(type, struct port_flow, age_type);
+			uint64_t flow_id = ctx.pf->id;
 			printf("%-20s\t%" PRIu64 "\t%" PRIu32 "\t%" PRIu32
 								 "\t%c%c%c\t\n",
 			       "Flow",
@@ -4172,9 +4173,10 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
 			       ctx.pf->rule.attr->egress ? 'e' : '-',
 			       ctx.pf->rule.attr->transfer ? 't' : '-');
 			if (destroy && !port_flow_destroy(port_id, 1,
-							  &ctx.pf->id, false))
+							  &flow_id, false))
 				total++;
 			break;
+		}
 		case ACTION_AGE_CONTEXT_TYPE_INDIRECT_ACTION:
 			ctx.pia = container_of(type,
 					struct port_indirect_action, age_type);
-- 
2.43.5

RE: [PATCH v2 2/2] app/testpmd: fix aged flow destroy

[Dariusz Sosnowski]

> -----Original Message-----
> From: Danylo Vodopianov <dvo-plv@napatech.com>
> Sent: Monday, November 18, 2024 12:26
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> aman.deep.singh@intel.com; yuying.zhang@intel.com; Ori Kam
> <orika@nvidia.com>; mko-plv@napatech.com; ckm@napatech.com; Dariusz
> Sosnowski <dsosnowski@nvidia.com>; sil-plv@napatech.com
> Cc: Gregory Etelson <getelson@nvidia.com>; Alexander Kozyrev
> <akozyrev@nvidia.com>; dev@dpdk.org; stable@dpdk.org;
> ferruh.yigit@amd.com
> Subject: [PATCH v2 2/2] app/testpmd: fix aged flow destroy
> 
> Avoid removal of additional flows after requested number of flows has been
> already removed.
> 
> port_flow_destroy() function goes through all flows and compares given flow ‘id’
> with them. However in some cases it can advance pointer with “given ID” and
> thus remove additional flow.

Could you please remove the first two paragraphs in the commit message?
I don't think they are relevant.

> 
> port_flow_destroy() function never assumed that rule array can be freed when
> it's executing, and port_flow_aged() just violated that assumption.

Also here, could you please describe what exactly is changed in port_flow_aged?
Current content explains the reason for the bug and the commit should describe how it's fixed.

> 
> Fixes: de956d5ecf08 ("app/testpmd: support age shared action context")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Danylo Vodopianov <dvo-plv@napatech.com>
> ---
>  app/test-pmd/config.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> c831166431..04de2fe59d 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -4160,8 +4160,9 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
>                 }
>                 type = (enum age_action_context_type *)contexts[idx];
>                 switch (*type) {
> -               case ACTION_AGE_CONTEXT_TYPE_FLOW:
> +               case ACTION_AGE_CONTEXT_TYPE_FLOW: {
>                         ctx.pf = container_of(type, struct port_flow, age_type);
> +                       uint64_t flow_id = ctx.pf->id;

Could you please refactor these lines as follows?

```
case ACTION_AGE_CONTEXT_TYPE_FLOW: {
uint64_t flow_id;

ctx.pf = container_of(type, struct port_flow, age_type);
flow_id = ctx.pf->id;
```

This is to make the style aligned - variables declared at the beginning of the block.

>                         printf("%-20s\t%" PRIu64 "\t%" PRIu32 "\t%" PRIu32
>                                                                  "\t%c%c%c\t\n",
>                                "Flow",
> @@ -4172,9 +4173,10 @@ port_flow_aged(portid_t port_id, uint8_t destroy)
>                                ctx.pf->rule.attr->egress ? 'e' : '-',
>                                ctx.pf->rule.attr->transfer ? 't' : '-');
>                         if (destroy && !port_flow_destroy(port_id, 1,
> -                                                         &ctx.pf->id, false))
> +                                                         &flow_id,
> + false))
>                                 total++;
>                         break;
> +               }
>                 case ACTION_AGE_CONTEXT_TYPE_INDIRECT_ACTION:
>                         ctx.pia = container_of(type,
>                                         struct port_indirect_action, age_type);
> --
> 2.43.5