diff mbox

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

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

Commit Message

Ouyang Changchun Jan. 7, 2015, 6:32 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>

changes in v5:
  - Fix '<' issue, it should be '<=' to test rxq number;
  - Extract a function to remove the embeded switch-case statement.

---
 lib/librte_ether/rte_ethdev.c | 50 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 5 deletions(-)

Comments

Vladislav Zolotarov Jan. 8, 2015, 9:19 a.m. UTC | #1
On 01/07/15 08:32, 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>
>
> changes in v5:
>    - Fix '<' issue, it should be '<=' to test rxq number;
>    - Extract a function to remove the embeded switch-case statement.
>
> ---
>   lib/librte_ether/rte_ethdev.c | 50 ++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 95f2ceb..8363e26 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -503,6 +503,31 @@ rte_eth_dev_tx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
>   }
>   
>   static int
> +rte_eth_dev_check_vf_rss_rxq_num(uint8_t port_id, uint16_t nb_rx_q)
> +{
> +	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> +	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:
> +		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;
> +
> +	return 0;
> +}
> +
> +static int
>   rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   		      const struct rte_eth_conf *dev_conf)
>   {
> @@ -510,8 +535,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 +549,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 +557,25 @@ 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)
> +				if (rte_eth_dev_check_vf_rss_rxq_num(port_id, nb_rx_q) != 0) {
> +					PMD_DEBUG_TRACE("ethdev port_id=%d"
> +						" SRIOV active, invalid queue"
> +						" number for VMDQ RSS\n",
> +						port_id);

Some nitpicking here: I'd add the allowed values descriptions to the 
error message. Something like: "invalid queue number for VMDQ RSS. 
Allowed values are 1, 2 or 4\n".

> +					return -EINVAL;
> +				}
> +			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 +595,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;

I'm not sure u may just remove it. These lines originally belong to a 
different flow. Are u sure u can remove them like that? What if the 
mq_mode is ETH_MQ_RX_NONE and nb_q_per_pool has been initialized to 4 or 
8 in ixgbe_pf_host_init()?

>   			break;
>   		}
>
Vladislav Zolotarov Jan. 8, 2015, 6:48 p.m. UTC | #2
On 01/08/15 11:19, Vlad Zolotarov wrote:
>
> On 01/07/15 08:32, 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>
>>
>> changes in v5:
>>    - Fix '<' issue, it should be '<=' to test rxq number;
>>    - Extract a function to remove the embeded switch-case statement.
>>
>> ---
>>   lib/librte_ether/rte_ethdev.c | 50 
>> ++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 45 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/librte_ether/rte_ethdev.c 
>> b/lib/librte_ether/rte_ethdev.c
>> index 95f2ceb..8363e26 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -503,6 +503,31 @@ rte_eth_dev_tx_queue_config(struct rte_eth_dev 
>> *dev, uint16_t nb_queues)
>>   }
>>     static int
>> +rte_eth_dev_check_vf_rss_rxq_num(uint8_t port_id, uint16_t nb_rx_q)
>> +{
>> +    struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>> +    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:
>> +        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;
>> +
>> +    return 0;
>> +}
>> +
>> +static int
>>   rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q, 
>> uint16_t nb_tx_q,
>>                 const struct rte_eth_conf *dev_conf)
>>   {
>> @@ -510,8 +535,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 +549,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 +557,25 @@ 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)
>> +                if (rte_eth_dev_check_vf_rss_rxq_num(port_id, 
>> nb_rx_q) != 0) {
>> +                    PMD_DEBUG_TRACE("ethdev port_id=%d"
>> +                        " SRIOV active, invalid queue"
>> +                        " number for VMDQ RSS\n",
>> +                        port_id);
>
> Some nitpicking here: I'd add the allowed values descriptions to the 
> error message. Something like: "invalid queue number for VMDQ RSS. 
> Allowed values are 1, 2 or 4\n".
>
>> +                    return -EINVAL;
>> +                }
>> +            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 +595,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;
>
> I'm not sure u may just remove it. These lines originally belong to a 
> different flow. Are u sure u can remove them like that? What if the 
> mq_mode is ETH_MQ_RX_NONE and nb_q_per_pool has been initialized to 4 
> or 8 in ixgbe_pf_host_init()?

I misread the patch - these lines belong to the txmode.mq_mode switch 
case. I think it's ok to remove these really strange lines here. And 
when I look at it i think for the similar reasons the similar lines 
should be removed in the Rx case too: consider non-RSS case with MQ DCB 
Tx configuration.

