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

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

[Hao Chen]

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, so I changed the MTU to a more
general 1500 when 'Device maximum MTU' bigger than 1500.

Signed-off-by: Hao Chen <chenh@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)
+			dev->mtu = 1500;
+		else
+			dev->mtu = mtu;
 		dev->max_mtu = mtu;
 	}
 
-- 
2.27.0

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Xuan Zhuo]

On Sat,  6 May 2023 10:15:29 +0800, Hao Chen <chenh@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?

If yes, I do not think this is the problem of driver.

Maybe you should give more details about the discard.

> so I changed the MTU to a more
> general 1500 when 'Device maximum MTU' bigger than 1500.
>
> Signed-off-by: Hao Chen <chenh@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
>

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Hao Chen]

在 2023/5/6 10:50, Xuan Zhuo 写道:
> On Sat,  6 May 2023 10:15:29 +0800, Hao Chen <chenh@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.
>> so I changed the MTU to a more
>> general 1500 when 'Device maximum MTU' bigger than 1500.
>>
>> Signed-off-by: Hao Chen <chenh@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
>>

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Michael S. Tsirkin]

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@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@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
> > > 

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Xuan Zhuo]

On Sun, 7 May 2023 04:58:58 -0400, "Michael S. Tsirkin" <mst@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@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@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
> > > >
>

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Michael S. Tsirkin]

On Mon, May 08, 2023 at 10:01:59AM +0800, Xuan Zhuo wrote:
> On Sun, 7 May 2023 04:58:58 -0400, "Michael S. Tsirkin" <mst@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@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.

It is not a problem as such. But VIRTIO_NET_F_MTU will set the
default MTU not just the maximum one, because spec seems to
say it can.


> 
> >
> > > > > so I changed the MTU to a more
> > > > > general 1500 when 'Device maximum MTU' bigger than 1500.
> > > > >
> > > > > Signed-off-by: Hao Chen <chenh@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
> > > > >
> >

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Xuan Zhuo]

On Mon, 8 May 2023 02:15:46 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, May 08, 2023 at 10:01:59AM +0800, Xuan Zhuo wrote:
> > On Sun, 7 May 2023 04:58:58 -0400, "Michael S. Tsirkin" <mst@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@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.
>
> It is not a problem as such. But VIRTIO_NET_F_MTU will set the
> default MTU not just the maximum one, because spec seems to
> say it can.

I see.

In the case of Jumbo Frame, we also hope that the driver will set the default
directly to the max mtu. Just like what you said "Bigger packets = better
performance."

I don't know, in any scenario, when the hardware supports a large mtu, but we do
not want the user to use it by default. Of course, the scene that this patch
wants to handle does exist, but I have never thought that this is a problem at
the driver level.

Thanks.


>
>
> >
> > >
> > > > > > so I changed the MTU to a more
> > > > > > general 1500 when 'Device maximum MTU' bigger than 1500.
> > > > > >
> > > > > > Signed-off-by: Hao Chen <chenh@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
> > > > > >
> > >
>

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Michael S. Tsirkin]

On Mon, May 08, 2023 at 02:18:08PM +0800, Xuan Zhuo wrote:
> On Mon, 8 May 2023 02:15:46 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Mon, May 08, 2023 at 10:01:59AM +0800, Xuan Zhuo wrote:
> > > On Sun, 7 May 2023 04:58:58 -0400, "Michael S. Tsirkin" <mst@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@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.
> >
> > It is not a problem as such. But VIRTIO_NET_F_MTU will set the
> > default MTU not just the maximum one, because spec seems to
> > say it can.
> 
> I see.
> 
> In the case of Jumbo Frame, we also hope that the driver will set the default
> directly to the max mtu. Just like what you said "Bigger packets = better
> performance."
> 
> I don't know, in any scenario, when the hardware supports a large mtu, but we do
> not want the user to use it by default.

When other devices on the same LAN have mtu set to 1500 and
won't accept bigger packets.

> Of course, the scene that this patch
> wants to handle does exist, but I have never thought that this is a problem at
> the driver level.
> 
> Thanks.
> 
> 
> >
> >
> > >
> > > >
> > > > > > > so I changed the MTU to a more
> > > > > > > general 1500 when 'Device maximum MTU' bigger than 1500.
> > > > > > >
> > > > > > > Signed-off-by: Hao Chen <chenh@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
> > > > > > >
> > > >
> >

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Xuan Zhuo]

