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

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

[Bing Zhao]

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(-)

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;
 }
-- 
2.34.1

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

[Ferruh Yigit]

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;
>  }

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

[Bing Zhao]

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

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

[Bing Zhao]

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 port_action_handle_destroy().

Or else, there would be some memory leak when closing / detaching a
port without quitting the application. In the meanwhile, since the
action handles are already destroyed, it makes no sense to hold the
indirect action software resources anymore.

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>
---
v2: update the description
---
 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;
 }
-- 
2.34.1

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

[Ferruh Yigit]

On 3/25/2024 10:58 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 port_action_handle_destroy().
> 
> Or else, there would be some memory leak when closing / detaching a
> port without quitting the application. In the meanwhile, since the
> action handles are already destroyed, it makes no sense to hold the
> indirect action software resources anymore.
> 
> 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>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.