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

[Vlad Zolotarov]

On 01/05/15 03:00, Ouyang, Changchun wrote:
>
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
>> Sent: Sunday, January 4, 2015 5:46 PM
>> To: Ouyang, Changchun; dev at 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 at cloudius-systems.com]
>>>> Sent: Sunday, January 4, 2015 4:45 PM
>>>> To: Ouyang, Changchun; dev at 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 at 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.

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