Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@amd.com>
> Sent: Tuesday, March 19, 2024 10:21 PM
> To: Bing Zhao <bi...@nvidia.com>; dmitry.kozl...@gmail.com;
> dev@dpdk.org
> Cc: aman.deep.si...@intel.com; yuying.zh...@intel.com; Matan Azrad
> <ma...@nvidia.com>; sta...@dpdk.org; Dariusz Sosnowski
> <dsosnow...@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.kozl...@gmail.com
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Bing Zhao <bi...@nvidia.com>
> > Reviewed-by: Dariusz Sosnowski <dsosnow...@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
