[dpdk-dev] [PATCH v2] net/ixgbe: support multiqueue mode VMDq DCB with SRIOV

[Iremonger, Bernard]

Hi Wenzhuo,

<snip>

> > > > Subject: RE: [PATCH v2] net/ixgbe: support multiqueue mode VMDq
> > > > DCB with SRIOV
> > > >
> > > > Hi Wenzhuo,
> > > >
> > > > > >  	if (hw->mac.type != ixgbe_mac_82598EB) { @@ -3339,11
> > > > > > +3340,17
> > > > > @@
> > > > > > ixgbe_dcb_tx_hw_config(struct ixgbe_hw *hw,
> > > > > >  		if (dcb_config->vt_mode)
> > > > > >  			reg |= IXGBE_MTQC_VT_ENA;
> > > > > >  		IXGBE_WRITE_REG(hw, IXGBE_MTQC, reg);
> > > > > > -
> > > > > > -		/* Disable drop for all queues */
> > > > > > -		for (q = 0; q < 128; q++)
> > > > > > -			IXGBE_WRITE_REG(hw, IXGBE_QDE,
> > > > > > -				(IXGBE_QDE_WRITE | (q <<
> > > > > > IXGBE_QDE_IDX_SHIFT)));
> > > > > > +		if (RTE_ETH_DEV_SRIOV(dev).active == 0) {
> > > > > > +			/* Disable drop for all queues in VMDQ
> mode*/
> > > > > > +			for (q = 0; q < 128; q++)
> > > > > > +				IXGBE_WRITE_REG(hw, IXGBE_QDE,
> > > > > > +						(IXGBE_QDE_WRITE |
> > (q <<
> > > > > > IXGBE_QDE_IDX_SHIFT) | IXGBE_QDE_ENABLE));
> This code is used to enable drop, but the comments say 'disable drop'.

The comment is correct but the code is not.
 
> > > > > > +		} else {
> > > > > > +			/* Enable drop for all queues in SRIOV mode
> */
> > > > > > +			for (q = 0; q < 128; q++)
> > > > > > +				IXGBE_WRITE_REG(hw, IXGBE_QDE,
> > > > > > +						(IXGBE_QDE_WRITE |
> > (q <<
> > > > > > IXGBE_QDE_IDX_SHIFT)));
> This code is used to disable drop, but the comments say 'enable drop'.

The comment is correct but the code is not.

> I've checked the V4, the same problem too. I think the problem is the
> comments, right?

No, the comments are correct but the code is reversed.
I will send a v5. 
 
 
> > > > > > +		}
> > > > > I think it has nothing to do with mq mode. Do I miss something?
> > > >
> > > > Behavior is different when SRIOV is enabled.
> > > I don't understand why the behavior is different. To my opinion, the
> > > drop has nothing to do with the mode. We can enable or disable it.
> > > The old behavior is disabling it by default. Now you change it to
> > > disabling it by default in NO-SRIOV mode, but enabling it in SRIOV mode.
> > > What I don't get is the reason.
> >
> > Please refer to section 4.6.11.3.1 page 180 of the
> > 82599-10-gbe-controller- datasheet.pdf The last paragraph on page 180
> > states:
> >
> > "Queue Drop Enable (PFQDE) - In SR-IO the QDE bit should be set to 1b
> > in the PFQDE register for all queues.
> > In VMDq mode, the QDE bit should be set to 0b for all queues."
> Got it, thanks a lot :)
> 
> >
> >
> > > > > >  		/* Enable the Tx desc arbiter */
> > > > > >  		reg = IXGBE_READ_REG(hw, IXGBE_RTTDCS); @@ -
> 3378,7
> > > > > > +3385,7 @@ ixgbe_vmdq_dcb_hw_tx_config(struct rte_eth_dev
> > > > > > +*dev,
> > > > > >  			vmdq_tx_conf->nb_queue_pools ==
> ETH_16_POOLS
> > > > > ?
> > > > > > 0xFFFF : 0xFFFFFFFF);
> > > > > >
> > > > > >  	/*Configure general DCB TX parameters*/
> > > > > > -	ixgbe_dcb_tx_hw_config(hw, dcb_config);
> > > > > > +	ixgbe_dcb_tx_hw_config(dev, dcb_config);
> > > > > >  }
> > > > > >
> > > > > >  static void
> > > > > > @@ -3661,7 +3668,7 @@ ixgbe_dcb_hw_configure(struct
> > > > > > rte_eth_dev
> > > > > *dev,
> > > > > >  		/*get DCB TX configuration parameters from
> rte_eth_conf*/
> > > > > >  		ixgbe_dcb_tx_config(dev, dcb_config);
> > > > > >  		/*Configure general DCB TX parameters*/
> > > > > > -		ixgbe_dcb_tx_hw_config(hw, dcb_config);
> > > > > > +		ixgbe_dcb_tx_hw_config(dev, dcb_config);
> > > > > >  		break;
> > > > > >  	default:
> > > > > >  		PMD_INIT_LOG(ERR, "Incorrect DCB TX mode
> > > > > configuration"); @@
> > > > > > -3810,9 +3817,6 @@ void ixgbe_configure_dcb(struct rte_eth_dev
> > > *dev)
> > > > > >  	    (dev_conf->rxmode.mq_mode != ETH_MQ_RX_DCB_RSS))
> > > > > >  		return;
> > > > > >
> > > > > > -	if (dev->data->nb_rx_queues != ETH_DCB_NUM_QUEUES)
> > > > > > -		return;
> > > > > I remember it's a limitation of implementation. The reason is
> > > > > the resource allocation. Why could we remove it now?
> > > >
> > > > ETH_DCB_NUM_QUEUES is 128,  nb_rx_queues may not be 128.
> > > I think it's a limitation to force the queue number to be
> > > ETH_DCB_NUM_QUEUES.
> > > Just to confirm it, have you try to set rx queue number to something
> > > different from 128, like 64, 32...
> >
> > In my test scenario the nb_rx_queues is 1.
> Glad to know that, thanks :)
> 

Regards,

Bernard