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

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