Skip to contentSkip to conversation

virtio_net: large tx MTU support

PATCH In review Mark McLoughlin 3 participants 13 messages Nov 26, 2008 – Dec 1, 2008

Subsystems (4)
Networking Network drivers virtio core virtio net driver
lore
Share & tools
lore

Mark McLoughlin

author
#0 lore raw

PATCH 1/1 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.

Signed-off-by Mark McLoughlin <[email protected]>
---
 drivers/net/virtio_net.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/net/virtio_net.c +12 -0
index e6b5d6e..71ca29c 100644
@@ -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;
Signature · 1 line
1.6.0.3
#3 lore raw
In reply to Mark McLoughlin
Quoted 2 lines · 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.

#4 lore raw
In reply to Mark McLoughlin
Quoted 2 lines · 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.

author
#5 lore raw

Hi Rusty,

Quoted 8 lines · Rusty Russell
Quoted 2 lines · 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 <[email protected]>
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 <[email protected]>
---
 drivers/net/virtio_net.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)
drivers/net/virtio_net.c +24 -0
index e6b5d6e..499c7a5 100644
@@ -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;
Signature · 1 line
1.6.0.3
author
#6 lore raw
In reply to Rusty Russell

Hi Rusty,

Quoted 8 lines · Rusty Russell
Quoted 2 lines · 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 <[email protected]>
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 <[email protected]>
---
 drivers/net/virtio_net.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)
drivers/net/virtio_net.c +24 -0
index e6b5d6e..499c7a5 100644
@@ -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;
Signature · 1 line
1.6.0.3
#7 lore raw
Quoted 9 lines · On Friday 28 November 2008 00:27:05 Mark McLoughlin wrote:

Hi Rusty,

Quoted 6 lines · Rusty Russell
Quoted 2 lines · 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,

Quoted 8 lines
Quoted 2 lines

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):

drivers/net/virtio_net.c +7 -9
@@ -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;
 }
#8 lore raw
In reply to Mark McLoughlin
Quoted 9 lines · On Friday 28 November 2008 00:27:05 Mark McLoughlin wrote:

Hi Rusty,

Quoted 6 lines · Rusty Russell
Quoted 2 lines · 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,

Quoted 8 lines
Quoted 2 lines

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):

drivers/net/virtio_net.c +7 -9
@@ -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;
 }
author
#9 lore raw
Quoted 13 lines · Rusty Russell
Quoted 12 lines · On Friday 28 November 2008 00:27:05 Mark McLoughlin wrote:

Hi Rusty,

Quoted 8 lines · Rusty Russell
Quoted 2 lines · 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?

Quoted 7 lines
Quoted 5 lines

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 :-)

Quoted 2 lines

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.

Quoted 1 line

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.

author
#10 lore raw
In reply to Rusty Russell
Quoted 13 lines · Rusty Russell
Quoted 12 lines · On Friday 28 November 2008 00:27:05 Mark McLoughlin wrote:

Hi Rusty,

Quoted 8 lines · Rusty Russell
Quoted 2 lines · 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?

Quoted 7 lines
Quoted 5 lines

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 :-)

Quoted 2 lines

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.

Quoted 1 line

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.

#11 lore raw
Quoted 17 lines · On Friday 28 November 2008 19:59:55 Mark McLoughlin wrote:
Quoted 16 lines · Rusty Russell
Quoted 15 lines · On Friday 28 November 2008 00:27:05 Mark McLoughlin wrote:

Hi Rusty,

Quoted 8 lines · Rusty Russell
Quoted 2 lines · 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.

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

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!

Quoted 5 lines
Quoted 2 lines

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.

Quoted 3 lines

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).

Quoted 4 lines

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.

#12 lore raw
In reply to Mark McLoughlin
Quoted 17 lines · On Friday 28 November 2008 19:59:55 Mark McLoughlin wrote:
Quoted 16 lines · Rusty Russell
Quoted 15 lines · On Friday 28 November 2008 00:27:05 Mark McLoughlin wrote:

Hi Rusty,

Quoted 8 lines · Rusty Russell
Quoted 2 lines · 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.

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

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!

Quoted 5 lines
Quoted 2 lines

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.

Quoted 3 lines

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).

Quoted 4 lines

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.