RE: [PATCH] app/testpmd: fix releasing action handle flush memory

[Bing Zhao <bingz@nvidia.com>]

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