Re: [dpdk-dev] [PATCH v4 2/2] app/testpmd: fix max-pkt-len option invalid

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

On 1/25/2021 3:46 PM, Lance Richardson wrote:
> On Mon, Jan 25, 2021 at 3:35 AM Steve Yang <stevex.yang@intel.com> wrote:
>>
>> Moved the setting of 'DEV_RX_OFFLOAD_JUMBO_FRAME' from
>> 'cmd_config_max_pkt_len_parsed()' to 'init_config()' caused fail the case
>> where 'max_rx_pkt_len' is changed from the command line via
>> "port config all max-pkt-len".
>>
>> The 'init_config()' function is only called when testpmd is started,
>> but the DEV_RX_OFFLOAD_JUMBO_FRAME setting needs to be recomputed whenever
>> 'max_rx_pkt_len' changes.
>>
>> Define the 'update_jumbo_frame_offload()' function for both 'init_config()'
>> and 'cmd_config_max_pkt_len_parsed()', and detect if 'max_rx_pkt_len'
>> should be update to 1500 + overhead as default configuration. At the same
>> time, the offloads of rx queue also should update the value once port
>> offloads changed (e.g.: disabled JUMBO_FRAME offload via changed
>> max-pkt-len, reproduce steps [1]), otherwise, it would cause unexpected
>> result.
>>
>> [1]
>> --------------------------------------------------------------------------
>> ./x86_64-native-linuxapp-gcc/app/dpdk-testpmd  -c 0xf -n 4  -- -i
>>      --max-pkt-len=9000 --tx-offloads=0x8000 --rxq=4 --txq=4 --disable-rss
>> testpmd>  set verbose 3
>> testpmd>  port stop all
>> testpmd>  port config all max-pkt-len 1518  port start all
>>
>> // Got fail error info without this patch
>> Configuring Port 0 (socket 1)
>> Ethdev port_id=0 rx_queue_id=0, new added offloads 0x800 must be
>> within per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()
>> Fail to configure port 0 rx queues //<-- Fail error info;
>> --------------------------------------------------------------------------
>>
>> Fixes: 761c4d6690 ("app/testpmd: fix max Rx packet length for VLAN packets")
>>
>> Signed-off-by: Steve Yang <stevex.yang@intel.com>
>> ---
>>   app/test-pmd/cmdline.c    |  1 +
>>   app/test-pmd/parameters.c |  1 +
>>   app/test-pmd/testpmd.c    | 63 ++++++++++++++++++++++++++++-----------
>>   app/test-pmd/testpmd.h    |  2 ++
>>   4 files changed, 50 insertions(+), 17 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 89034c8b72..8b6b7b6206 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -1901,6 +1901,7 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
>>                          printf("Unknown parameter\n");
>>                          return;
>>                  }
>> +               update_jumbo_frame_offload(pid, false);
> 
> I'm probably missing something, but why isn't this a matter of simply calling
> port_mtu_set() here (with mtu computed from max pkt len) and keeping
> init_config() as currently implemented?
> 

They are very similar, calling the 'port_mtu_set()' can be an option, but it is 
headache to get dev->info first and calculate overhead to be able to call 
'port_mtu_set()';
Since this is already done in 'init_config()', converting it to a function and 
reuse it also looks valid as done here.

I am updating that function and sending a new version soon.