diff mbox

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

Message ID 1420355937-18484-5-git-send-email-changchun.ouyang@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Ouyang Changchun Jan. 4, 2015, 7:18 a.m. UTC 
  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(-)

Comments

Vladislav Zolotarov Jan. 4, 2015, 8:45 a.m. UTC | #1 
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) {
> +				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?

> +			break;
>   		default: /* ETH_MQ_RX_VMDQ_ONLY or ETH_MQ_RX_NONE */
>   			/* if nothing mq mode configure, use default scheme */
>   			dev->data->dev_conf.rxmode.mq_mode = ETH_MQ_RX_VMDQ_ONLY;
> @@ -553,8 +584,6 @@ rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   		default: /* ETH_MQ_TX_VMDQ_ONLY or ETH_MQ_TX_NONE */
>   			/* if nothing mq mode configure, use default scheme */
>   			dev->data->dev_conf.txmode.mq_mode = ETH_MQ_TX_VMDQ_ONLY;
> -			if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)
> -				RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;
>   			break;
>   		}
>
Ouyang Changchun Jan. 4, 2015, 8:58 a.m. UTC | #2 
> -----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) {
> > +				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.

Thanks
Changchun
Vladislav Zolotarov Jan. 4, 2015, 9:45 a.m. UTC | #3 
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?

>>> +				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.

>
> Thanks
> Changchun
>     
>
Ouyang Changchun Jan. 5, 2015, 1 a.m. UTC | #4 
> -----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.

> >
> > Thanks
> > Changchun
> >
> >
Vladislav Zolotarov Jan. 5, 2015, 10:09 a.m. UTC | #5 
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.

>
>>> Thanks
>>> Changchun
>>>
>>>
Ouyang Changchun Jan. 6, 2015, 1:56 a.m. UTC | #6 
> -----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.

> >

> >>> Thanks

> >>> Changchun

> >>>

> >>>
Vladislav Zolotarov Jan. 6, 2015, 7:56 p.m. UTC | #7 
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
>>>>>
>>>>>
Ouyang Changchun Jan. 7, 2015, 2:28 a.m. UTC | #8 
> -----Original Message-----

> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]

> Sent: Wednesday, January 7, 2015 3:56 AM

> To: Ouyang, Changchun; dev@dpdk.org

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

> 

> 

> 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.


Well, it couldn't be done so easily, I think,  the test condition is:
if (nb_rx_q <= RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool),  
so the else clause is the case of nb_rx_q > RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool,
its functionality is comparing nb_rx_q and RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool,

but the switch case will further confine nb_rx_q to 1 or 2 or 4 on the condition of it passes the above test,

and also there are codes refine the RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool etc.

just changing the return into break, will break the logic, 
e.g.
when  nb_rx_q is 8, and RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool is 8,
the test pass, and go into default branch,
it just print some message and break, 
continue refining(but nothing changed this time),
then check valid queue number a few lines below, this time it fail the test, because 
nb_rx_q == rather than >  RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool , so it doesn't print err mesge and don't return the -EINVAL.

Then the behavior is not expected.

From other hand,
The reason why I have not the else branch for the test nb_rx_q <= RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool,
It  is because there is same check below itself, and just don't want the duplicated check for the same thing

> >>>>> Thanks

> >>>>> Changchun

> >>>>>

> >>>>>
diff mbox

Patch

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) {
+				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;
+			}
+			break;
 		default: /* ETH_MQ_RX_VMDQ_ONLY or ETH_MQ_RX_NONE */
 			/* if nothing mq mode configure, use default scheme */
 			dev->data->dev_conf.rxmode.mq_mode = ETH_MQ_RX_VMDQ_ONLY;
@@ -553,8 +584,6 @@  rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		default: /* ETH_MQ_TX_VMDQ_ONLY or ETH_MQ_TX_NONE */
 			/* if nothing mq mode configure, use default scheme */
 			dev->data->dev_conf.txmode.mq_mode = ETH_MQ_TX_VMDQ_ONLY;
-			if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)
-				RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;
 			break;
 		}