Hi,
> -----Original Message-----
> From: Danylo Vodopianov <dvo-...@napatech.com>
> Sent: Thursday, October 31, 2024 16:00
> To: NBU-Contact-Thomas Monjalon (EXTERNAL) <tho...@monjalon.net>;
> aman.deep.si...@intel.com; yuying.zh...@intel.com; Ori Kam
> <or...@nvidia.com>; mko-...@napatech.com; c...@napatech.com; sil-
> p...@napatech.com
> Cc: dev@dpdk.org; ferruh.yi...@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:
sta...@dpdk.org"
>
> Signed-off-by: Danylo Vodopianov <dvo-...@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