>
>>               break;
>>           }
>
Ouyang Changchun Jan. 9, 2015, 5:54 a.m. UTC | #3
> -----Original Message-----
> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> Sent: Friday, January 9, 2015 2:49 AM
> To: Ouyang, Changchun; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode
> 
> 
> On 01/08/15 11:19, Vlad Zolotarov wrote:
> >
> > On 01/07/15 08:32, 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>
> >>
> >> changes in v5:
> >>    - Fix '<' issue, it should be '<=' to test rxq number;
> >>    - Extract a function to remove the embeded switch-case statement.
> >>
> >> ---
> >>   lib/librte_ether/rte_ethdev.c | 50
> >> ++++++++++++++++++++++++++++++++++++++-----
> >>   1 file changed, 45 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/lib/librte_ether/rte_ethdev.c
> >> b/lib/librte_ether/rte_ethdev.c index 95f2ceb..8363e26 100644
> >> --- a/lib/librte_ether/rte_ethdev.c
> >> +++ b/lib/librte_ether/rte_ethdev.c
> >> @@ -503,6 +503,31 @@ rte_eth_dev_tx_queue_config(struct
> rte_eth_dev
> >> *dev, uint16_t nb_queues)
> >>   }
> >>     static int
> >> +rte_eth_dev_check_vf_rss_rxq_num(uint8_t port_id, uint16_t nb_rx_q)
> >> +{
> >> +    struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> >> +    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:
> >> +        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;
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int
> >>   rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q,
> >> uint16_t nb_tx_q,
> >>                 const struct rte_eth_conf *dev_conf)
> >>   {
> >> @@ -510,8 +535,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
> >> +549,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 +557,25 @@ 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)
> >> +                if (rte_eth_dev_check_vf_rss_rxq_num(port_id,
> >> nb_rx_q) != 0) {
> >> +                    PMD_DEBUG_TRACE("ethdev port_id=%d"
> >> +                        " SRIOV active, invalid queue"
> >> +                        " number for VMDQ RSS\n",
> >> +                        port_id);
> >
> > Some nitpicking here: I'd add the allowed values descriptions to the
> > error message. Something like: "invalid queue number for VMDQ RSS.
> > Allowed values are 1, 2 or 4\n".
> >
> >> +                    return -EINVAL;
> >> +                }
> >> +            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 +595,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;
> >
> > I'm not sure u may just remove it. These lines originally belong to a
> > different flow. Are u sure u can remove them like that? What if the
> > mq_mode is ETH_MQ_RX_NONE and nb_q_per_pool has been initialized
> to 4
> > or 8 in ixgbe_pf_host_init()?
> 
> I misread the patch - these lines belong to the txmode.mq_mode switch case.
> I think it's ok to remove these really strange lines here. And when I look at it i
> think for the similar reasons the similar lines should be removed in the Rx
> case too: consider non-RSS case with MQ DCB Tx configuration.
> 
I search code in this function, only one place has   
" if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1) 
           RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;"

The only place is default branch, which is for rx_none, or vmdq_only mode,
We don't need remove this, as it should assign as 1 because it did use 1 queue per pool.
Vladislav Zolotarov Jan. 9, 2015, 1:49 p.m. UTC | #4
On 01/09/15 07:54, Ouyang, Changchun wrote:
>
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>> Sent: Friday, January 9, 2015 2:49 AM
>> To: Ouyang, Changchun; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode
>>
>>
>> On 01/08/15 11:19, Vlad Zolotarov wrote:
>>> On 01/07/15 08:32, 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>
>>>>
>>>> changes in v5:
>>>>     - Fix '<' issue, it should be '<=' to test rxq number;
>>>>     - Extract a function to remove the embeded switch-case statement.
>>>>
>>>> ---
>>>>    lib/librte_ether/rte_ethdev.c | 50
>>>> ++++++++++++++++++++++++++++++++++++++-----
>>>>    1 file changed, 45 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/lib/librte_ether/rte_ethdev.c
>>>> b/lib/librte_ether/rte_ethdev.c index 95f2ceb..8363e26 100644
>>>> --- a/lib/librte_ether/rte_ethdev.c
>>>> +++ b/lib/librte_ether/rte_ethdev.c
>>>> @@ -503,6 +503,31 @@ rte_eth_dev_tx_queue_config(struct
>> rte_eth_dev
>>>> *dev, uint16_t nb_queues)
>>>>    }
>>>>      static int
>>>> +rte_eth_dev_check_vf_rss_rxq_num(uint8_t port_id, uint16_t nb_rx_q)
>>>> +{
>>>> +    struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>>> +    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:
>>>> +        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;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int
>>>>    rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q,
>>>> uint16_t nb_tx_q,
>>>>                  const struct rte_eth_conf *dev_conf)
>>>>    {
>>>> @@ -510,8 +535,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
>>>> +549,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 +557,25 @@ 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)
>>>> +                if (rte_eth_dev_check_vf_rss_rxq_num(port_id,
>>>> nb_rx_q) != 0) {
>>>> +                    PMD_DEBUG_TRACE("ethdev port_id=%d"
>>>> +                        " SRIOV active, invalid queue"
>>>> +                        " number for VMDQ RSS\n",
>>>> +                        port_id);
>>> Some nitpicking here: I'd add the allowed values descriptions to the
>>> error message. Something like: "invalid queue number for VMDQ RSS.
>>> Allowed values are 1, 2 or 4\n".
>>>
>>>> +                    return -EINVAL;
>>>> +                }
>>>> +            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 +595,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;
>>> I'm not sure u may just remove it. These lines originally belong to a
>>> different flow. Are u sure u can remove them like that? What if the
>>> mq_mode is ETH_MQ_RX_NONE and nb_q_per_pool has been initialized
>> to 4
>>> or 8 in ixgbe_pf_host_init()?
>> I misread the patch - these lines belong to the txmode.mq_mode switch case.
>> I think it's ok to remove these really strange lines here. And when I look at it i
>> think for the similar reasons the similar lines should be removed in the Rx
>> case too: consider non-RSS case with MQ DCB Tx configuration.
>>
> I search code in this function, only one place has
> " if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)
>             RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;"
>
> The only place is default branch, which is for rx_none, or vmdq_only mode,

Here is a snippet of an rte_eth_dev_check_mq_mode() from the current master:

		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 */
			PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8
					" SRIOV active, "
					"unsupported VMDQ mq_mode rx %u\n",
					port_id, dev_conf->rxmode.mq_mode);
			return (-EINVAL);
		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;
			*if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)**                  <---- This is one
