Re: [dpdk-dev] [PATCH v4 6/6] testpmd: Set Rx VMDq RSS mode

[Vlad Zolotarov <vladz@cloudius-systems.com>]

On 01/06/15 04:01, Ouyang, Changchun wrote:
>
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>> Sent: Monday, January 5, 2015 6:12 PM
>> To: Ouyang, Changchun; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v4 6/6] testpmd: Set Rx VMDq RSS mode
>>
>>
>> On 01/05/15 04:38, Ouyang, Changchun wrote:
>>>> -----Original Message-----
>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>> Sent: Sunday, January 4, 2015 5:47 PM
>>>> To: Ouyang, Changchun; dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH v4 6/6] testpmd: Set Rx VMDq RSS mode
>>>>
>>>>
>>>> On 01/04/15 11:01, Ouyang, Changchun wrote:
>>>>>> -----Original Message-----
>>>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>>>> Sent: Sunday, January 4, 2015 4:50 PM
>>>>>> To: Ouyang, Changchun; dev@dpdk.org
>>>>>> Subject: Re: [dpdk-dev] [PATCH v4 6/6] testpmd: Set Rx VMDq RSS
>>>>>> mode
>>>>>>
>>>>>>
>>>>>> On 01/04/15 09:18, Ouyang Changchun wrote:
>>>>>>> Set VMDq RSS mode if it has VF(VF number is more than 1) and has
>>>>>>> RSS
>>>>>> information.
>>>>>>> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
>>>>>>> ---
>>>>>>>      app/test-pmd/testpmd.c | 10 ++++++++++
>>>>>>>      1 file changed, 10 insertions(+)
>>>>>>>
>>>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>>>> 8c69756..6230f8b 100644
>>>>>>> --- a/app/test-pmd/testpmd.c
>>>>>>> +++ b/app/test-pmd/testpmd.c
>>>>>>> @@ -1708,6 +1708,16 @@ init_port_config(void)
>>>>>>>      				port->dev_conf.rxmode.mq_mode =
>>>>>> ETH_MQ_RX_NONE;
>>>>>>>      		}
>>>>>>>
>>>>>>> +		if (port->dev_info.max_vfs != 0) {
>>>>>>> +			if (port-
>>> dev_conf.rx_adv_conf.rss_conf.rss_hf != 0)
>>>>>>> +				port->dev_conf.rxmode.mq_mode =
>>>>>>> +					ETH_MQ_RX_VMDQ_RSS;
>>>>>>> +			else {
>>>>>>> +				port->dev_conf.rxmode.mq_mode =
>>>>>> ETH_MQ_RX_NONE;
>>>>>>> +				port->dev_conf.txmode.mq_mode =
>>>>>> ETH_MQ_TX_NONE;
>>>>>>
>>>>>> And what about the txmode.mq_mode when RSS is available (the :if"
>>>> clause)?
>>>>> I think we can keep its original value for txmode.mq_mode, so don't
>>>> change its value. How do you think of it?
>>>>
>>>> I agree that not changing a Tx mq_mode in both cases would be better.
>>> In the else clause, set txmode.mq_mode as ETH_MQ_TX_NONE explicitly
>> to
>>> make sure it is neither ETH_MQ_TX_DCB, ETH_MQ_TX_VMDQ_DCB, nor
>> ETH_MQ_TX_VMDQ_ONLY.
>>
>> It's not obvious to me why u should do that since AFAIK any of these modes
>> requires RX_RSS. Do I miss anything?
> No, I don't think so, in the else clause, it doesn't need rx_rss, and no way to do it,
> because the case is there is no rss configuration information(note: in the else clause, dev_conf.rx_adv_conf.rss_conf.rss_hf == 0).
>
> So ETH_MQ_RX_NONE for rx_mode, and ETH_MQ_TX_NONE for tx_mode.

Of course, however, in general, one may ask, why u configure TX MQ mode 
in "else" clause an don't do it in the "if" one. Possibly the "if" case 
in TX MQ context has been handled elsewhere but this is what makes this 
code confusing: to make it the most readable u'd rather configure the 
same feature set in both "if" and "else".
For instance:

if (bla-bla) {
   tx_mode = X1;
   rx_mode = X2;
} else {
  tx_mode = Y1;
  rx_mode = Y2;
}

Look at the non-SR-IOV clause right above the "if-else" block u've 
added. Why don't they configure tx_mode there? Is it a bug in their code?
By the way, u forgot to fix the remark below

/* In SR-IOV mode, RSS mode is not available */

which is located a few lines above the code u've added. ;)

>
>>>>> Thanks
>>>>> Changchun
>>>>>
>>>>>