Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode

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

On 01/06/15 03:56, Ouyang, Changchun wrote:
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>> Sent: Monday, January 5, 2015 6:10 PM
>> To: Ouyang, Changchun;dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode
>>
>>
>> On 01/05/15 03:00, Ouyang, Changchun wrote:
>>>> -----Original Message-----
>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>> Sent: Sunday, January 4, 2015 5:46 PM
>>>> To: Ouyang, Changchun;dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode
>>>>
>>>>
>>>> On 01/04/15 10:58, Ouyang, Changchun wrote:
>>>>>> -----Original Message-----
>>>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>>>>>> Sent: Sunday, January 4, 2015 4:45 PM
>>>>>> To: Ouyang, Changchun;dev@dpdk.org
>>>>>> Subject: Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode
>>>>>>
>>>>>>
>>>>>> On 01/04/15 09:18, Ouyang Changchun wrote:
>>>>>>> Check mq mode for VMDq RSS, handle it correctly instead of
>>>>>>> returning an error; Also remove the limitation of per pool queue
>>>>>>> number has max value of 1, because the per pool queue number
>> could
>>>>>>> be 2 or 4 if it is VMDq RSS mode;
>>>>>>>
>>>>>>> The number of rxq specified in config will determine the mq mode
>>>>>>> for
>>>>>> VMDq RSS.
>>>>>>> Signed-off-by: Changchun Ouyang<changchun.ouyang@intel.com>
>>>>>>> ---
>>>>>>>      lib/librte_ether/rte_ethdev.c | 39
>>>>>> ++++++++++++++++++++++++++++++++++-----
>>>>>>>      1 file changed, 34 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/lib/librte_ether/rte_ethdev.c
>>>>>>> b/lib/librte_ether/rte_ethdev.c index 95f2ceb..59ff325 100644
>>>>>>> --- a/lib/librte_ether/rte_ethdev.c
>>>>>>> +++ b/lib/librte_ether/rte_ethdev.c
>>>>>>> @@ -510,8 +510,7 @@ rte_eth_dev_check_mq_mode(uint8_t
>> port_id,
>>>>>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>>>>>>>
>>>>>>>      	if (RTE_ETH_DEV_SRIOV(dev).active != 0) {
>>>>>>>      		/* check multi-queue mode */
>>>>>>> -		if ((dev_conf->rxmode.mq_mode ==
>> ETH_MQ_RX_RSS) ||
>>>>>>> -		    (dev_conf->rxmode.mq_mode ==
>> ETH_MQ_RX_DCB) ||
>>>>>>> +		if ((dev_conf->rxmode.mq_mode ==
>> ETH_MQ_RX_DCB) ||
>>>>>>>      		    (dev_conf->rxmode.mq_mode ==
>> ETH_MQ_RX_DCB_RSS)
>>>>>> ||
>>>>>>>      		    (dev_conf->txmode.mq_mode ==
>> ETH_MQ_TX_DCB)) {
>>>>>>>      			/* SRIOV only works in VMDq enable mode
>> */ @@ -
>>>>>> 525,7 +524,6 @@
>>>>>>> rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q,
>>>>>> uint16_t nb_tx_q,
>>>>>>>      		}
>>>>>>>
>>>>>>>      		switch (dev_conf->rxmode.mq_mode) {
>>>>>>> -		case ETH_MQ_RX_VMDQ_RSS:
>>>>>>>      		case ETH_MQ_RX_VMDQ_DCB:
>>>>>>>      		case ETH_MQ_RX_VMDQ_DCB_RSS:
>>>>>>>      			/* DCB/RSS VMDQ in SRIOV mode, not
>> implement
>>>>>> yet */ @@ -534,6
>>>>>>> +532,39 @@ rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t
>>>>>> nb_rx_q, uint16_t nb_tx_q,
>>>>>>>      					"unsupported VMDQ
>> mq_mode
>>>>>> rx %u\n",
>>>>>>>      					port_id, dev_conf-
>>>>>>> rxmode.mq_mode);
>>>>>>>      			return (-EINVAL);
>>>>>>> +		case ETH_MQ_RX_RSS:
>>>>>>> +			PMD_DEBUG_TRACE("ethdev port_id=%"
>> PRIu8
>>>>>>> +					" SRIOV active, "
>>>>>>> +					"Rx mq mode is changed
>> from:"
>>>>>>> +					"mq_mode %u into VMDQ
>>>>>> mq_mode %u\n",
>>>>>>> +					port_id,
>>>>>>> +					dev_conf-
>>> rxmode.mq_mode,
>>>>>>> +					dev->data-
>>>>>>> dev_conf.rxmode.mq_mode);
>>>>>>> +		case ETH_MQ_RX_VMDQ_RSS:
>>>>>>> +			dev->data->dev_conf.rxmode.mq_mode =
>>>>>> ETH_MQ_RX_VMDQ_RSS;
>>>>>>> +			if (nb_rx_q <
>>>>>> RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool) {
>>>> Missed that before: shouldn't it be "<=" here?
>>> Agree with you, need <= here, I will fix it in v5
>>>
>>>>>>> +				switch (nb_rx_q) {
>>>>>>> +				case 1:
>>>>>>> +				case 2:
>>>>>>> +
>> 	RTE_ETH_DEV_SRIOV(dev).active =
>>>>>>> +						ETH_64_POOLS;
>>>>>>> +					break;
>>>>>>> +				case 4:
>>>>>>> +
>> 	RTE_ETH_DEV_SRIOV(dev).active =
>>>>>>> +						ETH_32_POOLS;
>>>>>>> +					break;
>>>>>>> +				default:
>>>>>>> +
>> 	PMD_DEBUG_TRACE("ethdev
>>>>>> port_id=%d"
>>>>>>> +						" SRIOV active, "
>>>>>>> +						"queue number
>> invalid\n",
>>>>>>> +						port_id);
>>>>>>> +					return -EINVAL;
>>>>>>> +				}
>>>>>>> +
>> 	RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool =
>>>>>> nb_rx_q;
>>>>>>> +
>> 	RTE_ETH_DEV_SRIOV(dev).def_pool_q_idx =
>>>>>>> +					dev->pci_dev->max_vfs *
>> nb_rx_q;
>>>>>>> +			}
>>>>>> Don't u need to return an error in the "else" here?
>>>>> Actually it has such a check after these code snippet, and it does
>>>>> return error for the else case, Because it is original logic, I
>>>>> don't change any
>>>> code around it, so it doesn't display here, you can check the codes.
>>>>
>>>> I see. The flow is a bit confusing since the switch-case above will
>>>> end up executing a "default" clause which will set
>>>> RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool to 1 and then the error
>> message
>>>> in the check u are referring will be a bit confusing.
>>> ' set RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool to 1 ' is original code,
>> which is for vmdq only case, or single queue case.
>>> It is in default clause, and not in VMDQ_RSS clause.
>>> I think my new code is ok here.
>> The original code is ok and your current code will work. The only problem
>> with your new code is that in case on an error like I've described above the
>> error message will be confusing.
> Then what's your suggestion for the better log message?  I can consider refine it if you have better one.

Just like I've suggested before - u may break with appropriate error 
message right when u see the problem (in a "else" clause). This way the 
code will be both more readable and more robust and won't break if 
anybody decides to change the not-RSS-specific logic u r relying on.

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