Re: [dpdk-dev] [PATCH 1/2] pmd: add DCB for VF for ixgbe

["Wodkowski, PawelX" <pawelx.wodkowski@intel.com>]

> > >
> > >   - split nb_q_per_pool to nb_rx_q_per_pool and nb_tx_q_per_pool
> > >
> > >     Rationale:
> > >
> > >     rx and tx number of queue might be different if RX and TX are
> > >
> > >     configured in different mode. This allow to inform VF about
> > >
> > >     proper number of queues.
> >
> >
> > Nice move! Ouyang, this is a nice answer to my recent remarks about your
> > PATCH4 in "Enable VF RSS for Niantic" series.
> 
> After I respond your last comments, I see this,  :-), I am sure we both agree it is
> the right way to resolve it in vmdq dcb case.
> 

I am now dividing this patch with your suggestions and I am little confused.

In this (DCB in SRIOV) case the primary cause for spliting nb_q_per_pool into
nb_rx_q_per_pool and nb_tx_q_per_pool was because of this code:

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index af9e261..be3afe4 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -537,8 +537,8 @@
 		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)
-				RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;
+			if (RTE_ETH_DEV_SRIOV(dev).nb_rx_q_per_pool > 1)
+				RTE_ETH_DEV_SRIOV(dev).nb_rx_q_per_pool = 1;
 			break;
 		}
 
@@ -553,17 +553,18 @@
 		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;
+			if (RTE_ETH_DEV_SRIOV(dev).nb_tx_q_per_pool > 1)
+				RTE_ETH_DEV_SRIOV(dev).nb_tx_q_per_pool = 1;
 			break;
 		}
 
 		/* check valid queue number */
-		if ((nb_rx_q > RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool) ||
-		    (nb_tx_q > RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool)) {
+		if ((nb_rx_q > RTE_ETH_DEV_SRIOV(dev).nb_rx_q_per_pool) ||
+		    (nb_tx_q > RTE_ETH_DEV_SRIOV(dev).nb_tx_q_per_pool)) {
 			PMD_DEBUG_TRACE("ethdev port_id=%d SRIOV active, "
-				    "queue number must less equal to %d\n",
-					port_id, RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool);
+				    "rx/tx queue number must less equal to %d/%d\n",
+					port_id, RTE_ETH_DEV_SRIOV(dev).nb_rx_q_per_pool,
+					RTE_ETH_DEV_SRIOV(dev).nb_tx_q_per_pool);
 			return (-EINVAL);
 		}
 	} else {
--

This introduced an issue when RX and TX was configure in different way. The problem was
that the RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool as common for RX and TX and it is
changed. So I did the above. But when testpmd was adjusted for DCB in SRIOV there 
was another issue. Testpmd is pre-configuring ports by default and since
nb_rx_q_per_pool  and nb_tx_q_per_pool was already reset to 1 there was no way to 
use it for DCB in SRIOV. So I did another modification:

> +		uint16_t nb_rx_q_per_pool = RTE_ETH_DEV_SRIOV(dev).nb_rx_q_per_pool;
> +		uint16_t nb_tx_q_per_pool = RTE_ETH_DEV_SRIOV(dev).nb_tx_q_per_pool;
> +
>   		switch (dev_conf->rxmode.mq_mode) {
> -		case ETH_MQ_RX_VMDQ_RSS:
>   		case ETH_MQ_RX_VMDQ_DCB:
> +			break;
> +		case ETH_MQ_RX_VMDQ_RSS:
>   		case ETH_MQ_RX_VMDQ_DCB_RSS:
> -			/* DCB/RSS VMDQ in SRIOV mode, not implement yet */
> +			/* 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",
> @@ -537,37 +560,32 @@ rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   		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)
> -				RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = 1;
> +			if (nb_rx_q_per_pool > 1)
> +				nb_rx_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);
> +		case ETH_MQ_TX_VMDQ_DCB: /* DCB VMDQ in SRIOV mode*/
> +			break;
>   		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;
> +			if (nb_tx_q_per_pool > 1)
> +				nb_tx_q_per_pool = 1;
>   			break;
>   		}
>   
>   		/* check valid queue number */
> -		if ((nb_rx_q > RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool) ||
> -		    (nb_tx_q > RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool)) {
> +		if (nb_rx_q > nb_rx_q_per_pool || nb_tx_q > nb_tx_q_per_pool) {
>   			PMD_DEBUG_TRACE("ethdev port_id=%d SRIOV active, "
> -				    "queue number must less equal to %d\n",
> -					port_id, RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool);
> +				    "rx/tx queue number must less equal to %d/%d\n",
> +					port_id, RTE_ETH_DEV_SRIOV(dev).nb_rx_q_per_pool,
> +					RTE_ETH_DEV_SRIOV(dev).nb_tx_q_per_pool);
>   			return (-EINVAL);
>   		}

For this point I think that splitting RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool might be not
needed. From my point  of view (DCB), since nb_q_per_pool is untouched, I think I can stay with:

> +		uint16_t nb_rx_q_per_pool = RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool;
> +		uint16_t nb_tx_q_per_pool = RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool;
> +

What do you think? I noticed that you was discussing some issue about nb_q_per_pool in face
of RSS functionality. Can you spoke about my doubts in face of that RSS?

Pawel