**				RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;*
			break;
		}

		switch (dev_conf->txmode.mq_mode) {
		case ETH_MQ_TX_VMDQ_DCB:
			/* DCB VMDQ in SRIOV mode, not implement yet */
			PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8
					" SRIOV active, "
					"unsupported VMDQ mq_mode tx %u\n",
					port_id, dev_conf->txmode.mq_mode);
			return (-EINVAL);
		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)              <------ This is two. This is what your patch is removing
				RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;
			break;
		}




> We don't need remove this, as it should assign as 1 because it did use 1 queue per pool.

And why is that? Just because RSS was not enabled? And what if a user 
wants multiple Tx queues? Mode 1100b of MRQE for instance?
Ouyang Changchun Jan. 12, 2015, 3:41 a.m. UTC | #5
From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
Sent: Friday, January 09, 2015 9:50 PM
To: Ouyang, Changchun; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode


On 01/09/15 07:54, Ouyang, Changchun wrote:





-----Original Message-----

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

Sent: Friday, January 9, 2015 2:49 AM

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

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





On 01/08/15 11:19, Vlad Zolotarov wrote:



On 01/07/15 08:32, 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><mailto:changchun.ouyang@intel.com>



changes in v5:

   - Fix '<' issue, it should be '<=' to test rxq number;

   - Extract a function to remove the embeded switch-case statement.



---

  lib/librte_ether/rte_ethdev.c | 50

++++++++++++++++++++++++++++++++++++++-----

  1 file changed, 45 insertions(+), 5 deletions(-)



diff --git a/lib/librte_ether/rte_ethdev.c

b/lib/librte_ether/rte_ethdev.c index 95f2ceb..8363e26 100644

--- a/lib/librte_ether/rte_ethdev.c

+++ b/lib/librte_ether/rte_ethdev.c

@@ -503,6 +503,31 @@ rte_eth_dev_tx_queue_config(struct

rte_eth_dev

*dev, uint16_t nb_queues)

  }

    static int

+rte_eth_dev_check_vf_rss_rxq_num(uint8_t port_id, uint16_t nb_rx_q)

+{

+    struct rte_eth_dev *dev = &rte_eth_devices[port_id];

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

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

+

+    return 0;

+}

+

+static int

  rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q,

uint16_t nb_tx_q,

                const struct rte_eth_conf *dev_conf)

  {

@@ -510,8 +535,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

+549,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 +557,25 @@ 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)

+                if (rte_eth_dev_check_vf_rss_rxq_num(port_id,

nb_rx_q) != 0) {

+                    PMD_DEBUG_TRACE("ethdev port_id=%d"

+                        " SRIOV active, invalid queue"

+                        " number for VMDQ RSS\n",

+                        port_id);



Some nitpicking here: I'd add the allowed values descriptions to the

error message. Something like: "invalid queue number for VMDQ RSS.

Allowed values are 1, 2 or 4\n".



+                    return -EINVAL;

+                }

+            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 +595,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;



I'm not sure u may just remove it. These lines originally belong to a

different flow. Are u sure u can remove them like that? What if the

mq_mode is ETH_MQ_RX_NONE and nb_q_per_pool has been initialized

to 4

or 8 in ixgbe_pf_host_init()?



I misread the patch - these lines belong to the txmode.mq_mode switch case.

I think it's ok to remove these really strange lines here. And when I look at it i

think for the similar reasons the similar lines should be removed in the Rx

case too: consider non-RSS case with MQ DCB Tx configuration.



I search code in this function, only one place has

" if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)

           RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;"



The only place is default branch, which is for rx_none, or vmdq_only mode,

Here is a snippet of an rte_eth_dev_check_mq_mode() from the current master:

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

                       PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8

                                      " SRIOV active, "

                                      "unsupported VMDQ mq_mode rx %u\n",

                                      port_id, dev_conf->rxmode.mq_mode);

                       return (-EINVAL);

               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;

                       if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)                 <---- This is one

                               RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;

                       break;

               }



               switch (dev_conf->txmode.mq_mode) {

               case ETH_MQ_TX_VMDQ_DCB:

                       /* DCB VMDQ in SRIOV mode, not implement yet */

                       PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8

                                      " SRIOV active, "

                                      "unsupported VMDQ mq_mode tx %u\n",

                                      port_id, dev_conf->txmode.mq_mode);

                       return (-EINVAL);

               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)              <------ This is two. This is what your patch is removing

                               RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;

                       break;

               }


Changchun: yes you are correct, what I mean in my last response is that only one place AFTER my removal, so there are 2 places before my removal.
no controversial here.




We don't need remove this, as it should assign as 1 because it did use 1 queue per pool.

And why is that? Just because RSS was not enabled? And what if a user wants multiple Tx queues? Mode 1100b of MRQE for instance?

Changchun: I can explain why I need this change(remove the second place) here,
In the txmode, when txmode is ETH_MQ_TX_NONE, but the rx mode could either be ETH_MQ_RX_NONE or
ETH_MQ_RX_VMDQ_RSS, so we could not forcedly set nb_q_per_pool into 1 just hit the condition of txmode is ETH_MQ_TX_NONE,
Because we need consider it is combination of rx mode is ETH_MQ_RX_VMDQ_RSS, and tx mode is  ETH_MQ_TX_NONE,
In such a case, the queue number per pool could be 1, or 2, or 4.

In another hand, introducing ETH_MQ_TX_VMDQ_RSS for tx mode, seems very strange, because tx side has no rss feature.

