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

[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

[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 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

[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 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

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

[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

[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

[PATCH v3 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.

v3:
	1. Update commit message
	2. Aligned variable declaration code style

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

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

-- 
2.43.5

[PATCH v3 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

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

[Danylo Vodopianov]

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

In case of flow async create failure, it tries to do a cleanup, but it
wrongly removes a 1st flow (with id 0). pf->id is not set at this
moment and it always is 0, thus 1st flow is removed. A local copy of
flow->id must be used to call of port_flow_destroy() to avoid access
and processing of flow->id after the flow is removed.

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 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index c831166431..28d45568ac 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -4160,8 +4160,10 @@ 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: {
+			uint64_t flow_id;
 			ctx.pf = container_of(type, struct port_flow, age_type);
+			flow_id = ctx.pf->id;
 			printf("%-20s\t%" PRIu64 "\t%" PRIu32 "\t%" PRIu32
 								 "\t%c%c%c\t\n",
 			       "Flow",
@@ -4172,9 +4174,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 v3 1/2] app/testpmd: fix flow update

[Dariusz Sosnowski]

> -----Original Message-----
> From: Danylo Vodopianov <dvo-plv@napatech.com>
> Sent: Monday, November 18, 2024 19:03
> 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 v3 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
> 
> 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.

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

Best regards,
Dariusz Sosnowski

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

[Dariusz Sosnowski]

> -----Original Message-----
> From: Danylo Vodopianov <dvo-plv@napatech.com>
> Sent: Monday, November 18, 2024 19:03
> 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 v3 2/2] app/testpmd: fix aged flow destroy
> 
> External email: Use caution opening links or attachments
> 
> 
> port_flow_destroy() function never assumed that rule array can be freed when
> it's executing, and port_flow_aged() just violated that assumption.
> 
> In case of flow async create failure, it tries to do a cleanup, but it wrongly removes
> a 1st flow (with id 0). pf->id is not set at this moment and it always is 0, thus 1st
> flow is removed. A local copy of
> flow->id must be used to call of port_flow_destroy() to avoid access
> and processing of flow->id after the flow is removed.
> 
> 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 | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> c831166431..28d45568ac 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -4160,8 +4160,10 @@ 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: {
> +                       uint64_t flow_id;
>                         ctx.pf = container_of(type, struct port_flow, age_type);
> +                       flow_id = ctx.pf->id;
>                         printf("%-20s\t%" PRIu64 "\t%" PRIu32 "\t%" PRIu32
>                                                                  "\t%c%c%c\t\n",
>                                "Flow",
> @@ -4172,9 +4174,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
> 
> 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.

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

Best regards,
Dariusz Sosnowski

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

[Thomas Monjalon]

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

Applied, thanks.