[PATCH] virtio_net: large tx MTU support

[PATCH] virtio_net: large tx MTU support

[Mark McLoughlin]

We don't really have a max tx packet size limit, so allow configuring
the device with up to 64k tx MTU.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 drivers/net/virtio_net.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e6b5d6e..71ca29c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -613,6 +613,17 @@ static struct ethtool_ops virtnet_ethtool_ops = {
 	.set_tso = ethtool_op_set_tso,
 };
 
+#define MIN_MTU 68
+#define MAX_MTU 65535
+
+static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
+{
+	if (new_mtu < MIN_MTU || new_mtu > MAX_MTU)
+		return -EINVAL;
+	dev->mtu = new_mtu;
+	return 0;
+}
+
 static int virtnet_probe(struct virtio_device *vdev)
 {
 	int err;
@@ -628,6 +639,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 	dev->open = virtnet_open;
 	dev->stop = virtnet_close;
 	dev->hard_start_xmit = start_xmit;
+	dev->change_mtu = virtnet_change_mtu;
 	dev->features = NETIF_F_HIGHDMA;
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	dev->poll_controller = virtnet_netpoll;
-- 
1.6.0.3

Re: [PATCH] virtio_net: large tx MTU support

[David Miller]

From: Mark McLoughlin <markmc@redhat.com>
Date: Wed, 26 Nov 2008 13:58:11 +0000

> We don't really have a max tx packet size limit, so allow configuring
> the device with up to 64k tx MTU.
> 
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>

Rusty, ACK?

If so, I'll toss this into net-next-2.6, thanks!

Re: [PATCH] virtio_net: large tx MTU support

[Rusty Russell]

On Thursday 27 November 2008 00:28:11 Mark McLoughlin wrote:
> We don't really have a max tx packet size limit, so allow configuring
> the device with up to 64k tx MTU.

Hi Mark,

Just one comment: maybe we should be conservative and maybe limit to 1500 if 
the host doesn't offer any of the GSO or MRG_RXBUF features?

Thanks,
Rusty.

Re: [PATCH] virtio_net: large tx MTU support

[Mark McLoughlin]

Hi Rusty,

On Thu, 2008-11-27 at 23:00 +1030, Rusty Russell wrote:
> On Thursday 27 November 2008 00:28:11 Mark McLoughlin wrote:
> > We don't really have a max tx packet size limit, so allow configuring
> > the device with up to 64k tx MTU.
> 
> Hi Mark,
> 
> Just one comment: maybe we should be conservative and maybe limit to 1500 if 
> the host doesn't offer any of the GSO or MRG_RXBUF features?

That was actually what I was going to do until I thought about it a bit
more and discussed it with Herbert.

The virtio_net MTU only affects the transmit path, so there shouldn't be
any issue with a host that doesn't support those features.

The MTU on the receive path is a different story - e.g. the host needs
to have a large enough buffer when reading from the tap fd and the guest
needs to have allocated large enough receive buffers. The former we
fixed recently in KVM (we used to only have a 64k buffer if built with
IFF_VNET_HDR support) and the latter is currently only true if either
GSO or MRG_RXBUF has been negotiated.

So, in summary - making change_mtu() check for GSO and MRG_RXBUF isn't
correct, but perhaps we could do it anyway so as to give the user a hint
that a large MTU isn't going to work on the receive path. If you feel
that's best, here's another version.

Cheers,
Mark.

From: Mark McLoughlin <markmc@redhat.com>
Subject: [PATCH] virtio_net: large tx MTU support

We don't really have a max tx packet size limit, so allow configuring
the device with up to 64k tx MTU.

On the receive path, we can only handle a large MTU if we negotiate
GSO or MRG_RXBUF with the host. In order to give the user some hint
as to this limitation, we also limit the transmit path to a smaller
MTU if neither of those features are available.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 drivers/net/virtio_net.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e6b5d6e..499c7a5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -613,6 +613,29 @@ static struct ethtool_ops virtnet_ethtool_ops = {
 	.set_tso = ethtool_op_set_tso,
 };
 
+#define MIN_MTU 68
+#define MAX_MTU 65535
+
+static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	int max_mtu;
+
+	/* Only allow a large MTU if we know we have a chance
+	 * of also supporting that MTU on the receive side. */
+	if (vi->mergeable_rx_bufs || vi->big_packets)
+		max_mtu = MAX_MTU;
+	else
+		max_mtu = ETH_DATA_LEN;
+
+	if (new_mtu < MIN_MTU || new_mtu > max_mtu)
+		return -EINVAL;
+
+	dev->mtu = new_mtu;
+
+	return 0;
+}
+
 static int virtnet_probe(struct virtio_device *vdev)
 {
 	int err;
@@ -628,6 +651,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 	dev->open = virtnet_open;
 	dev->stop = virtnet_close;
 	dev->hard_start_xmit = start_xmit;
+	dev->change_mtu = virtnet_change_mtu;
 	dev->features = NETIF_F_HIGHDMA;
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	dev->poll_controller = virtnet_netpoll;
-- 
1.6.0.3

Re: [PATCH] virtio_net: large tx MTU support

[Rusty Russell]

On Friday 28 November 2008 00:27:05 Mark McLoughlin wrote:
> Hi Rusty,
>
> On Thu, 2008-11-27 at 23:00 +1030, Rusty Russell wrote:
> > On Thursday 27 November 2008 00:28:11 Mark McLoughlin wrote:
> > > We don't really have a max tx packet size limit, so allow configuring
> > > the device with up to 64k tx MTU.
> >
> > Hi Mark,
> >
> > Just one comment: maybe we should be conservative and maybe limit to 1500
> > if the host doesn't offer any of the GSO or MRG_RXBUF features?
>
> That was actually what I was going to do until I thought about it a bit
> more and discussed it with Herbert.
>
> The virtio_net MTU only affects the transmit path, so there shouldn't be
> any issue with a host that doesn't support those features.

Not quite what I meant.  A minimal host can reasonably expect ethernet-fitting
packets.  If it supports GSO of course it must handle larger ones.  Otherwise
we should add YA feature bit or even a max-mtu field.

So, I was thinking something like this over your patch (I also removed the
used-once MAX and MIN definitions; I dislike gratuitous indirection):

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -480,26 +480,24 @@ static struct ethtool_ops virtnet_ethtoo
 	.set_sg = ethtool_op_set_sg,
 };
 