thanks Changchun
Vladislav Zolotarov Jan. 12, 2015, 1:58 p.m. UTC | #6
On 01/12/15 05:41, Ouyang, Changchun wrote:
>
> *From:*Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> *Sent:* Friday, January 09, 2015 9:50 PM
> *To:* Ouyang, Changchun; dev@dpdk.org
> *Subject:* Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode
>
> On 01/09/15 07:54, Ouyang, Changchun wrote:
>
>       
>
>       
>
>         -----Original Message-----
>
>         From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>
>         Sent: Friday, January 9, 2015 2:49 AM
>
>         To: Ouyang, Changchun;dev@dpdk.org  <mailto:dev@dpdk.org>
>
>         Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode
>
>           
>
>           
>
>         On 01/08/15 11:19, Vlad Zolotarov wrote:
>
>               
>
>             On 01/07/15 08:32, 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>  <mailto:changchun.ouyang@intel.com>
>
>                   
>
>                 changes in v5:
>
>                     - Fix '<' issue, it should be '<=' to test rxq number;
>
>                     - Extract a function to remove the embeded switch-case statement.
>
>                   
>
>                 ---
>
>                    lib/librte_ether/rte_ethdev.c | 50
>
>                 ++++++++++++++++++++++++++++++++++++++-----
>
>                    1 file changed, 45 insertions(+), 5 deletions(-)
>
>                   
>
>                 diff --git a/lib/librte_ether/rte_ethdev.c
>
>                 b/lib/librte_ether/rte_ethdev.c index 95f2ceb..8363e26 100644
>
>                 --- a/lib/librte_ether/rte_ethdev.c
>
>                 +++ b/lib/librte_ether/rte_ethdev.c
>
>                 @@ -503,6 +503,31 @@ rte_eth_dev_tx_queue_config(struct
>
>         rte_eth_dev
>
>                 *dev, uint16_t nb_queues)
>
>                    }
>
>                      static int
>
>                 +rte_eth_dev_check_vf_rss_rxq_num(uint8_t port_id, uint16_t nb_rx_q)
>
>                 +{
>
>                 +    struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>
>                 +    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:
>
>                 +        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;
>
>                 +
>
>                 +    return 0;
>
>                 +}
>
>                 +
>
>                 +static int
>
>                    rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q,
>
>                 uint16_t nb_tx_q,
>
>                                  const struct rte_eth_conf *dev_conf)
>
>                    {
>
>                 @@ -510,8 +535,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
>
>                 +549,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 +557,25 @@ 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)
>
>                 +                if (rte_eth_dev_check_vf_rss_rxq_num(port_id,
>
>                 nb_rx_q) != 0) {
>
>                 +                    PMD_DEBUG_TRACE("ethdev port_id=%d"
>
>                 +                        " SRIOV active, invalid queue"
>
>                 +                        " number for VMDQ RSS\n",
>
>                 +                        port_id);
>
>               
>
>             Some nitpicking here: I'd add the allowed values descriptions to the
>
>             error message. Something like: "invalid queue number for VMDQ RSS.
>
>             Allowed values are 1, 2 or 4\n".
>
>               
>
>                 +                    return -EINVAL;
>
>                 +                }
>
>                 +            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 +595,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;
>
>               
>
>             I'm not sure u may just remove it. These lines originally belong to a
>
>             different flow. Are u sure u can remove them like that? What if the
>
>             mq_mode is ETH_MQ_RX_NONE and nb_q_per_pool has been initialized
>
>         to 4
>
>             or 8 in ixgbe_pf_host_init()?
>
>           
>
>         I misread the patch - these lines belong to the txmode.mq_mode switch case.
>
>         I think it's ok to remove these really strange lines here. And when I look at it i
>
>         think for the similar reasons the similar lines should be removed in the Rx
>
>         case too: consider non-RSS case with MQ DCB Tx configuration.
>
>           
>
>     I search code in this function, only one place has
>
>     " if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)
>
>                 RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;"
>
>       
>
>     The only place is default branch, which is for rx_none, or vmdq_only mode,
>
>
> Here is a snippet of an rte_eth_dev_check_mq_mode() from the current 
> master:
>
>                 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 */
>                         PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8
>                                        " SRIOV active, "
>                                        "unsupported VMDQ mq_mode rx %u\n",
>                                        port_id, dev_conf->rxmode.mq_mode);
>                         return (-EINVAL);
>                 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;
>                         *if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)                 <---- This is one*
> *                                RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;*
>                         break;
>                 }
>   
>                 switch (dev_conf->txmode.mq_mode) {
>                 case ETH_MQ_TX_VMDQ_DCB:
>                         /* DCB VMDQ in SRIOV mode, not implement yet */
>                         PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8
>                                        " SRIOV active, "
>                                        "unsupported VMDQ mq_mode tx %u\n",
>                                        port_id, dev_conf->txmode.mq_mode);
>                         return (-EINVAL);
>                 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)              <------ This is two. This is what your patch is removing
>                                 RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;
>                         break;
>                 }
>
>
>
> Changchun: yes you are correct, what I mean in my last response is 
> that only one place AFTER my removal, so there are 2 places before my 
> removal.
> no controversial here.
>
>   
> We don't need remove this, as it should assign as 1 because it did use 1 queue per pool.
>
>
> And why is that? Just because RSS was not enabled? And what if a user 
> wants multiple Tx queues? Mode 1100b of MRQE for instance?
>
> Changchun: I can explain why I need this change(remove the second 
> place) here,
>

