Re: [dpdk-dev] Couple of PMD questions

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Apr 19, 2016 at 03:16:37PM -0500, Jay Rolette wrote:
> In ixgbe_dev_rx_init(), there is this bit of code:
> 
> 	/*
> 	 * Configure the RX buffer size in the BSIZEPACKET field of
> 	 * the SRRCTL register of the queue.
> 	 * The value is in 1 KB resolution. Valid values can be from
> 	 * 1 KB to 16 KB.
> 	 */
> 	buf_size = (uint16_t)(rte_pktmbuf_data_room_size(rxq->mb_pool) -
> 		RTE_PKTMBUF_HEADROOM);
> 	srrctl |= ((buf_size >> IXGBE_SRRCTL_BSIZEPKT_SHIFT) &
> 		   IXGBE_SRRCTL_BSIZEPKT_MASK);
> 
> 	IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(rxq->reg_idx), srrctl);
> 
> 	buf_size = (uint16_t) ((srrctl & IXGBE_SRRCTL_BSIZEPKT_MASK) <<
> 			       IXGBE_SRRCTL_BSIZEPKT_SHIFT);
> 
> 	/* It adds dual VLAN length for supporting dual VLAN */
> 	if (dev->data->dev_conf.rxmode.max_rx_pkt_len +
> 				    2 * IXGBE_VLAN_TAG_SIZE > buf_size)
> 		dev->data->scattered_rx = 1;
> 
> 
> Couple of questions I have about it:
> 
> 1) If the caller configured the MBUF memory pool data room size to be
> something other than a multiple of 1K (+ RTE_PKTMBUF_HEADROOM), then the
> driver ends up silently programming the NIC to use a smaller max RX size
> than the caller specified.
> 
> Should the driver error out in that case instead of only "sort of" working?
> If not, it seems like it should be logging an error message.

Well, it does log at the end of the whole rx init process what RX function is
being used, so there is that.
However, looking at it now, given that there is an explicit setting in the config
to request scattered mode, I think you may be right and that we should error out
here if scattered mode is needed and not set. It could help prevent unexpected
problems with these drivers.

> 
> 2) Second question is about the "/* It adds dual VLAN length for supporting
> dual VLAN */" bit...
> 
> As I understand it, dev_conf.rxmode.max_rx_pkt_len is supposed to be set to
> the max frame size you support (although it says it's only used if jumbo
> frame is enabled). That same value is generally used when calculating the
> size that mbuf elements should be created at.
> 
> The description for the data_room_size parameter of
> rte_pktmbuf_pool_create():
> 
> "Size of data buffer in each mbuf, including RTE_PKTMBUF_HEADROOM."
> 
> 
> If I support a max frame size of 9216 bytes (exactly a 1K multiple to make
> the NIC happy), then max_rx_pkt_len is going to be 9216 and data_room_size
> will be 9216 + RTE_PKTMBUF_HEADROOM.
> 
> ixgbe_dev_rx_init() will calculate normalize that back to 9216, which will
> fail the dual VLAN length check. The really nasty part about that is it has
> a side-effect of enabling scattered RX regardless of the fact that I didn't
> enable scattered RX in dev_conf.rxmode.
> 
> The code in the e1000 PMD is similar, so nothing unique to ixgbe.
> 
> Is that check correct? It seems wrong to be adding space for q-in-q on top
> of your specified max frame size...

The issue here is what the correct behaviour needs to be. If we have the user
specify the maximum frame size including all vlan tags, then we hit the problem
where we have to subtract the VLAN tag sizes when writing the value to the NIC.
In that case, we will hit a problem where we get a e.g. 9210 byte frame - to 
reuse your example - without any VLAN tags, which will be rejected by the
hardware as being oversized. If we don't do the subtraction, and we get the
same 9210 byte packet with 2 VLAN tags on it, the hardware will accept it and
then split it across multiple descriptors because the actual DMA size is
9218 bytes.

I'm not sure there is a works-in-all-cases solution here.

/Bruce

> 
> Thanks,
> Jay