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

[Dariusz Sosnowski <dsosnowski@nvidia.com>]

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