I understood why u needed it in the first place. I just say that for 
exactly the same reasons u need to remove the "first place" too. ;)

> In the txmode, when txmode is ETH_MQ_TX_NONE, but the rx mode could 
> either be ETH_MQ_RX_NONE or
>
> ETH_MQ_RX_VMDQ_RSS, so we could not forcedly set nb_q_per_pool into 1 
> just hit the condition of txmode is ETH_MQ_TX_NONE,
>
> Because we need consider it is combination of rx mode is 
> ETH_MQ_RX_VMDQ_RSS, and tx mode is  ETH_MQ_TX_NONE,
>
> In such a case, the queue number per pool could be 1, or 2, or 4.
>
> In another hand, introducing ETH_MQ_TX_VMDQ_RSS for tx mode, seems 
> very strange, because tx side has no rss feature.
>

It's called ETH_MQ_TX_VMDQ_DCB in DPDK notation. ;) However I see that 
it's not yet supported. But *when* it's going to be supported the above 
code will turn to be bogus since u actually don't want to set the 
nb_q_per_pool to 1 neither if Rx mode is not MQ and nor if Tx mode is 
not MQ but only if them **both* *are not MQ. And this "if" is simply 
missing in the rte_eth_dev_check_mq_mode().

>
> thanks Changchun
>
>
>
Ouyang Changchun Jan. 13, 2015, 1:50 a.m. UTC | #7
From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
Sent: Monday, January 12, 2015 9:59 PM
To: Ouyang, Changchun; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode


On 01/12/15 05:41, Ouyang, Changchun wrote:


From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
Sent: Friday, January 09, 2015 9:50 PM
To: Ouyang, Changchun; dev@dpdk.org<mailto:dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode


On 01/09/15 07:54, Ouyang, Changchun wrote:





-----Original Message-----

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

Sent: Friday, January 9, 2015 2:49 AM

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

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





On 01/08/15 11:19, Vlad Zolotarov wrote:



On 01/07/15 08:32, 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><mailto:changchun.ouyang@intel.com>



changes in v5:

   - Fix '<' issue, it should be '<=' to test rxq number;

   - Extract a function to remove the embeded switch-case statement.



---

  lib/librte_ether/rte_ethdev.c | 50

++++++++++++++++++++++++++++++++++++++-----

  1 file changed, 45 insertions(+), 5 deletions(-)



diff --git a/lib/librte_ether/rte_ethdev.c

b/lib/librte_ether/rte_ethdev.c index 95f2ceb..8363e26 100644

--- a/lib/librte_ether/rte_ethdev.c

+++ b/lib/librte_ether/rte_ethdev.c

@@ -503,6 +503,31 @@ rte_eth_dev_tx_queue_config(struct

rte_eth_dev

*dev, uint16_t nb_queues)

  }

    static int

+rte_eth_dev_check_vf_rss_rxq_num(uint8_t port_id, uint16_t nb_rx_q)

+{

+    struct rte_eth_dev *dev = &rte_eth_devices[port_id];

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

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

+

+    return 0;

+}

+

+static int

  rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q,

uint16_t nb_tx_q,

                const struct rte_eth_conf *dev_conf)

  {

@@ -510,8 +535,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

+549,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 +557,25 @@ 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)

+                if (rte_eth_dev_check_vf_rss_rxq_num(port_id,

nb_rx_q) != 0) {

+                    PMD_DEBUG_TRACE("ethdev port_id=%d"

+                        " SRIOV active, invalid queue"

+                        " number for VMDQ RSS\n",

+                        port_id);



Some nitpicking here: I'd add the allowed values descriptions to the

error message. Something like: "invalid queue number for VMDQ RSS.

Allowed values are 1, 2 or 4\n".



+                    return -EINVAL;

+                }

+            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 +595,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;



I'm not sure u may just remove it. These lines originally belong to a

different flow. Are u sure u can remove them like that? What if the

mq_mode is ETH_MQ_RX_NONE and nb_q_per_pool has been initialized

to 4

or 8 in ixgbe_pf_host_init()?



I misread the patch - these lines belong to the txmode.mq_mode switch case.

I think it's ok to remove these really strange lines here. And when I look at it i

think for the similar reasons the similar lines should be removed in the Rx

case too: consider non-RSS case with MQ DCB Tx configuration.



I search code in this function, only one place has

" if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)

           RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;"



The only place is default branch, which is for rx_none, or vmdq_only mode,

Here is a snippet of an rte_eth_dev_check_mq_mode() from the current master:

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

                       PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8

                                      " SRIOV active, "

                                      "unsupported VMDQ mq_mode rx %u\n",

                                      port_id, dev_conf->rxmode.mq_mode);

                       return (-EINVAL);

               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;

                       if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)                 <---- This is one

                               RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;

                       break;

               }



               switch (dev_conf->txmode.mq_mode) {

               case ETH_MQ_TX_VMDQ_DCB:

                       /* DCB VMDQ in SRIOV mode, not implement yet */

                       PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8

                                      " SRIOV active, "

                                      "unsupported VMDQ mq_mode tx %u\n",

                                      port_id, dev_conf->txmode.mq_mode);

                       return (-EINVAL);

               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)              <------ This is two. This is what your patch is removing

                               RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;

                       break;

               }



Changchun: yes you are correct, what I mean in my last response is that only one place AFTER my removal, so there are 2 places before my removal.
no controversial here.





We don't need remove this, as it should assign as 1 because it did use 1 queue per pool.

