Re: [PATCH 3/5] ethdev: add mtu accessors

["Ananyev, Konstantin" <konstantin.ananyev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

Hi David,

>This patch adds two new functions in ethdev api to retrieve current MTU and
>change MTU of a port.
>These operations have been implemented for rte_em_pmd, rte_igb_pmd and
>rte_ixgbe_pmd.

>+
>+void ixgbe_dev_set_rx_scatter_mode(struct rte_eth_dev *dev)
>+{
>+	if (dev->rx_pkt_burst != ixgbe_recv_scattered_pkts) {
>+		dev->rx_pkt_burst = ixgbe_recv_scattered_pkts;
>+		/* we must ensure that this is done when we leave the
>+		   function */
>+		rte_wmb();
>+	}
>+}

I don't think it is safe to change RX function on the fly (without calling dev_stop first).
If before that ixgbe_recv_pkts_bulk_alloc() was used, then up to 32 mbufs could be stored internally in the stage[],
and ixgbe_recv_scattered_pkts() doesn't have a clue about them.
Also with ixgbe_recv_pkts_bulk_alloc(), it could be up to rx_free_thresh RX descriptors not armed yet with proper buffer addresses,
while ixgbe_recv_scattered_pkts()  doesn't expect that - it re-arms RXD straight after it receives a packet from it.

I wonder is the ability to change mtu on the fly (without dev_stop) is really needed?
If so, then we probably can allow ixgbe_dev_set_mtu() to increase MTU only if
new frame size is less than RX buffer size OR that device already using  ixgbe_recv_scattered_pkts().
Something like:
frame_size <= rx_buf_size || dev->rx_pkt_burst == ixgbe_recv_scattered_pkts


> +static int
>+ixgbe_dev_set_mtu(struct rte_eth_dev *dev, uint16_t *mtu)
>+{
>....
>+
>+	/* switch to jumbo mode if needed */
>+	if (frame_size  > ETHER_MAX_LEN) {

Why ETHER_MAX_LEN?
If we have rx_buffer big enough, we don't need to split packets even for jumbo frames. 
Shouldn't it be something like inside ixgbe_dev_rx_init():
If (mtu + 2 * IXGBE_VLAN_TAG_SIZE) > rx_buf_size) {...

Same thing for igb.

Konstantin