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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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