virtio_net: don't set NETIF_F_FRAGLIST

PATCH In review Mark McLoughlin 3 participants 7 messages Nov 5, 2008 – Nov 6, 2008

Subsystems (3)

Networking Network drivers virtio net driver

lore

Share & tools

lore

Mark McLoughlin

author

2008-11-05 12:35 UTC #0 lore raw

PATCH 1/1 virtio_net: don't set NETIF_F_FRAGLIST

We don't handle skb_shared_info->frag_list, so we shouldn't be setting the NETIF_F_FRAGLIST flag.

Signed-off-by Mark McLoughlin <[email protected]>

---
 drivers/net/virtio_net.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/virtio_net.c +1 -1

index 0196a0d..9d4fdad 100644
@@ -505,7 +505,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 	/* Do we support "hardware" checksums? */
 	if (csum && virtio_has_feature(vdev, VIRTIO_NET_F_CSUM)) {
 		/* This opens up the world of extra features. */
-		dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
+		dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG;
 		if (gso && virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
 			dev->features |= NETIF_F_TSO | NETIF_F_UFO
 				| NETIF_F_TSO_ECN | NETIF_F_TSO6;

Signature · 1 line

1.5.4.3

Rusty Russell

2008-11-05 21:53 UTC #1 lore raw

Quoted 2 lines · On Wednesday 05 November 2008 23:34:46 Mark McLoughlin wrote:

We don't handle skb_shared_info->frag_list, so we shouldn't be setting the NETIF_F_FRAGLIST flag.

We don't?

skb_to_sgvec() should handle it OK, is there some subtlety I'm missing?

Rusty.

Mark McLoughlin

author

2008-11-06 10:11 UTC #2 lore raw

Quoted 7 lines · Rusty Russell

Quoted 2 lines · On Wednesday 05 November 2008 23:34:46 Mark McLoughlin wrote:
We don't handle skb_shared_info->frag_list, so we shouldn't be setting the NETIF_F_FRAGLIST flag.
We don't?
skb_to_sgvec() should handle it OK, is there some subtlety I'm missing?

Good grief ... yes, sorry.

Same is true for the tun patch, but I wonder ... could a fraglist skb exceed 64k? i.e. we need to know the max possible size of a packet before reading it from the tap.

Cheers,
Mark.

Jeff Garzik

2008-11-06 05:43 UTC #3 lore raw

In reply to Mark McLoughlin

Quoted 13 lines · Mark McLoughlin

We don't handle skb_shared_info->frag_list, so we shouldn't be setting the NETIF_F_FRAGLIST flag.
Signed-off-by Mark McLoughlin <[email protected]>
---
 drivers/net/virtio_net.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/net/virtio_net.c +0 -0
index 0196a0d..9d4fdad 100644

drivers/net/virtio_net.c static int virtnet_probe(struct virtio_device *vdev) · line 511

Quoted 8 lines

drivers/net/virtio_net.c +1 -1

@@ -505,7 +505,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 	/* Do we support "hardware" checksums? */
 	if (csum && virtio_has_feature(vdev, VIRTIO_NET_F_CSUM)) {
 		/* This opens up the world of extra features. */
-		dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
+		dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG;
 		if (gso && virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
 			dev->features |= NETIF_F_TSO | NETIF_F_UFO
 				| NETIF_F_TSO_ECN | NETIF_F_TSO6;

applied this and tun patch

Mark McLoughlin

author

2008-11-06 10:13 UTC #4 lore raw

Quoted 9 lines · Jeff Garzik

Quoted 8 lines · Mark McLoughlin
We don't handle skb_shared_info->frag_list, so we shouldn't be setting the NETIF_F_FRAGLIST flag.
Signed-off-by Mark McLoughlin <[email protected]>
---
 drivers/net/virtio_net.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Quoted 15 lines

drivers/net/virtio_net.c static int virtnet_probe(struct virtio_device *vdev) · line 511

Quoted 13 lines

drivers/net/virtio_net.c +1 -1

index 0196a0d..9d4fdad 100644
@@ -505,7 +505,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 	/* Do we support "hardware" checksums? */
 	if (csum && virtio_has_feature(vdev, VIRTIO_NET_F_CSUM)) {
 		/* This opens up the world of extra features. */
-		dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
+		dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG;
 		if (gso && virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
 			dev->features |= NETIF_F_TSO | NETIF_F_UFO
 				| NETIF_F_TSO_ECN | NETIF_F_TSO6;

applied this and tun patch

Sorry Jeff, Rusty correctly points out that we do in fact handle shinfo->frags; just a moment of blindness on my part.

Thanks,
Mark.

Jeff Garzik

2008-11-06 10:48 UTC #5 lore raw

Quoted 25 lines · Mark McLoughlin

Quoted 9 lines · Jeff Garzik
Quoted 8 lines · Mark McLoughlin
We don't handle skb_shared_info->frag_list, so we shouldn't be setting the NETIF_F_FRAGLIST flag.
Signed-off-by Mark McLoughlin <[email protected]>
---
 drivers/net/virtio_net.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Quoted 14 lines
drivers/net/virtio_net.c static int virtnet_probe(struct virtio_device *vdev) · line 511
Quoted 13 lines
drivers/net/virtio_net.c +1 -1
index 0196a0d..9d4fdad 100644
@@ -505,7 +505,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 	/* Do we support "hardware" checksums? */
 	if (csum && virtio_has_feature(vdev, VIRTIO_NET_F_CSUM)) {
 		/* This opens up the world of extra features. */
-		dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
+		dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG;
 		if (gso && virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
 			dev->features |= NETIF_F_TSO | NETIF_F_UFO
 				| NETIF_F_TSO_ECN | NETIF_F_TSO6;
applied this and tun patch

Quoted 2 lines

Sorry Jeff, Rusty correctly points out that we do in fact handle shinfo->frags; just a moment of blindness on my part.

but what about frag_list ?

Mark McLoughlin

author

2008-11-06 10:57 UTC #6 lore raw

Quoted 26 lines · Jeff Garzik

Quoted 25 lines · Mark McLoughlin
Quoted 23 lines · Jeff Garzik
Quoted 13 lines · Mark McLoughlin
We don't handle skb_shared_info->frag_list, so we shouldn't be setting the NETIF_F_FRAGLIST flag.
Signed-off-by Mark McLoughlin <[email protected]>
---
 drivers/net/virtio_net.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/net/virtio_net.c +0 -0
index 0196a0d..9d4fdad 100644
drivers/net/virtio_net.c static int virtnet_probe(struct virtio_device *vdev) · line 511
Quoted 8 lines
drivers/net/virtio_net.c +1 -1
@@ -505,7 +505,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 	/* Do we support "hardware" checksums? */
 	if (csum && virtio_has_feature(vdev, VIRTIO_NET_F_CSUM)) {
 		/* This opens up the world of extra features. */
-		dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
+		dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG;
 		if (gso && virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
 			dev->features |= NETIF_F_TSO | NETIF_F_UFO
 				| NETIF_F_TSO_ECN | NETIF_F_TSO6;
applied this and tun patch

Quoted 4 lines

Quoted 2 lines
Sorry Jeff, Rusty correctly points out that we do in fact handle shinfo->frags; just a moment of blindness on my part.
but what about frag_list ?

Yeah, I meant shinfo->frag_list.

virtio_net uses skb_to_sgvec() and tun uses skb_copy_datagram_iovec()

Both handle shinfo->frag_list correctly.

Thanks,
Mark.