Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

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

On 5/21/2019 2:30 AM, Zhao1, Wei wrote:
> Hi, Ferruh
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Monday, May 20, 2019 11:23 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>
>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
>> default configuration
>>
>> On 5/14/2019 2:56 AM, Zhao1, Wei wrote:
>>> Hi,  Ferruh
>>>
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Tuesday, May 14, 2019 12:36 AM
>>>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>>>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
>>>> <wenzhuo.lu@intel.com>
>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
>>>> overwrite by default configuration
>>>>
>>>> On 5/9/2019 8:20 AM, Wei Zhao wrote:
>>>>> There is an error in function rxtx_port_config(), which may
>>>>> overwrite offloads configuration get from function
>>>>> launch_args_parse() when run testpmd app. So rxtx_port_config() should
>> do "or" for port offloads.
>>>>>
>>>>> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
>>>>> cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>>>> ---
>>>>>  app/test-pmd/testpmd.c | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>> 6fbfd29..f0061d9 100644
>>>>> --- a/app/test-pmd/testpmd.c
>>>>> +++ b/app/test-pmd/testpmd.c
>>>>> @@ -2809,9 +2809,12 @@ static void
>>>>>  rxtx_port_config(struct rte_port *port)  {
>>>>>  	uint16_t qid;
>>>>> +	uint64_t offloads;
>>>>>
>>>>>  	for (qid = 0; qid < nb_rxq; qid++) {
>>>>> +		offloads = port->rx_conf[qid].offloads;
>>>>>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
>>>>> +		port->rx_conf[qid].offloads |= offloads;
>>>>
>>>> OK to this changes as a fix for this release.
>>>>
>>>> But I think intention is, if no offload information is provided by
>>>> user to use use the driver provided defaults, if user explicitly
>>>> provided some values to use them, instead of OR these two.
>>>>
>>>> With this approach it is not possible to disable a driver default
>>>> value, so it becomes mandatory offload instead of default offload values.
>>>>
>>>> Wei, what do you think, does it make sense?
>>>
>>>
>>> I agree with you, but it is sure that the original code has offloads overwrite
>> issue.
>>> What is your suggestion for code implement?
>>> I find that Thomas has apply it, if you has other idea, maybe you has to
>> commit patch base to this patch.
>>
>> Hi Wei,
>>
>> Yes this needs to be incremental fix to existing code.
>>
>> Queue specific offload can be altered either by providing Rx/Tx offload as
>> command line argument [1] (port configs set to each queues) or via testpmd
>> commands [2].
>> Does it make sense to set a global flag when one of above occurs and use
>> default config only if it is not set?
> 
> I  AGREE with you to submit an incremental fix, and it make sense to set a global flag when one of above occurs and use
>  default config only if it is not set when implement code, but I do not have time to prepare such a patch by now, so maybe later or some else.

I see, can you submit a public defect to record the issue, so it can be
addressed later without forgotten?

> 
>>
>> [1]
>> Tx
>>   tx-offloads
>> Rx
>>   disable-crc-strip
>>   enable-lro
>>   enable-scatter
>>   enable-rx-cksum
>>   enable-rx-timestamp
>>   enable-hw-vlan
>>   enable-hw-vlan-filter
>>   enable-hw-vlan-strip
>>   enable-hw-vlan-extend
>>
>> [2]
>> "port config <port_id> rx_offload ..."
>> "port <port_id> rxq <queue_id> rx_offload ..."
>> "port config <port_id> tx_offload ..."
>> "port <port_id> txq <queue_id> tx_offload ..."
>>
>