* [PATCH next-next-2.6] virtio_net: missing sg_init_table @ 2010-03-29 18:31 Shirley Ma 2010-03-29 19:31 ` Thomas Müller 0 siblings, 1 reply; 10+ messages in thread From: Shirley Ma @ 2010-03-29 18:31 UTC (permalink / raw) To: David Miller; +Cc: netdev, linux-kernel, Thomas Müller Add missing sg_init_table for sg_set_buf in virtio_net. Reported-by: Thomas Müller <thomas@mathtm.de> Signed-off-by: Shirley Ma <xma@us.ibm.com> --- drivers/net/virtio_net.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 25dc77c..3f5be35 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -326,6 +326,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, gfp_t gfp) struct scatterlist sg[2]; int err; + sg_init_table(sg, 2); skb = netdev_alloc_skb_ip_align(vi->dev, MAX_PACKET_LEN); if (unlikely(!skb)) return -ENOMEM; @@ -351,6 +352,7 @@ static int add_recvbuf_big(struct virtnet_info *vi, gfp_t gfp) char *p; int i, err, offset; + sg_init_table(sg, MAX_SKB_FRAGS + 2); /* page in sg[MAX_SKB_FRAGS + 1] is list tail */ for (i = MAX_SKB_FRAGS + 1; i > 1; --i) { first = get_a_page(vi, gfp); ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH next-next-2.6] virtio_net: missing sg_init_table 2010-03-29 18:31 [PATCH next-next-2.6] virtio_net: missing sg_init_table Shirley Ma @ 2010-03-29 19:31 ` Thomas Müller 2010-03-30 1:19 ` [PATCH next-next-2.6 v2] " Shirley Ma 2010-03-30 1:19 ` [PATCH next-next-2.6] virtio_net: missing sg_init_table Shirley Ma 0 siblings, 2 replies; 10+ messages in thread From: Thomas Müller @ 2010-03-29 19:31 UTC (permalink / raw) To: Shirley Ma; +Cc: David Miller, netdev, linux-kernel Am 29.03.2010 20:31, schrieb Shirley Ma: > Add missing sg_init_table for sg_set_buf in virtio_net. > > Reported-by: Thomas Müller <thomas@mathtm.de> > Signed-off-by: Shirley Ma <xma@us.ibm.com> I've just tested the patch and it works fine, so I guess you can add a Tested-by: Thomas Müller <thomas@mathtm.de> line, if you like. Thanks, Thomas > --- > drivers/net/virtio_net.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 25dc77c..3f5be35 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -326,6 +326,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, gfp_t gfp) > struct scatterlist sg[2]; > int err; > > + sg_init_table(sg, 2); > skb = netdev_alloc_skb_ip_align(vi->dev, MAX_PACKET_LEN); > if (unlikely(!skb)) > return -ENOMEM; > @@ -351,6 +352,7 @@ static int add_recvbuf_big(struct virtnet_info *vi, gfp_t gfp) > char *p; > int i, err, offset; > > + sg_init_table(sg, MAX_SKB_FRAGS + 2); > /* page in sg[MAX_SKB_FRAGS + 1] is list tail */ > for (i = MAX_SKB_FRAGS + 1; i > 1; --i) { > first = get_a_page(vi, gfp); > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH next-next-2.6 v2] virtio_net: missing sg_init_table 2010-03-29 19:31 ` Thomas Müller @ 2010-03-30 1:19 ` Shirley Ma 2010-03-31 1:32 ` David Miller 2010-03-31 9:20 ` Michael S. Tsirkin 2010-03-30 1:19 ` [PATCH next-next-2.6] virtio_net: missing sg_init_table Shirley Ma 1 sibling, 2 replies; 10+ messages in thread From: Shirley Ma @ 2010-03-30 1:19 UTC (permalink / raw) To: David Miller; +Cc: Thomas Müller, netdev, linux-kernel Add missing sg_init_table for sg_set_buf in virtio_net which induced in defer skb patch. Reported-by: Thomas Müller <thomas@mathtm.de> Tested-by: Thomas Müller <thomas@mathtm.de> Signed-off-by: Shirley Ma <xma@us.ibm.com> --- drivers/net/virtio_net.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 25dc77c..3f5be35 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -326,6 +326,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, gfp_t gfp) struct scatterlist sg[2]; int err; + sg_init_table(sg, 2); skb = netdev_alloc_skb_ip_align(vi->dev, MAX_PACKET_LEN); if (unlikely(!skb)) return -ENOMEM; @@ -351,6 +352,7 @@ static int add_recvbuf_big(struct virtnet_info *vi, gfp_t gfp) char *p; int i, err, offset; + sg_init_table(sg, MAX_SKB_FRAGS + 2); /* page in sg[MAX_SKB_FRAGS + 1] is list tail */ for (i = MAX_SKB_FRAGS + 1; i > 1; --i) { first = get_a_page(vi, gfp); ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH next-next-2.6 v2] virtio_net: missing sg_init_table 2010-03-30 1:19 ` [PATCH next-next-2.6 v2] " Shirley Ma @ 2010-03-31 1:32 ` David Miller 2010-03-31 9:20 ` Michael S. Tsirkin 1 sibling, 0 replies; 10+ messages in thread From: David Miller @ 2010-03-31 1:32 UTC (permalink / raw) To: mashirle; +Cc: thomas, netdev, linux-kernel From: Shirley Ma <mashirle@us.ibm.com> Date: Mon, 29 Mar 2010 18:19:15 -0700 > Add missing sg_init_table for sg_set_buf in virtio_net which > induced in defer skb patch. > > Reported-by: Thomas Müller <thomas@mathtm.de> > Tested-by: Thomas Müller <thomas@mathtm.de> > Signed-off-by: Shirley Ma <xma@us.ibm.com> Applied, thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH next-next-2.6 v2] virtio_net: missing sg_init_table 2010-03-30 1:19 ` [PATCH next-next-2.6 v2] " Shirley Ma 2010-03-31 1:32 ` David Miller @ 2010-03-31 9:20 ` Michael S. Tsirkin 2010-03-31 10:16 ` David Miller 1 sibling, 1 reply; 10+ messages in thread From: Michael S. Tsirkin @ 2010-03-31 9:20 UTC (permalink / raw) To: Shirley Ma; +Cc: David Miller, Thomas Müller, netdev, linux-kernel On Mon, Mar 29, 2010 at 06:19:15PM -0700, Shirley Ma wrote: > Add missing sg_init_table for sg_set_buf in virtio_net which > induced in defer skb patch. > > Reported-by: Thomas Müller <thomas@mathtm.de> > Tested-by: Thomas Müller <thomas@mathtm.de> > Signed-off-by: Shirley Ma <xma@us.ibm.com> I'm concerned that the 'big' path might cause a performance regression. Let's move sg into virtnet_info so that this needs to be only called once? > --- > drivers/net/virtio_net.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 25dc77c..3f5be35 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -326,6 +326,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, gfp_t gfp) > struct scatterlist sg[2]; > int err; > > + sg_init_table(sg, 2); > skb = netdev_alloc_skb_ip_align(vi->dev, MAX_PACKET_LEN); > if (unlikely(!skb)) > return -ENOMEM; > @@ -351,6 +352,7 @@ static int add_recvbuf_big(struct virtnet_info *vi, gfp_t gfp) > char *p; > int i, err, offset; > > + sg_init_table(sg, MAX_SKB_FRAGS + 2); > /* page in sg[MAX_SKB_FRAGS + 1] is list tail */ > for (i = MAX_SKB_FRAGS + 1; i > 1; --i) { > first = get_a_page(vi, gfp); > > > -- > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH next-next-2.6 v2] virtio_net: missing sg_init_table 2010-03-31 9:20 ` Michael S. Tsirkin @ 2010-03-31 10:16 ` David Miller 2010-03-31 12:41 ` [PATCH] virtio-net: move sg off stack Michael S. Tsirkin [not found] ` <20100331124156.GA4598@redhat.com> 0 siblings, 2 replies; 10+ messages in thread From: David Miller @ 2010-03-31 10:16 UTC (permalink / raw) To: mst; +Cc: mashirle, thomas, netdev, linux-kernel From: "Michael S. Tsirkin" <mst@redhat.com> Date: Wed, 31 Mar 2010 12:20:22 +0300 > On Mon, Mar 29, 2010 at 06:19:15PM -0700, Shirley Ma wrote: >> Add missing sg_init_table for sg_set_buf in virtio_net which >> induced in defer skb patch. >> >> Reported-by: Thomas Müller <thomas@mathtm.de> >> Tested-by: Thomas Müller <thomas@mathtm.de> >> Signed-off-by: Shirley Ma <xma@us.ibm.com> > > I'm concerned that the 'big' path might cause a performance regression. > Let's move sg into virtnet_info so that this needs to be only called > once? Yeah that might improve things. Shirley's change is already in net-next-2.6 so anything implementing this would need to be submitted relative to that. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] virtio-net: move sg off stack 2010-03-31 10:16 ` David Miller @ 2010-03-31 12:41 ` Michael S. Tsirkin [not found] ` <20100331124156.GA4598@redhat.com> 1 sibling, 0 replies; 10+ messages in thread From: Michael S. Tsirkin @ 2010-03-31 12:41 UTC (permalink / raw) To: David S. Miller, Rusty Russell, Jiri Pirko, Michael S. Tsirkin, Shirley Ma, ne Move sg structure off stack and into virtnet_info structure. This helps remove extra sg_init_table calls as well as reduce stack usage. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Compile-tested only for now. Shirley, Rusty Russell, does this fix CONFIG_DEBUG_SG=y for you? drivers/net/virtio_net.c | 52 ++++++++++++++++++++++----------------------- 1 files changed, 25 insertions(+), 27 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 3f5be35..93dcde2 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -39,8 +39,7 @@ module_param(gso, bool, 0444); #define VIRTNET_SEND_COMMAND_SG_MAX 2 -struct virtnet_info -{ +struct virtnet_info { struct virtio_device *vdev; struct virtqueue *rvq, *svq, *cvq; struct net_device *dev; @@ -61,6 +60,10 @@ struct virtnet_info /* Chain pages by the private ptr. */ struct page *pages; + + /* fragments + linear part + virtio header */ + struct scatterlist rx_sg[MAX_SKB_FRAGS + 2]; + struct scatterlist tx_sg[MAX_SKB_FRAGS + 2]; }; struct skb_vnet_hdr { @@ -323,10 +326,8 @@ static int add_recvbuf_small(struct virtnet_info *vi, gfp_t gfp) { struct sk_buff *skb; struct skb_vnet_hdr *hdr; - struct scatterlist sg[2]; int err; - sg_init_table(sg, 2); skb = netdev_alloc_skb_ip_align(vi->dev, MAX_PACKET_LEN); if (unlikely(!skb)) return -ENOMEM; @@ -334,11 +335,11 @@ static int add_recvbuf_small(struct virtnet_info *vi, gfp_t gfp) skb_put(skb, MAX_PACKET_LEN); hdr = skb_vnet_hdr(skb); - sg_set_buf(sg, &hdr->hdr, sizeof hdr->hdr); + sg_set_buf(vi->rx_sg, &hdr->hdr, sizeof hdr->hdr); - skb_to_sgvec(skb, sg + 1, 0, skb->len); + skb_to_sgvec(skb, vi->rx_sg + 1, 0, skb->len); - err = vi->rvq->vq_ops->add_buf(vi->rvq, sg, 0, 2, skb); + err = vi->rvq->vq_ops->add_buf(vi->rvq, vi->rx_sg, 0, 2, skb); if (err < 0) dev_kfree_skb(skb); @@ -347,13 +348,11 @@ static int add_recvbuf_small(struct virtnet_info *vi, gfp_t gfp) static int add_recvbuf_big(struct virtnet_info *vi, gfp_t gfp) { - struct scatterlist sg[MAX_SKB_FRAGS + 2]; struct page *first, *list = NULL; char *p; int i, err, offset; - sg_init_table(sg, MAX_SKB_FRAGS + 2); - /* page in sg[MAX_SKB_FRAGS + 1] is list tail */ + /* page in vi->rx_sg[MAX_SKB_FRAGS + 1] is list tail */ for (i = MAX_SKB_FRAGS + 1; i > 1; --i) { first = get_a_page(vi, gfp); if (!first) { @@ -361,7 +360,7 @@ static int add_recvbuf_big(struct virtnet_info *vi, gfp_t gfp) give_pages(vi, list); return -ENOMEM; } - sg_set_buf(&sg[i], page_address(first), PAGE_SIZE); + sg_set_buf(&vi->rx_sg[i], page_address(first), PAGE_SIZE); /* chain new page in list head to match sg */ first->private = (unsigned long)list; @@ -375,17 +374,17 @@ static int add_recvbuf_big(struct virtnet_info *vi, gfp_t gfp) } p = page_address(first); - /* sg[0], sg[1] share the same page */ - /* a separated sg[0] for virtio_net_hdr only during to QEMU bug*/ - sg_set_buf(&sg[0], p, sizeof(struct virtio_net_hdr)); + /* vi->rx_sg[0], vi->rx_sg[1] share the same page */ + /* a separated vi->rx_sg[0] for virtio_net_hdr only due to QEMU bug */ + sg_set_buf(&vi->rx_sg[0], p, sizeof(struct virtio_net_hdr)); - /* sg[1] for data packet, from offset */ + /* vi->rx_sg[1] for data packet, from offset */ offset = sizeof(struct padded_vnet_hdr); - sg_set_buf(&sg[1], p + offset, PAGE_SIZE - offset); + sg_set_buf(&vi->rx_sg[1], p + offset, PAGE_SIZE - offset); /* chain first in list head */ first->private = (unsigned long)list; - err = vi->rvq->vq_ops->add_buf(vi->rvq, sg, 0, MAX_SKB_FRAGS + 2, + err = vi->rvq->vq_ops->add_buf(vi->rvq, vi->rx_sg, 0, MAX_SKB_FRAGS + 2, first); if (err < 0) give_pages(vi, first); @@ -396,16 +395,15 @@ static int add_recvbuf_big(struct virtnet_info *vi, gfp_t gfp) static int add_recvbuf_mergeable(struct virtnet_info *vi, gfp_t gfp) { struct page *page; - struct scatterlist sg; int err; page = get_a_page(vi, gfp); if (!page) return -ENOMEM; - sg_init_one(&sg, page_address(page), PAGE_SIZE); + sg_init_one(&vi->rx_sg, page_address(page), PAGE_SIZE); - err = vi->rvq->vq_ops->add_buf(vi->rvq, &sg, 0, 1, page); + err = vi->rvq->vq_ops->add_buf(vi->rvq, &vi->rx_sg, 0, 1, page); if (err < 0) give_pages(vi, page); @@ -514,12 +512,9 @@ static unsigned int free_old_xmit_skbs(struct virtnet_info *vi) static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb) { - struct scatterlist sg[2+MAX_SKB_FRAGS]; struct skb_vnet_hdr *hdr = skb_vnet_hdr(skb); const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest; - sg_init_table(sg, 2+MAX_SKB_FRAGS); - pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest); if (skb->ip_summed == CHECKSUM_PARTIAL) { @@ -553,12 +548,13 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb) /* Encode metadata header at front. */ if (vi->mergeable_rx_bufs) - sg_set_buf(sg, &hdr->mhdr, sizeof hdr->mhdr); + sg_set_buf(vi->tx_sg, &hdr->mhdr, sizeof hdr->mhdr); else - sg_set_buf(sg, &hdr->hdr, sizeof hdr->hdr); + sg_set_buf(vi->tx_sg, &hdr->hdr, sizeof hdr->hdr); - hdr->num_sg = skb_to_sgvec(skb, sg+1, 0, skb->len) + 1; - return vi->svq->vq_ops->add_buf(vi->svq, sg, hdr->num_sg, 0, skb); + hdr->num_sg = skb_to_sgvec(skb, vi->tx_sg + 1, 0, skb->len) + 1; + return vi->svq->vq_ops->add_buf(vi->svq, vi->tx_sg, hdr->num_sg, + 0, skb); } static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) @@ -941,6 +937,8 @@ static int virtnet_probe(struct virtio_device *vdev) vdev->priv = vi; vi->pages = NULL; INIT_DELAYED_WORK(&vi->refill, refill_work); + sg_init_table(vi->rx_sg, ARRAY_SIZE(vi->rx_sg)); + sg_init_table(vi->tx_sg, ARRAY_SIZE(vi->tx_sg)); /* If we can receive ANY GSO packets, we must allocate large ones. */ if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || -- 1.7.0.2.280.gc6f05 ^ permalink raw reply related [flat|nested] 10+ messages in thread
[parent not found: <20100331124156.GA4598@redhat.com>]
* Re: [PATCH] virtio-net: move sg off stack [not found] ` <20100331124156.GA4598@redhat.com> @ 2010-04-02 2:26 ` David Miller 2010-04-06 3:14 ` Rusty Russell 0 siblings, 1 reply; 10+ messages in thread From: David Miller @ 2010-04-02 2:26 UTC (permalink / raw) To: mst; +Cc: rusty, jpirko, xma, netdev, linux-kernel From: "Michael S. Tsirkin" <mst@redhat.com> Date: Wed, 31 Mar 2010 15:41:58 +0300 > Move sg structure off stack and into virtnet_info structure. > This helps remove extra sg_init_table calls as well as reduce > stack usage. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Compile-tested only for now. > Shirley, Rusty Russell, does this fix CONFIG_DEBUG_SG=y for you? I'll apply this once I see some ACKs on testing. Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] virtio-net: move sg off stack 2010-04-02 2:26 ` David Miller @ 2010-04-06 3:14 ` Rusty Russell 0 siblings, 0 replies; 10+ messages in thread From: Rusty Russell @ 2010-04-06 3:14 UTC (permalink / raw) To: David Miller; +Cc: mst, jpirko, xma, netdev, linux-kernel On Fri, 2 Apr 2010 12:56:48 pm David Miller wrote: > From: "Michael S. Tsirkin" <mst@redhat.com> > Date: Wed, 31 Mar 2010 15:41:58 +0300 > > > Move sg structure off stack and into virtnet_info structure. > > This helps remove extra sg_init_table calls as well as reduce > > stack usage. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > Compile-tested only for now. > > Shirley, Rusty Russell, does this fix CONFIG_DEBUG_SG=y for you? Yes, this fixes the BUG_ON() for me (trivial changes to base on Linus' tree only). Thanks, Rusty. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH next-next-2.6] virtio_net: missing sg_init_table 2010-03-29 19:31 ` Thomas Müller 2010-03-30 1:19 ` [PATCH next-next-2.6 v2] " Shirley Ma @ 2010-03-30 1:19 ` Shirley Ma 1 sibling, 0 replies; 10+ messages in thread From: Shirley Ma @ 2010-03-30 1:19 UTC (permalink / raw) To: Thomas Müller; +Cc: David Miller, netdev, linux-kernel On Mon, 2010-03-29 at 21:31 +0200, Thomas Müller wrote: > I've just tested the patch and it works fine, so I guess you can add a > Tested-by: Thomas Müller <thomas@mathtm.de> > line, if you like. Thanks. Updated the patch with Tested-by. Shirley ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-04-06 3:14 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-29 18:31 [PATCH next-next-2.6] virtio_net: missing sg_init_table Shirley Ma
2010-03-29 19:31 ` Thomas Müller
2010-03-30 1:19 ` [PATCH next-next-2.6 v2] " Shirley Ma
2010-03-31 1:32 ` David Miller
2010-03-31 9:20 ` Michael S. Tsirkin
2010-03-31 10:16 ` David Miller
2010-03-31 12:41 ` [PATCH] virtio-net: move sg off stack Michael S. Tsirkin
[not found] ` <20100331124156.GA4598@redhat.com>
2010-04-02 2:26 ` David Miller
2010-04-06 3:14 ` Rusty Russell
2010-03-30 1:19 ` [PATCH next-next-2.6] virtio_net: missing sg_init_table Shirley Ma
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).