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

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

On 5/24/2019 2:55 AM, Zhao1, Wei wrote:
> 
> Hi, Ferruh
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, May 21, 2019 11:43 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/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?
> 
> Sure, but what is a public defect patch? Do you mean I need to update some doc? Can you give me a link as an example 

No documentation, please create an issue in public DPDK bug tracker:
https://bugs.dpdk.org/


> 
> 
>>
>>>
>>>>
>>>> [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 ..."
>>>>
>>>
>