And why is that? Just because RSS was not enabled? And what if a user wants multiple Tx queues? Mode 1100b of MRQE for instance?

Changchun: I can explain why I need this change(remove the second place) here,

   I understood why u needed it in the first place. I just say that for exactly the same reasons u need to remove the "first place" too. ;)


Changchun: then I will try to explain why I can't remove the first place :)
When the rx mode is ETH_MQ_RX_NONE and tx mode is ETH_MQ_TX_NONE,
The function ixgbe_pf_host_init still set the nb_q_per_pool into 2 or 4 or 8 according to max vf num,
(actually at that point, it has no knowledge of what is the rx and tx configuration value, so have to just set
an estimated (and not so accurate) value according to the max vf num)
then in the check_mq_mode function, need further refine this value according to a few factors:
sriov.active, and rxmode.mq_mode.
When it finds the rx mode is RX_NONE, and the nb_q_per_pool is larger than 1, then it should refine to 1.
So if I remove the first place, VMDQ_RSS case works well, but I break the case of RX_NONE.

So I think we can't treat rx path and tx path in absolutely same way here, i.e. if you add it in the first place(rx path) then you need also add it in the second place(tx path)
Vice versa,
that's my understanding :)

Thanks
Changchun
Vladislav Zolotarov Jan. 13, 2015, 9 a.m. UTC | #8
On 01/13/15 03:50, Ouyang, Changchun wrote:
>
> *From:*Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> *Sent:* Monday, January 12, 2015 9:59 PM
> *To:* Ouyang, Changchun; dev@dpdk.org
> *Subject:* Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode
>
> On 01/12/15 05:41, Ouyang, Changchun wrote:
>
>     *From:*Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>     *Sent:* Friday, January 09, 2015 9:50 PM
>     *To:* Ouyang, Changchun; dev@dpdk.org <mailto:dev@dpdk.org>
>     *Subject:* Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode
>
>     On 01/09/15 07:54, Ouyang, Changchun wrote:
>
>           
>
>           
>
>             -----Original Message-----
>
>             From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>
>             Sent: Friday, January 9, 2015 2:49 AM
>
>             To: Ouyang, Changchun;dev@dpdk.org  <mailto:dev@dpdk.org>
>
>             Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode
>
>               
>
>               
>
>             On 01/08/15 11:19, Vlad Zolotarov wrote:
>
>                   
>
>                 On 01/07/15 08:32, 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>  <mailto:changchun.ouyang@intel.com>
>
>                       
>
>                     changes in v5:
>
>                         - Fix '<' issue, it should be '<=' to test rxq number;
>
>                         - Extract a function to remove the embeded switch-case statement.
>
>                       
>
>                     ---
>
>                        lib/librte_ether/rte_ethdev.c | 50
>
>                     ++++++++++++++++++++++++++++++++++++++-----
>
>                        1 file changed, 45 insertions(+), 5 deletions(-)
>
>                       
>
>                     diff --git a/lib/librte_ether/rte_ethdev.c
>
>                     b/lib/librte_ether/rte_ethdev.c index 95f2ceb..8363e26 100644
>
>                     --- a/lib/librte_ether/rte_ethdev.c
>
>                     +++ b/lib/librte_ether/rte_ethdev.c
>
>                     @@ -503,6 +503,31 @@ rte_eth_dev_tx_queue_config(struct
>
>             rte_eth_dev
>
>                     *dev, uint16_t nb_queues)
>
>                        }
>
>                          static int
>
>                     +rte_eth_dev_check_vf_rss_rxq_num(uint8_t port_id, uint16_t nb_rx_q)
>
>                     +{
>
>                     +    struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>
>                     +    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:
>
>                     +        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;
>
>                     +
>
>                     +    return 0;
>
>                     +}
>
>                     +
>
>                     +static int
>
>                        rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q,
>
>                     uint16_t nb_tx_q,
>
>                                      const struct rte_eth_conf *dev_conf)
>
>                        {
>
>                     @@ -510,8 +535,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
>
>                     +549,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 +557,25 @@ 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)
>
>                     +                if (rte_eth_dev_check_vf_rss_rxq_num(port_id,
>
>                     nb_rx_q) != 0) {
>
>                     +                    PMD_DEBUG_TRACE("ethdev port_id=%d"
>
>                     +                        " SRIOV active, invalid queue"
>
>                     +                        " number for VMDQ RSS\n",
>
>                     +                        port_id);
>
>                   
>
>                 Some nitpicking here: I'd add the allowed values descriptions to the
>
>                 error message. Something like: "invalid queue number for VMDQ RSS.
>
>                 Allowed values are 1, 2 or 4\n".
>
>                   
>
>                     +                    return -EINVAL;
>
>                     +                }
>
>                     +            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 +595,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;
>
>                   
>
>                 I'm not sure u may just remove it. These lines originally belong to a
>
>                 different flow. Are u sure u can remove them like that? What if the
>
>                 mq_mode is ETH_MQ_RX_NONE and nb_q_per_pool has been initialized
>
>             to 4
>
>                 or 8 in ixgbe_pf_host_init()?
>
>               
>
>             I misread the patch - these lines belong to the txmode.mq_mode switch case.
>
>             I think it's ok to remove these really strange lines here. And when I look at it i
>
>             think for the similar reasons the similar lines should be removed in the Rx
>
>             case too: consider non-RSS case with MQ DCB Tx configuration.
>
>               
>
>         I search code in this function, only one place has
>
>         " if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)
>
>                     RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;"
>
>           
>
>         The only place is default branch, which is for rx_none, or vmdq_only mode,
>
>
>     Here is a snippet of an rte_eth_dev_check_mq_mode() from the
>     current master:
>
>                     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 */
>
>                             PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8
>
>                                            " SRIOV active, "
>
>                                            "unsupported VMDQ mq_mode rx %u\n",
>
>                                            port_id, dev_conf->rxmode.mq_mode);
>
>                             return (-EINVAL);
>
>                     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;
>
>                             *if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)                 <---- This is one*
>
>     *                                RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;*
>
>                             break;
>
>                     }
>
>       
>
>                     switch (dev_conf->txmode.mq_mode) {
>
>                     case ETH_MQ_TX_VMDQ_DCB:
>
>                             /* DCB VMDQ in SRIOV mode, not implement yet */
>
>                             PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8
>
>                                            " SRIOV active, "
>
>                                            "unsupported VMDQ mq_mode tx %u\n",
>
>                                            port_id, dev_conf->txmode.mq_mode);
>
>                             return (-EINVAL);
>
>                     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)              <------ This is two. This is what your patch is removing
>
>                                     RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;
>
>                             break;
>
>                     }
>
>
>
>
>     Changchun: yes you are correct, what I mean in my last response is
>     that only one place AFTER my removal, so there are 2 places before
>     my removal.
>     no controversial here.
>
>
>       
>
>     We don't need remove this, as it should assign as 1 because it did use 1 queue per pool.
>
>
>     And why is that? Just because RSS was not enabled? And what if a
>     user wants multiple Tx queues? Mode 1100b of MRQE for instance?
>
>     Changchun: I can explain why I need this change(remove the second
>     place) here,
>
>
> I understood why u needed it in the first place. I just say that for 
> exactly the same reasons u need to remove the "first place" too. ;)
>
> Changchun: then I will try to explain why I can’t remove the first place J
>
> When the rx mode is ETH_MQ_RX_NONE and tx mode is ETH_MQ_TX_NONE,
>
> The function ixgbe_pf_host_init still set the nb_q_per_pool into 2 or 
> 4 or 8 according to max vf num,
>
> (actually at that point, it has no knowledge of what is the rx and tx 
> configuration value, so have to just set
>
> an estimated (and not so accurate) value according to the max vf num)
>
> then in the check_mq_mode function, need further refine this value 
> according to a few factors:
>
> sriov.active, and rxmode.mq_mode.
>
> When it finds the rx mode is RX_NONE, and the nb_q_per_pool is larger 
> than 1, then it should refine to 1.
>
> So if I remove the first place, VMDQ_RSS case works well, but I break 
> the case of RX_NONE.
>
> So I think we can’t treat rx path and tx path in absolutely same way 
> here, i.e. if you add it in the first place(rx path) then you need 
> also add it in the second place(tx path)
>
> Vice versa,
>
> that’s my understanding J
>

