Re: [PATCH net-next] net: mvpp2: increase MTU limit when XDP enabled

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Tue, 5 Jan 2021 18:43:08 +0100
Marek Behún <kabel@kernel.org> wrote:

> On Tue, 5 Jan 2021 18:24:37 +0100
> Sven Auhagen <sven.auhagen@voleatech.de> wrote:
> 
> > On Tue, Jan 05, 2021 at 06:19:21PM +0100, Marek Behún wrote:  
> > > Currently mvpp2_xdp_setup won't allow attaching XDP program if
> > >   mtu > ETH_DATA_LEN (1500).
> > > 
> > > The mvpp2_change_mtu on the other hand checks whether
> > >   MVPP2_RX_PKT_SIZE(mtu) > MVPP2_BM_LONG_PKT_SIZE.
> > > 
> > > These two checks are semantically different.
> > > 
> > > Moreover this limit can be increased to MVPP2_MAX_RX_BUF_SIZE, since in
> > > mvpp2_rx we have
> > >   xdp.data = data + MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM;
> > >   xdp.frame_sz = PAGE_SIZE;
> > > 
> > > Change the checks to check whether
> > >   mtu > MVPP2_MAX_RX_BUF_SIZE    
> > 
> > Hello Marek,
> > 
> > in general, XDP is based on the model, that packets are not bigger
> > than 1500.

This is WRONG.

The XDP design/model (with PAGE_SIZE 4096) allows MTU to be 3506 bytes.

This comes from:
 * 4096 = frame_sz = PAGE_SIZE
 * -256 = reserved XDP_PACKET_HEADROOM
 * -320 = reserved tailroom with sizeof(skb_shared_info)
 * - 14 = Ethernet header size as MTU value is L3

4096-256-320-14 = 3506 bytes

Depending on driver memory layout choices this can (of-cause) be lower.

> > I am not sure if that has changed, I don't believe Jumbo Frames are
> > upstreamed yet.

This is unrelated to this patch, but Lorenzo and Eelco is assigned to
work on this.

> > You are correct that the MVPP2 driver can handle bigger packets
> > without a problem but if you do XDP redirect that won't work with
> > other drivers and your packets will disappear.  
> 

This statement is too harsh.  The XDP layer will not do (IP-level)
fragmentation for you.  Thus, if you redirect/transmit frames out
another interface with lower MTU than the frame packet size then the
packet will of-cause be dropped (the drop counter is unfortunately not
well defined).  This is pretty standard behavior.

This is why I'm proposing the BPF-helper bpf_check_mtu().  To allow the
BPF-prog to check the MTU before doing the redirect.


> At least 1508 is required when I want to use XDP with a Marvell DSA
> switch: the DSA header is 4 or 8 bytes long there.
> 
> The DSA driver increases MTU on CPU switch interface by this length
> (on my switches to 1504).
> 
> So without this I cannot use XDP with mvpp2 with a Marvell switch with
> default settings, which I think is not OK.
> 
> Since with the mvneta driver it works (mvneta checks for
> MVNETA_MAX_RX_BUF_SIZE rather than ETH_DATA_LEN), I think it should also work
> with mvpp2.

I think you patch makes perfect sense.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer