| Message ID | 1225888486-977-1-git-send-email-markmc@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Jeff Garzik |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark McLoughlin wrote: > 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 <markmc@redhat.com> > --- > drivers/net/virtio_net.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 0196a0d..9d4fdad 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2008-11-06 at 08:53 +1100, Rusty Russell wrote: > 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2008-11-06 at 00:43 -0500, Jeff Garzik wrote: > Mark McLoughlin wrote: > > 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 <markmc@redhat.com> > > --- > > drivers/net/virtio_net.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 0196a0d..9d4fdad 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark McLoughlin wrote: > On Thu, 2008-11-06 at 00:43 -0500, Jeff Garzik wrote: >> Mark McLoughlin wrote: >>> 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 <markmc@redhat.com> >>> --- >>> drivers/net/virtio_net.c | 2 +- >>> 1 files changed, 1 insertions(+), 1 deletions(-) >>> >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>> index 0196a0d..9d4fdad 100644 >>> --- a/drivers/net/virtio_net.c >>> +++ b/drivers/net/virtio_net.c >>> @@ -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. but what about frag_list ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2008-11-06 at 05:48 -0500, Jeff Garzik wrote: > Mark McLoughlin wrote: > > On Thu, 2008-11-06 at 00:43 -0500, Jeff Garzik wrote: > >> Mark McLoughlin wrote: > >>> 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 <markmc@redhat.com> > >>> --- > >>> drivers/net/virtio_net.c | 2 +- > >>> 1 files changed, 1 insertions(+), 1 deletions(-) > >>> > >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > >>> index 0196a0d..9d4fdad 100644 > >>> --- a/drivers/net/virtio_net.c > >>> +++ b/drivers/net/virtio_net.c > >>> @@ -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. > > 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 0196a0d..9d4fdad 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -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;
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 <markmc@redhat.com> --- drivers/net/virtio_net.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)