On Mon, 8 May 2023 02:43:24 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, May 08, 2023 at 02:18:08PM +0800, Xuan Zhuo wrote:
> > On Mon, 8 May 2023 02:15:46 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Mon, May 08, 2023 at 10:01:59AM +0800, Xuan Zhuo wrote:
> > > > On Sun, 7 May 2023 04:58:58 -0400, "Michael S. Tsirkin" <mst@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@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.
> > >
> > > It is not a problem as such. But VIRTIO_NET_F_MTU will set the
> > > default MTU not just the maximum one, because spec seems to
> > > say it can.
> >
> > I see.
> >
> > In the case of Jumbo Frame, we also hope that the driver will set the default
> > directly to the max mtu. Just like what you said "Bigger packets = better
> > performance."
> >
> > I don't know, in any scenario, when the hardware supports a large mtu, but we do
> > not want the user to use it by default.
>
> When other devices on the same LAN have mtu set to 1500 and
> won't accept bigger packets.

So, that depends on pmtu/tcp-probe-mtu.

If the os without pmtu/tcp-probe-mtu has a bigger mtu, then it's big packet
will lost.

Thanks.


>
> > Of course, the scene that this patch
> > wants to handle does exist, but I have never thought that this is a problem at
> > the driver level.
> >
> > Thanks.
> >
> >
> > >
> > >
> > > >
> > > > >
> > > > > > > > so I changed the MTU to a more
> > > > > > > > general 1500 when 'Device maximum MTU' bigger than 1500.
> > > > > > > >
> > > > > > > > Signed-off-by: Hao Chen <chenh@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
> > > > > > > >
> > > > >
> > >
>

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Michael S. Tsirkin]

On Mon, May 08, 2023 at 03:41:56PM +0800, Xuan Zhuo wrote:
> On Mon, 8 May 2023 02:43:24 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Mon, May 08, 2023 at 02:18:08PM +0800, Xuan Zhuo wrote:
> > > On Mon, 8 May 2023 02:15:46 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > On Mon, May 08, 2023 at 10:01:59AM +0800, Xuan Zhuo wrote:
> > > > > On Sun, 7 May 2023 04:58:58 -0400, "Michael S. Tsirkin" <mst@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@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.
> > > >
> > > > It is not a problem as such. But VIRTIO_NET_F_MTU will set the
> > > > default MTU not just the maximum one, because spec seems to
> > > > say it can.
> > >
> > > I see.
> > >
> > > In the case of Jumbo Frame, we also hope that the driver will set the default
> > > directly to the max mtu. Just like what you said "Bigger packets = better
> > > performance."
> > >
> > > I don't know, in any scenario, when the hardware supports a large mtu, but we do
> > > not want the user to use it by default.
> >
> > When other devices on the same LAN have mtu set to 1500 and
> > won't accept bigger packets.
> 
> So, that depends on pmtu/tcp-probe-mtu.
> 
> If the os without pmtu/tcp-probe-mtu has a bigger mtu, then it's big packet
> will lost.
> 
> Thanks.
> 

pmtu is designed for routing. LAN is supposed to be configured with
a consistent MTU.

> >
> > > Of course, the scene that this patch
> > > wants to handle does exist, but I have never thought that this is a problem at
> > > the driver level.
> > >
> > > Thanks.
> > >
> > >
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > > > > so I changed the MTU to a more
> > > > > > > > > general 1500 when 'Device maximum MTU' bigger than 1500.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Hao Chen <chenh@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
> > > > > > > > >
> > > > > >
> > > >
> >

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Stephen Hemminger]

On Mon, 8 May 2023 06:30:07 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> > > > I don't know, in any scenario, when the hardware supports a large mtu, but we do
> > > > not want the user to use it by default.  
> > >
> > > When other devices on the same LAN have mtu set to 1500 and
> > > won't accept bigger packets.  
> > 
> > So, that depends on pmtu/tcp-probe-mtu.
> > 
> > If the os without pmtu/tcp-probe-mtu has a bigger mtu, then it's big packet
> > will lost.
> > 
> > Thanks.
> >   
> 
> pmtu is designed for routing. LAN is supposed to be configured with
> a consistent MTU.

Virtio is often used with bridging or macvlan which can't support PMTU.
PMTU only works when forwarding at layer 3 (ie routing) where there is
a IP address to send the ICMP response. If doing L2 forwarding, the
only thin the bridge can do is drop the packet.

TCP cab recover but detecting an MTU blackhole requires retransmissions.

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Michael S. Tsirkin]

