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

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

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?

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