-#define MIN_MTU 68
-#define MAX_MTU 65535
-
 static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 	int max_mtu;
 
-	/* Only allow a large MTU if we know we have a chance
-	 * of also supporting that MTU on the receive side. */
-	if (vi->mergeable_rx_bufs || vi->big_packets)
-		max_mtu = MAX_MTU;
+	/* A host which can handle GSO must handle large packets. */
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GSO)
+	    || virtio_has_feature(vi->vdev, VIRTIO_NET_F_HOST_TSO4)
+	    || virtio_has_feature(vi->vdev, VIRTIO_NET_F_HOST_TSO6)
+	    || virtio_has_feature(vi->vdev, VIRTIO_NET_F_HOST_UFO))
+		max_mtu = 65535;
 	else
 		max_mtu = ETH_DATA_LEN;
 
-	if (new_mtu < MIN_MTU || new_mtu > max_mtu)
+	if (new_mtu < 68 || new_mtu > max_mtu)
 		return -EINVAL;
 
 	dev->mtu = new_mtu;
-
 	return 0;
 }
 

Re: [PATCH] virtio_net: large tx MTU support

[Mark McLoughlin]

On Fri, 2008-11-28 at 10:52 +1030, Rusty Russell wrote:
> On Friday 28 November 2008 00:27:05 Mark McLoughlin wrote:
> > Hi Rusty,
> >
> > On Thu, 2008-11-27 at 23:00 +1030, Rusty Russell wrote:
> > > On Thursday 27 November 2008 00:28:11 Mark McLoughlin wrote:
> > > > We don't really have a max tx packet size limit, so allow configuring
> > > > the device with up to 64k tx MTU.
> > >
> > > Hi Mark,
> > >
> > > Just one comment: maybe we should be conservative and maybe limit to 1500
> > > if the host doesn't offer any of the GSO or MRG_RXBUF features?
> >
> > That was actually what I was going to do until I thought about it a bit
> > more and discussed it with Herbert.
> >
> > The virtio_net MTU only affects the transmit path, so there shouldn't be
> > any issue with a host that doesn't support those features.
> 
> Not quite what I meant.

