[v1,2/2] app/testpmd: fix flow destroy
Checks
Commit Message
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(-)
Comments
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
@@ -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: