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

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

On 6/17/2019 2:51 AM, Zhao1, Wei wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Friday, June 14, 2019 11:42 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 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.
>>
> 
> But if delete the patch I submitted, this bug still exists there.

OK, after checking again, the defect was there as you said, but your patch makes
it visible.

How to reproduce the problem:

testpmd> port stop all
testpmd> port config all crc-strip on
testpmd> port start all
Ethdev port_id=0 rx_queue_id=0, new added offloads 0x10000 must be within
per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()

The failure is coming from 'rte_eth_rx_queue_setup()' and a valid error, a "port
level offload" can't be set to individual queues without setting in port level,
that is what happening here.
"port config all crc-strip on" removes the 'keep_crc' offload from port but not
from queues, so when 'rte_eth_rx_queue_setup()' called it complains about it.

Before your patch "queue offloads" were overwritten and this error was not observed.

The solution can be updating "port config all xxxx on|off" command to updates
queue offload values too, but even better we can remove it completely:

There are already different testpmd commands for same thing:
"port config <port_id> rx_offload <offload>"
"port config <port_id> tx_offload <offload>"
"port <port_id> txq <queue_id> rx_offload <offload>"
"port <port_id> txq <queue_id> tx_offload <offload>"

For example using "port config 0 rx_offload crc-strip on" will work without
error since it sets the port and queue offload properly.

Only missing thing in above commands are "all" parameter. I suggest removing
"port config all <offload> on|off" and adding "all" support to above commands.


> 
>>
>>>
>>>
>>>
>>>>
>>>>>
>>>>>  		/* 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)
>>>>>
>>>
>