Well, you did mention MRG_RXBUF :-)

> A minimal host can reasonably expect ethernet-fitting packets.  If it
> supports GSO of course it must handle larger ones.

I think this is orthogonal to GSO - e.g. a host may support GSO even if
it can only physically transmit 1500 byte frames.

MTU configuration is commonly a trial and error thing, so we're better
off allowing larger MTU sizes in cases where the host might not support
it rather than disallowing it in cases where the host can support it.

> Otherwise we should add YA feature bit or even a max-mtu field.

If e.g. we allowed physical device assignment via virtio, then the MTU
would be limited to the MTU supported by the physical device. In that
case it might make sense to add a max-mtu field or similar, but IMHO
it's fine to allow larger MTU sizes in the mean time.

Cheers,
Mark.

Re: [PATCH] virtio_net: large tx MTU support

[Rusty Russell]

On Friday 28 November 2008 19:59:55 Mark McLoughlin wrote:
> On Fri, 2008-11-28 at 10:52 +1030, Rusty Russell wrote:
> > On Friday 28 November 2008 00:27:05 Mark McLoughlin wrote:
> > > Hi Rusty,
> > >
> > > On Thu, 2008-11-27 at 23:00 +1030, Rusty Russell wrote:
> > > > On Thursday 27 November 2008 00:28:11 Mark McLoughlin wrote:
> > > > > We don't really have a max tx packet size limit, so allow
> > > > > configuring the device with up to 64k tx MTU.
> > > >
> > > > Hi Mark,
> > > >
> > > > Just one comment: maybe we should be conservative and maybe limit to
> > > > 1500 if the host doesn't offer any of the GSO or MRG_RXBUF features?
> > >
> > > That was actually what I was going to do until I thought about it a bit
> > > more and discussed it with Herbert.
> > >
> > > The virtio_net MTU only affects the transmit path, so there shouldn't
> > > be any issue with a host that doesn't support those features.
> >
> > Not quite what I meant.
>
> Well, you did mention MRG_RXBUF :-)

Yeah, but you should know by now not to listen to me!

> > A minimal host can reasonably expect ethernet-fitting packets.  If it
> > supports GSO of course it must handle larger ones.
>
> I think this is orthogonal to GSO - e.g. a host may support GSO even if
> it can only physically transmit 1500 byte frames.

Sure, but we know at least it can take large packets in some respect.

> MTU configuration is commonly a trial and error thing, so we're better
> off allowing larger MTU sizes in cases where the host might not support
> it rather than disallowing it in cases where the host can support it.

We're changing the driver behaviour; we *should* add a feature bit.  The only 
counter-argument to this is that all the existing hosts we know of allow the 
behaviour we're specifying.

(Grovelling through an old qemu here it seems to be true, and old tun/tap 
would take anything).

> If e.g. we allowed physical device assignment via virtio, then the MTU
> would be limited to the MTU supported by the physical device. In that
> case it might make sense to add a max-mtu field or similar, but IMHO
> it's fine to allow larger MTU sizes in the mean time.

OK, I'm swayed.  Only because the user has to make a conscious decision to up 
the MTU; and if they choose wrong, networking won't work.

I've applied your original patch.

Thanks!
Rusty.