app/testpmd: fix releasing action handle flush memory
Checks
Commit Message
The memory of the indirect action handles should be freed after
being destroyed in the flush. The behavior needs to be consistent
with the single handle destroy.
Or else, there will be some unexpected error when the action handle
is destroyed for the 2nd time, for example, the port needs to be
closed again.
Fixes: f7352c176bbf ("app/testpmd: fix use of indirect action after port close")
Cc: dmitry.kozliuk@gmail.com
Cc: stable@dpdk.org
Signed-off-by: Bing Zhao <bingz@nvidia.com>
Reviewed-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
---
app/test-pmd/config.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
Comments
On 3/19/2024 9:32 AM, Bing Zhao wrote:
> The memory of the indirect action handles should be freed after
> being destroyed in the flush. The behavior needs to be consistent
> with the single handle destroy.
>
> Or else, there will be some unexpected error when the action handle
> is destroyed for the 2nd time, for example, the port needs to be
> closed again.
>
Ports can be closed only once, so above reasoning is not valid, but I
assume the purpose of this patch is to prevent memory leak, can you
please clarify the problem and impact of the patch in commit log?
Also what is "single handle destroy" mentioned above?
The fixed commit is from a few release ago, so this is not something
introduced in this release, do you think can this wait next release
instead of merging for -rc4 which is more risky?
> Fixes: f7352c176bbf ("app/testpmd: fix use of indirect action after port close")
> Cc: dmitry.kozliuk@gmail.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> Reviewed-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
> ---
> app/test-pmd/config.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index ba1007ace6..f62ba90c87 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1918,8 +1918,7 @@ port_action_handle_flush(portid_t port_id)
> /* Poisoning to make sure PMDs update it in case of error. */
> memset(&error, 0x44, sizeof(error));
> if (pia->handle != NULL) {
> - ret = pia->type ==
> - RTE_FLOW_ACTION_TYPE_INDIRECT_LIST ?
> + ret = pia->type == RTE_FLOW_ACTION_TYPE_INDIRECT_LIST ?
> rte_flow_action_list_handle_destroy
> (port_id, pia->list_handle, &error) :
> rte_flow_action_handle_destroy
> @@ -1929,11 +1928,9 @@ port_action_handle_flush(portid_t port_id)
> pia->id);
> ret = port_flow_complain(&error);
> }
> - tmp = &pia->next;
> - } else {
> - *tmp = pia->next;
> - free(pia);
> }
> + *tmp = pia->next;
> + free(pia);
> }
> return ret;
> }
Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Tuesday, March 19, 2024 10:21 PM
> To: Bing Zhao <bingz@nvidia.com>; dmitry.kozliuk@gmail.com;
> dev@dpdk.org
> Cc: aman.deep.singh@intel.com; yuying.zhang@intel.com; Matan Azrad
> <matan@nvidia.com>; stable@dpdk.org; Dariusz Sosnowski
> <dsosnowski@nvidia.com>
> Subject: Re: [PATCH] app/testpmd: fix releasing action handle flush memory
>
> External email: Use caution opening links or attachments
>
>
> On 3/19/2024 9:32 AM, Bing Zhao wrote:
> > The memory of the indirect action handles should be freed after being
> > destroyed in the flush. The behavior needs to be consistent with the
> > single handle destroy.
> >
> > Or else, there will be some unexpected error when the action handle is
> > destroyed for the 2nd time, for example, the port needs to be closed
> > again.
> >
>
> Ports can be closed only once, so above reasoning is not valid, but I assume
> the purpose of this patch is to prevent memory leak, can you please clarify the
> problem and impact of the patch in commit log?
To close the port twice is something I am testing internally of "errno EBUSY", I didn't describe it clearly.
At the same time, YES, there is some memory leak should be fixed.
>
>
> Also what is "single handle destroy" mentioned above?
I mean the function call port_action_handle_destroy().
>
> The fixed commit is from a few release ago, so this is not something
> introduced in this release, do you think can this wait next release instead of
> merging for -rc4 which is more risky?
Yes, it is not something that blocking the release. The memory should be recycled by the system once the
application quits completely.
I will polish the commit message and send v2.
>
>
> > Fixes: f7352c176bbf ("app/testpmd: fix use of indirect action after
> > port close")
> > Cc: dmitry.kozliuk@gmail.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Bing Zhao <bingz@nvidia.com>
> > Reviewed-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
> > ---
> > app/test-pmd/config.c | 9 +++------
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > ba1007ace6..f62ba90c87 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -1918,8 +1918,7 @@ port_action_handle_flush(portid_t port_id)
> > /* Poisoning to make sure PMDs update it in case of error. */
> > memset(&error, 0x44, sizeof(error));
> > if (pia->handle != NULL) {
> > - ret = pia->type ==
> > - RTE_FLOW_ACTION_TYPE_INDIRECT_LIST ?
> > + ret = pia->type == RTE_FLOW_ACTION_TYPE_INDIRECT_LIST ?
> > rte_flow_action_list_handle_destroy
> > (port_id, pia->list_handle, &error) :
> > rte_flow_action_handle_destroy @@ -1929,11
> > +1928,9 @@ port_action_handle_flush(portid_t port_id)
> > pia->id);
> > ret = port_flow_complain(&error);
> > }
> > - tmp = &pia->next;
> > - } else {
> > - *tmp = pia->next;
> > - free(pia);
> > }
> > + *tmp = pia->next;
> > + free(pia);
> > }
> > return ret;
> > }
Thanks
BR. Bing
@@ -1918,8 +1918,7 @@ port_action_handle_flush(portid_t port_id)
/* Poisoning to make sure PMDs update it in case of error. */
memset(&error, 0x44, sizeof(error));
if (pia->handle != NULL) {
- ret = pia->type ==
- RTE_FLOW_ACTION_TYPE_INDIRECT_LIST ?
+ ret = pia->type == RTE_FLOW_ACTION_TYPE_INDIRECT_LIST ?
rte_flow_action_list_handle_destroy
(port_id, pia->list_handle, &error) :
rte_flow_action_handle_destroy
@@ -1929,11 +1928,9 @@ port_action_handle_flush(portid_t port_id)
pia->id);
ret = port_flow_complain(&error);
}
- tmp = &pia->next;
- } else {
- *tmp = pia->next;
- free(pia);
}
+ *tmp = pia->next;
+ free(pia);
}
return ret;
}