Re: [net-next v2 9/9] ice: allow 3k MTU for XDP

[Maciej Fijalkowski <maciejromanfijalkowski@gmail.com>]

On Tue, 5 Nov 2019 13:37:23 +0100
Jesper Dangaard Brouer <brouer@redhat.com> wrote:

> On Mon,  4 Nov 2019 13:51:25 -0800
> Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
> 
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > 
> > At this point ice driver is able to work on order 1 pages that are split
> > onto two 3k buffers. Let's reflect that when user is setting new MTU
> > size and XDP is present on interface.
> > 
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_main.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> > index 29eea08807fd..363b284e8aa1 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > @@ -4658,6 +4658,18 @@ static void ice_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
> >  	dev_err(dev, "Rebuild failed, unload and reload driver\n");
> >  }
> >  
> > +/**
> > + * ice_max_xdp_frame_size - returns the maximum allowed frame size for XDP
> > + * @vsi: Pointer to VSI structure
> > + */
> > +static int ice_max_xdp_frame_size(struct ice_vsi *vsi)
> > +{
> > +	if (PAGE_SIZE >= 8192 || test_bit(ICE_FLAG_LEGACY_RX, vsi->back->flags))
> > +		return ICE_RXBUF_2048 - XDP_PACKET_HEADROOM;  
> 
> I've not checked the details of the ICE drivers memory model, are you
> using a split-page model?

Yes.

> 
> If so, in case of ICE_FLAG_LEGACY_RX and PAGE_SIZE==4096, then other
> Intel drivers use headroom size 192 bytes and not
> XDP_PACKET_HEADROOM=256, because it doesn't fit with split-page model.

That's not quite right.

What mostly ICE_FLAG_LEGACY_RX does is that it indicates whether we're using
build_skb() or not.

If !ICE_FLAG_LEGACY_RX && PAGE_SIZE==4096, we provide a 192 byte headroom and
320 byte tailroom dedicated for skb_shared_info in order to support the
build_skb(). We can piggy-back on that headroom for XDP purposes, which we're
currently doing in intel drivers.

Otherwise, the legacy Rx flow doesn't provide *any* headroom/tailroom, so to
satisfy the XDP headroom requirement, it needs to be explicitly taken into
account, which is what I'm trying to address in this series, see
ice_rx_offset()@[1].

Seems that i40e is not doing it and I suppose it is broken for case where XDP
prog is enlarging the frame and legacy Rx path is taken. I can later submit
small set with other two issues that came up from Jakub's review on first
revision of this set.

> 
> Asked in another way: Have you taking into account the 320 bytes needed
> by skb_shared_info ?

For legacy Rx there's no particular need for it as __napi_alloc_skb() is
handling it for us (note that legacy Rx implies ice_construct_skb() call where
we get the skb allocated via already mentioned __napi_alloc_skb() and then
memcpy the actual frame contents onto this skb).

Thanks,
Maciej

[1] :
https://lore.kernel.org/netdev/20191104215125.16745-8-jeffrey.t.kirsher@intel.com/

> 
> 
> > +	else
> > +		return ICE_RXBUF_3072;
> > +}
> > +
> >  /**
> >   * ice_change_mtu - NDO callback to change the MTU
> >   * @netdev: network interface device structure
> > @@ -4678,11 +4690,11 @@ static int ice_change_mtu(struct net_device *netdev, int new_mtu)
> >  	}
> >  
> >  	if (ice_is_xdp_ena_vsi(vsi)) {
> > -		int frame_size = ICE_RXBUF_2048 - XDP_PACKET_HEADROOM;
> > +		int frame_size = ice_max_xdp_frame_size(vsi);
> >  
> >  		if (new_mtu + ICE_ETH_PKT_HDR_PAD > frame_size) {
> >  			netdev_err(netdev, "max MTU for XDP usage is %d\n",
> > -				   frame_size);
> > +				   frame_size - ICE_ETH_PKT_HDR_PAD);
> >  			return -EINVAL;
> >  		}
> >  	}  
> 
> 
>