Virtualization - May 2023 - [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

On Sat, May 06, 2023 at 04:56:35PM +0800, Hao Chen
wrote:
> 
> 
> ? 2023/5/6 10:50, Xuan Zhuo ??:
> > On Sat,  6 May 2023 10:15:29 +0800, Hao Chen <chenh at
yusur.tech> wrote:
> > > When VIRTIO_NET_F_MTU(3) Device maximum MTU reporting is
supported.
> > > If offered by the device, device advises driver about the value
of its
> > > maximum MTU. If negotiated, the driver uses mtu as the maximum
> > > MTU value. But there the driver also uses it as default mtu,
> > > some devices may have a maximum MTU greater than 1500, this may
> > > cause some large packages to be discarded,
> > 
> > You mean tx packet?
> Yes.
> > 
> > If yes, I do not think this is the problem of driver.
> > 
> > Maybe you should give more details about the discard.
> > 
> In the current code, if the maximum MTU supported by the virtio net
hardware
> is 9000, the default MTU of the virtio net driver will also be set to 9000.
> When sending packets through "ping -s 5000", if the peer router
does not
> support negotiating a path MTU through ICMP packets, the packets will be
> discarded. If the peer router supports negotiating path mtu through ICMP
> packets, the host side will perform packet sharding processing based on the
> negotiated path mtu, which is generally within 1500.
> This is not a bugfix patch, I think setting the default mtu to within 1500
> would be more suitable here.Thanks.
I don't think VIRTIO_NET_F_MTU is appropriate for support for jumbo packets.
The spec says:
	The device MUST forward transmitted packets of up to mtu (plus low level
ethernet header length) size with
	gso_type NONE or ECN, and do so without fragmentation, after VIRTIO_NET_F_MTU
has been success-
	fully negotiated.
VIRTIO_NET_F_MTU has been designed for all kind of tunneling devices,
and this is why we set mtu to max by default.

For things like jumbo frames where MTU might or might not be available,
a new feature would be more appropriate.
> > > so I changed the MTU to a more
> > > general 1500 when 'Device maximum MTU' bigger than 1500.
> > > 
> > > Signed-off-by: Hao Chen <chenh at yusur.tech>
> > > ---
> > >   drivers/net/virtio_net.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 8d8038538fc4..e71c7d1b5f29 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -4040,7 +4040,10 @@ static int virtnet_probe(struct
virtio_device *vdev)
> > >   			goto free;
> > >   		}
> > > 
> > > -		dev->mtu = mtu;
> > > +		if (mtu > 1500)
> > 
> > s/1500/ETH_DATA_LEN/
> > 
> > Thanks.
> > 
> > > +			dev->mtu = 1500;
> > > +		else
> > > +			dev->mtu = mtu;
> > >   		dev->max_mtu = mtu;
> > >   	}
> > > 
> > > --
> > > 2.27.0
> > >

On Sun, 7 May 2023 04:58:58 -0400, "Michael S. Tsirkin" <mst at
redhat.com> wrote:
> On Sat, May 06, 2023 at 04:56:35PM +0800, Hao Chen wrote:
> >
> >
> > ? 2023/5/6 10:50, Xuan Zhuo ??:
> > > On Sat,  6 May 2023 10:15:29 +0800, Hao Chen <chenh at
yusur.tech> wrote:
> > > > When VIRTIO_NET_F_MTU(3) Device maximum MTU reporting is
supported.
> > > > If offered by the device, device advises driver about the
value of its
> > > > maximum MTU. If negotiated, the driver uses mtu as the
maximum
> > > > MTU value. But there the driver also uses it as default mtu,
> > > > some devices may have a maximum MTU greater than 1500, this
may
> > > > cause some large packages to be discarded,
> > >
> > > You mean tx packet?
> > Yes.
> > >
> > > If yes, I do not think this is the problem of driver.
> > >
> > > Maybe you should give more details about the discard.
> > >
> > In the current code, if the maximum MTU supported by the virtio net
hardware
> > is 9000, the default MTU of the virtio net driver will also be set to
9000.
> > When sending packets through "ping -s 5000", if the peer
router does not
> > support negotiating a path MTU through ICMP packets, the packets will
be
> > discarded. If the peer router supports negotiating path mtu through
ICMP
> > packets, the host side will perform packet sharding processing based
on the
> > negotiated path mtu, which is generally within 1500.
> > This is not a bugfix patch, I think setting the default mtu to within
1500
> > would be more suitable here.Thanks.
>
> I don't think VIRTIO_NET_F_MTU is appropriate for support for jumbo
packets.
> The spec says:
> 	The device MUST forward transmitted packets of up to mtu (plus low level
ethernet header length) size with
> 	gso_type NONE or ECN, and do so without fragmentation, after
VIRTIO_NET_F_MTU has been success-
> 	fully negotiated.
> VIRTIO_NET_F_MTU has been designed for all kind of tunneling devices,
> and this is why we set mtu to max by default.
>
> For things like jumbo frames where MTU might or might not be available,
> a new feature would be more appropriate.

So for jumbo frame, what is the problem?

We are trying to do this. @Heng

Thanks.

>
> > > > so I changed the MTU to a more
> > > > general 1500 when 'Device maximum MTU' bigger than
1500.
> > > >
> > > > Signed-off-by: Hao Chen <chenh at yusur.tech>
> > > > ---
> > > >   drivers/net/virtio_net.c | 5 ++++-
> > > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c
b/drivers/net/virtio_net.c
> > > > index 8d8038538fc4..e71c7d1b5f29 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -4040,7 +4040,10 @@ static int virtnet_probe(struct
virtio_device *vdev)
> > > >   			goto free;
> > > >   		}
> > > >
> > > > -		dev->mtu = mtu;
> > > > +		if (mtu > 1500)
> > >
> > > s/1500/ETH_DATA_LEN/
> > >
> > > Thanks.
> > >
> > > > +			dev->mtu = 1500;
> > > > +		else
> > > > +			dev->mtu = mtu;
> > > >   		dev->max_mtu = mtu;
> > > >   	}
> > > >
> > > > --
> > > > 2.27.0
> > > >
>