And now consider the case when rx_mode == RSS_NONE (since user has 
configured only a single Rx queue) and tx_mode == TX_DCB (user has 
configured 4 Tx queues and requested the above Tx mode). After your 
patch the nb_q_per_pool will still be set to 1 while it should have 
remained 4 because u want a pool to support 4 queues (MRQC.MRQE == 
1010b) but u will configure the PSRTYPE[n].RQPL for this pool to 0.

> Thanks
>
> Changchun
>
Ouyang Changchun Jan. 14, 2015, 12:44 a.m. UTC | #9
From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
Sent: Tuesday, January 13, 2015 5:00 PM
To: Ouyang, Changchun; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode


On 01/13/15 03:50, Ouyang, Changchun wrote:


From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
Sent: Monday, January 12, 2015 9:59 PM
To: Ouyang, Changchun; dev@dpdk.org<mailto:dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode


On 01/12/15 05:41, Ouyang, Changchun wrote:


From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
Sent: Friday, January 09, 2015 9:50 PM
To: Ouyang, Changchun; dev@dpdk.org<mailto:dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v5 4/6] ether: Check VMDq RSS mode


On 01/09/15 07:54, Ouyang, Changchun wrote:





-----Original Message-----

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

Sent: Friday, January 9, 2015 2:49 AM

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

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





On 01/08/15 11:19, Vlad Zolotarov wrote:



On 01/07/15 08:32, 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><mailto:changchun.ouyang@intel.com>



changes in v5:

   - Fix '<' issue, it should be '<=' to test rxq number;

   - Extract a function to remove the embeded switch-case statement.



---

  lib/librte_ether/rte_ethdev.c | 50

++++++++++++++++++++++++++++++++++++++-----

  1 file changed, 45 insertions(+), 5 deletions(-)



diff --git a/lib/librte_ether/rte_ethdev.c

b/lib/librte_ether/rte_ethdev.c index 95f2ceb..8363e26 100644

--- a/lib/librte_ether/rte_ethdev.c

+++ b/lib/librte_ether/rte_ethdev.c

@@ -503,6 +503,31 @@ rte_eth_dev_tx_queue_config(struct

rte_eth_dev

*dev, uint16_t nb_queues)

  }

    static int

+rte_eth_dev_check_vf_rss_rxq_num(uint8_t port_id, uint16_t nb_rx_q)

+{

+    struct rte_eth_dev *dev = &rte_eth_devices[port_id];

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

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

+

+    return 0;

+}

+

+static int

  rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q,

uint16_t nb_tx_q,

                const struct rte_eth_conf *dev_conf)

  {

@@ -510,8 +535,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

+549,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 +557,25 @@ 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)

+                if (rte_eth_dev_check_vf_rss_rxq_num(port_id,

nb_rx_q) != 0) {

+                    PMD_DEBUG_TRACE("ethdev port_id=%d"

+                        " SRIOV active, invalid queue"

+                        " number for VMDQ RSS\n",

+                        port_id);



Some nitpicking here: I'd add the allowed values descriptions to the

error message. Something like: "invalid queue number for VMDQ RSS.

Allowed values are 1, 2 or 4\n".



+                    return -EINVAL;

+                }

+            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 +595,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;



I'm not sure u may just remove it. These lines originally belong to a

different flow. Are u sure u can remove them like that? What if the

mq_mode is ETH_MQ_RX_NONE and nb_q_per_pool has been initialized

to 4

or 8 in ixgbe_pf_host_init()?



I misread the patch - these lines belong to the txmode.mq_mode switch case.

I think it's ok to remove these really strange lines here. And when I look at it i

think for the similar reasons the similar lines should be removed in the Rx

case too: consider non-RSS case with MQ DCB Tx configuration.



I search code in this function, only one place has

" if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)

           RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;"



The only place is default branch, which is for rx_none, or vmdq_only mode,

Here is a snippet of an rte_eth_dev_check_mq_mode() from the current master:

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

                       PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8

                                      " SRIOV active, "

                                      "unsupported VMDQ mq_mode rx %u\n",

                                      port_id, dev_conf->rxmode.mq_mode);

                       return (-EINVAL);

               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;

                       if (RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool > 1)                 <---- This is one

                               RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;

                       break;

               }



               switch (dev_conf->txmode.mq_mode) {

               case ETH_MQ_TX_VMDQ_DCB:

                       /* DCB VMDQ in SRIOV mode, not implement yet */

                       PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8

                                      " SRIOV active, "

                                      "unsupported VMDQ mq_mode tx %u\n",

                                      port_id, dev_conf->txmode.mq_mode);

                       return (-EINVAL);

               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)              <------ This is two. This is what your patch is removing

                               RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;

                       break;

               }




Changchun: yes you are correct, what I mean in my last response is that only one place AFTER my removal, so there are 2 places before my removal.
no controversial here.






We don't need remove this, as it should assign as 1 because it did use 1 queue per pool.

And why is that? Just because RSS was not enabled? And what if a user wants multiple Tx queues? Mode 1100b of MRQE for instance?

Changchun: I can explain why I need this change(remove the second place) here,

   I understood why u needed it in the first place. I just say that for exactly the same reasons u need to remove the "first place" too. ;)



Changchun: then I will try to explain why I can't remove the first place :)
When the rx mode is ETH_MQ_RX_NONE and tx mode is ETH_MQ_TX_NONE,
The function ixgbe_pf_host_init still set the nb_q_per_pool into 2 or 4 or 8 according to max vf num,
(actually at that point, it has no knowledge of what is the rx and tx configuration value, so have to just set
an estimated (and not so accurate) value according to the max vf num)
then in the check_mq_mode function, need further refine this value according to a few factors:
sriov.active, and rxmode.mq_mode.
When it finds the rx mode is RX_NONE, and the nb_q_per_pool is larger than 1, then it should refine to 1.
So if I remove the first place, VMDQ_RSS case works well, but I break the case of RX_NONE.

So I think we can't treat rx path and tx path in absolutely same way here, i.e. if you add it in the first place(rx path) then you need also add it in the second place(tx path)
Vice versa,
that's my understanding :)

  And now consider the case when rx_mode == RSS_NONE (since user has configured only a single Rx queue) and tx_mode == TX_DCB (user has configured 4 Tx queues and requested the above Tx     mode). After your patch the nb_q_per_pool will still be set to 1 while it should have remained 4 because u want a pool to support 4 queues (MRQC.MRQE == 1010b) but u will configure the    PSRTYPE[n].RQPL for this pool to 0.

[Changchun]
As currently vmdq dcb is not supported yet, so it don't consider that case, as vf rss(vmdq rss) concerned, this patch is ok, I think you also agree that, am I right?
Go back to your question, considering your case, with vmdq dcb, you are right,
So as we can see Jastrzebski, MichalX K michalx.k.jastrzebski@intel.com<mailto:michalx.k.jastrzebski@intel.com> resolve this issue in his "add dcb for vf for ixgbe" by split nb_q_per_pool into nb_rx_q_per_pool, and nb_tx_q_per_pool,
I thinks that's good way to do it.

So my opinion is we can discuss this in "dcb for vf for ixgbe", because the question is now switch to dcb for vf, not rss for vf itself,
How do you think of it?
Changchun
diff mbox

Patch

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 95f2ceb..8363e26 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -503,6 +503,31 @@  rte_eth_dev_tx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 }
 
 static int
+rte_eth_dev_check_vf_rss_rxq_num(uint8_t port_id, uint16_t nb_rx_q)
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+	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:
+		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;
+
+	return 0;
+}
+
+static int
 rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		      const struct rte_eth_conf *dev_conf)
 {
@@ -510,8 +535,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 +549,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 +557,25 @@  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)
+				if (rte_eth_dev_check_vf_rss_rxq_num(port_id, nb_rx_q) != 0) {
+					PMD_DEBUG_TRACE("ethdev port_id=%d"
+						" SRIOV active, invalid queue"
+						" number for VMDQ RSS\n",
+						port_id);
+					return -EINVAL;
+				}
+			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 +595,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;
 		}