[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