Re: [PATCH] app/testpmd: fix quit testpmd with vfs and pf

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

On 3/28/2022 11:06 AM, Singh, Aman Deep wrote:
> 
> 
> On 3/22/2022 12:48 PM, Ke Zhang wrote:
>> When testpmd startups with pf and vfs,this error occurs when quitting,
>> results in pf is released before vfs ,so the vf would access an
>> freed heap memory.
>>
>> The solution is two steps:
>> 1. Fetch the valid port value from RTE_ETH_FOREACH_DEV.
>> 2. free the port in reverse order.
>>
>> Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
>> ---
> This fix looks similar to series: 21823. Please check Ferruh's comments 
> on it.
> 
> There is a scenario, where this fix may not work.
> "if there is an earlier port id which is released, after that new
> vf is created taking that port id. In such case port id of vf < pf port id"
> 
> This patch covers most of the cases and that way its an improvement.
> So I am ok with it.
> 
> Acked-by: Aman Singh <aman.deep.singh@intel.com>

Agree that patch covers most of the cases with simple/small change, so 
OK to continue with it.

@Ke, can you please check minor comments below.

>>   app/test-pmd/testpmd.c | 28 +++++++++++++++++++++++-----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index f7e18aee25..ca6c77b14b 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -3378,8 +3378,11 @@ detach_devargs(char *identifier)
>>   void
>>   pmd_test_exit(void)
>>   {
>> +    unsigned int ports[RTE_MAX_ETHPORTS];

testpmd holds port IDs as 'portid_t', so can you please change type form 
'int' to 'portid_t'?

>> +    unsigned int count = 0;

For 'index' & 'count', no reason to make one 'unsigned int' and other 
'int', better to keep type same for both, and both can be 'int'.

>>       portid_t pt_id;
>>       unsigned int i;
>> +    int index;

These new variables only used in one if block, can you please move 
declaring them to that scope, to the beginning of that block.

>>       int ret;
>>       if (test_done == 0)
>> @@ -3396,15 +3399,30 @@ pmd_test_exit(void)
>>   #endif
>>       if (ports != NULL) {
>>           no_link_check = 1;
>> +        i = 0;
>> +
>> +        /* Fetch the valid port id from port list*/
>>           RTE_ETH_FOREACH_DEV(pt_id) {
>> -            printf("\nStopping port %d...\n", pt_id);
>> +            ports[i] = pt_id;
>> +            i++;

Can use 'count' directly, instead of 'i'.

>> +        }
>> +
>> +        count = i;
>> +        /*
>> +         * Free the port from Reverse order, as general,
>> +         * PF port < VF port， VF should be free before PF
>> +         * be free.
>> +         */
>> +        for (index = count - 1 ; index >= 0 ; index--) {
>> +            printf("\nStopping port %d...\n", ports[index]);
>>               fflush(stdout);
>> -            stop_port(pt_id);
>> +            stop_port(ports[index]);
>>           }
>> -        RTE_ETH_FOREACH_DEV(pt_id) {
>> -            printf("\nShutting down port %d...\n", pt_id);
>> +
>> +        for (index = count - 1 ; index >= 0 ; index--) {
>> +            printf("\nShutting down port %d...\n", ports[index]);
>>               fflush(stdout);
>> -            close_port(pt_id);
>> +            close_port(ports[index]);
>>           }
>>       }
>