On Mon, May 08, 2023 at 09:25:48AM -0700, Stephen Hemminger wrote:
> On Mon, 8 May 2023 06:30:07 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > > > I don't know, in any scenario, when the hardware supports a large mtu, but we do
> > > > > not want the user to use it by default.  
> > > >
> > > > When other devices on the same LAN have mtu set to 1500 and
> > > > won't accept bigger packets.  
> > > 
> > > So, that depends on pmtu/tcp-probe-mtu.
> > > 
> > > If the os without pmtu/tcp-probe-mtu has a bigger mtu, then it's big packet
> > > will lost.
> > > 
> > > Thanks.
> > >   
> > 
> > pmtu is designed for routing. LAN is supposed to be configured with
> > a consistent MTU.
> 
> Virtio is often used with bridging or macvlan which can't support PMTU.
> PMTU only works when forwarding at layer 3 (ie routing) where there is
> a IP address to send the ICMP response. If doing L2 forwarding, the
> only thin the bridge can do is drop the packet.
> 
> TCP cab recover but detecting an MTU blackhole requires retransmissions.

Exactly. That's why we basically use the MTU advice supplied by device
by default - it's designed for use-cases of software devices where
the device might have more information about the MTU than the guest.
If hardware devices want e.g. a way to communicate support for
jumbo frames without communicating any information about the LAN,
a new feature will be needed.

-- 
MST

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Xuan Zhuo]

On Mon, 8 May 2023 14:10:07 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, May 08, 2023 at 09:25:48AM -0700, Stephen Hemminger wrote:
> > On Mon, 8 May 2023 06:30:07 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > > > > I don't know, in any scenario, when the hardware supports a large mtu, but we do
> > > > > > not want the user to use it by default.
> > > > >
> > > > > When other devices on the same LAN have mtu set to 1500 and
> > > > > won't accept bigger packets.
> > > >
> > > > So, that depends on pmtu/tcp-probe-mtu.
> > > >
> > > > If the os without pmtu/tcp-probe-mtu has a bigger mtu, then it's big packet
> > > > will lost.
> > > >
> > > > Thanks.
> > > >
> > >
> > > pmtu is designed for routing. LAN is supposed to be configured with
> > > a consistent MTU.
> >
> > Virtio is often used with bridging or macvlan which can't support PMTU.
> > PMTU only works when forwarding at layer 3 (ie routing) where there is
> > a IP address to send the ICMP response. If doing L2 forwarding, the
> > only thin the bridge can do is drop the packet.
> >
> > TCP cab recover but detecting an MTU blackhole requires retransmissions.
>
> Exactly. That's why we basically use the MTU advice supplied by device
> by default - it's designed for use-cases of software devices where
> the device might have more information about the MTU than the guest.
> If hardware devices want e.g. a way to communicate support for
> jumbo frames without communicating any information about the LAN,
> a new feature will be needed.


Let's think this question carefully. If necessary, we will try to introduce a
new feature for virtio-net spec to support Jumbo Frame.

Thanks.


>
> --
> MST
>

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[David Woodhouse]

On 6 May 2023 09:56:35 BST, Hao Chen <chenh@yusur.tech> wrote:
>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.

That router is just plain broken, and it's going to break all kinds of traffic. Hacking the virtio-net MTU is only a partial workaround.

Surely the correct fix here is to apply percussive education to whatever idiot thought it was OK to block ICMP. Not to hack the default MTU of one device to the lowest common denominator.

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Michael S. Tsirkin]

On Sun, May 07, 2023 at 10:31:34AM +0100, David Woodhouse wrote:
> 
> 
> On 6 May 2023 09:56:35 BST, Hao Chen <chenh@yusur.tech> wrote:
> >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.
> 
> That router is just plain broken, and it's going to break all kinds of traffic. Hacking the virtio-net MTU is only a partial workaround.
> 
> Surely the correct fix here is to apply percussive education to whatever idiot thought it was OK to block ICMP. Not to hack the default MTU of one device to the lowest common denominator. 

Yea I don't understand what does path MTU have to do with it.
MTU has to be set the same for all endpoints on LAN, that's
a fundamental assumption that ethernet makes. Going outside LAN
all best are off.


-- 
MST

Re: [PATCH] virtio_net: set default mtu to 1500 when 'Device maximum MTU' bigger than 1500

[Michael S. Tsirkin]

On Sat, May 06, 2023 at 10:15:29AM +0800, Hao Chen 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, so I changed the MTU to a more
> general 1500 when 'Device maximum MTU' bigger than 1500.
> 
> Signed-off-by: Hao Chen <chenh@yusur.tech>

I don't see why not use the maximum. Bigger packets = better
performance.

> ---
>  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)
> +			dev->mtu = 1500;
> +		else
> +			dev->mtu = mtu;
>  		dev->max_mtu = mtu;
>  	}
>  
> -- 
> 2.27.0