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

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

On 6/12/2019 2:17 AM, Zhao1, Wei wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, June 11, 2019 10:37 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>; Kevin Traynor <ktraynor@redhat.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;
>>
>> While talking with Kevin, he pointed out the error in this code.
>>
>> We are updating queue level offloads, with whatever in the 'offloads' and it can
>> be non-queue level offloads in it, next time ethdev API called these values are
>> caught by the API checks and causing an error.
>>
>> It looks like port level offload flags needs to be masted out before writing to
>> queue level 'offloads' variable.
> 
> 
> By the way, this error in not introduced in this patch, it seems has exist long before this patch.
> This patch is just fix for overwrite problem.

I disagree, writing 'offloads' to "rx_conf[].offloads" without checking if they
queue offloads or not causing this problem. And that write introduced in this patch.


> 
> 
> 
>>
>>>
>>>  		/* Check if any Rx parameters have been passed */
>>>  		if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2833,7
>> +2836,9 @@
>>> rxtx_port_config(struct rte_port *port)
>>>  	}
>>>
>>>  	for (qid = 0; qid < nb_txq; qid++) {
>>> +		offloads = port->tx_conf[qid].offloads;
>>>  		port->tx_conf[qid] = port->dev_info.default_txconf;
>>> +		port->tx_conf[qid].offloads |= offloads;
>>>
>>>  		/* Check if any Tx parameters have been passed */
>>>  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
>>>
>