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

[Dariusz Sosnowski <dsosnowski@nvidia.com>]

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