* [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb
@ 2013-11-20 9:07 Jason Wang
2013-11-20 9:07 ` [PATCH net 2/3] virtio-net: fix num calculation on frag skb allocation failure Jason Wang
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Jason Wang @ 2013-11-20 9:07 UTC (permalink / raw)
To: rusty, mst, virtualization, netdev, linux-kernel
Cc: Michael Dalton, Eric Dumazet, Shirley Ma
When mergeable buffer were used, we only put the first page buf leave the rest
of buffers in the virt queue. This will cause the driver could not get the
correct head buffer any more. Fix this by dropping the rest of buffers for this
packet.
The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
(virtio_net: Defer skb allocation in receive path).
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Michael Dalton <mwdalton@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Shirley Ma <xma@us.ibm.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
This patch was needed for stable
---
drivers/net/virtio_net.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7bab4de..24fd502 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -222,6 +222,17 @@ static void skb_xmit_done(struct virtqueue *vq)
netif_wake_subqueue(vi->dev, vq2txq(vq));
}
+static void drop_mergeable_buffer(struct receive_queue *rq, int num_buf)
+{
+ char *buf;
+ int len;
+
+ while (--num_buf && (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
+ --rq->num;
+ put_page(virt_to_head_page(buf));
+ }
+}
+
/* Called from bottom half context */
static struct sk_buff *page_to_skb(struct receive_queue *rq,
struct page *page, unsigned int offset,
@@ -237,8 +248,13 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
/* copy small packet so we can reuse these pages for small data */
skb = netdev_alloc_skb_ip_align(vi->dev, GOOD_COPY_LEN);
- if (unlikely(!skb))
+ if (unlikely(!skb)) {
+ if (vi->mergeable_rx_bufs) {
+ hdr = (struct skb_vnet_hdr *)p;
+ drop_mergeable_buffer(rq, hdr->mhdr.num_buffers);
+ }
return NULL;
+ }
hdr = skb_vnet_hdr(skb);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH net 2/3] virtio-net: fix num calculation on frag skb allocation failure
2013-11-20 9:07 [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb Jason Wang
@ 2013-11-20 9:07 ` Jason Wang
2013-11-20 10:37 ` Michael S. Tsirkin
2013-11-20 9:07 ` [PATCH net 3/3] virtio-net: fix resources leaking when fail to allocate frag skb Jason Wang
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Jason Wang @ 2013-11-20 9:07 UTC (permalink / raw)
To: rusty, mst, virtualization, netdev, linux-kernel
Cc: Michael Dalton, Eric Dumazet
We need decrease the rq->num after we can get a buf through
virtqueue_get_buf() even if we could not allocate frag skb. Otherwise, the
refill routine won't be triggered under heavy memory stress since the driver may
still think there's enough room.
This bug was introduced by commit 2613af0ed18a11d5c566a81f9a6510b73180660a
(virtio_net: migrate mergeable rx buffers to page frag allocators).
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Michael Dalton <mwdalton@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
The patch was needed for 3.12 stable.
---
drivers/net/virtio_net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 24fd502..de1d6ca 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -333,6 +333,7 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
head_skb->dev->stats.rx_length_errors++;
return -EINVAL;
}
+ --rq->num;
if (unlikely(len > MERGE_BUFFER_LEN)) {
pr_debug("%s: rx error: merge buffer too long\n",
head_skb->dev->name);
@@ -367,7 +368,6 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
skb_add_rx_frag(curr_skb, num_skb_frags, page,
offset, len, MERGE_BUFFER_LEN);
}
- --rq->num;
}
return 0;
}
--
1.8.3.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH net 3/3] virtio-net: fix resources leaking when fail to allocate frag skb
2013-11-20 9:07 [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb Jason Wang
2013-11-20 9:07 ` [PATCH net 2/3] virtio-net: fix num calculation on frag skb allocation failure Jason Wang
@ 2013-11-20 9:07 ` Jason Wang
2013-11-20 10:34 ` [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb Michael S. Tsirkin
2013-11-20 13:26 ` [PATCH RFC] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
3 siblings, 0 replies; 14+ messages in thread
From: Jason Wang @ 2013-11-20 9:07 UTC (permalink / raw)
To: rusty, mst, virtualization, netdev, linux-kernel
Cc: Michael Dalton, Eric Dumazet
When we fail to allocate a frag skb, we need put the refcnt of the page and drop
the rest of the buffers. Otherwise the page was leaked and driver won't get a
correct head buffer after this failure.
This bug was introduced by commit 2613af0ed18a11d5c566a81f9a6510b73180660a
(virtio_net: migrate mergeable rx buffers to page frag allocators).
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Michael Dalton <mwdalton@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
The patch was needed for 3.12 stable.
---
drivers/net/virtio_net.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index de1d6ca..f6f1e20 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -339,9 +339,12 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
head_skb->dev->name);
len = MERGE_BUFFER_LEN;
}
+ page = virt_to_head_page(buf);
if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
if (unlikely(!nskb)) {
+ put_page(page);
+ drop_mergeable_buffer(rq, num_buf);
head_skb->dev->stats.rx_dropped++;
return -ENOMEM;
}
@@ -358,7 +361,6 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
head_skb->len += len;
head_skb->truesize += MERGE_BUFFER_LEN;
}
- page = virt_to_head_page(buf);
offset = buf - (char *)page_address(page);
if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
put_page(page);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb
2013-11-20 9:07 [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb Jason Wang
2013-11-20 9:07 ` [PATCH net 2/3] virtio-net: fix num calculation on frag skb allocation failure Jason Wang
2013-11-20 9:07 ` [PATCH net 3/3] virtio-net: fix resources leaking when fail to allocate frag skb Jason Wang
@ 2013-11-20 10:34 ` Michael S. Tsirkin
2013-11-20 12:08 ` Jason Wang
2013-11-20 13:26 ` [PATCH RFC] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
3 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2013-11-20 10:34 UTC (permalink / raw)
To: Jason Wang
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet, Shirley Ma
On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
> When mergeable buffer were used, we only put the first page buf leave the rest
> of buffers in the virt queue. This will cause the driver could not get the
> correct head buffer any more. Fix this by dropping the rest of buffers for this
> packet.
>
> The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> (virtio_net: Defer skb allocation in receive path).
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Michael Dalton <mwdalton@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Shirley Ma <xma@us.ibm.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> This patch was needed for stable
> ---
> drivers/net/virtio_net.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7bab4de..24fd502 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -222,6 +222,17 @@ static void skb_xmit_done(struct virtqueue *vq)
> netif_wake_subqueue(vi->dev, vq2txq(vq));
> }
>
> +static void drop_mergeable_buffer(struct receive_queue *rq, int num_buf)
> +{
> + char *buf;
> + int len;
> +
> + while (--num_buf && (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
> + --rq->num;
> + put_page(virt_to_head_page(buf));
> + }
> +}
> +
This is the same code we have in receive_mergeable anyway.
So let's reuse that.
> /* Called from bottom half context */
> static struct sk_buff *page_to_skb(struct receive_queue *rq,
> struct page *page, unsigned int offset,
> @@ -237,8 +248,13 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
>
> /* copy small packet so we can reuse these pages for small data */
> skb = netdev_alloc_skb_ip_align(vi->dev, GOOD_COPY_LEN);
> - if (unlikely(!skb))
> + if (unlikely(!skb)) {
> + if (vi->mergeable_rx_bufs) {
> + hdr = (struct skb_vnet_hdr *)p;
> + drop_mergeable_buffer(rq, hdr->mhdr.num_buffers);
> + }
> return NULL;
> + }
>
> hdr = skb_vnet_hdr(skb);
>
> --
> 1.8.3.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 2/3] virtio-net: fix num calculation on frag skb allocation failure
2013-11-20 9:07 ` [PATCH net 2/3] virtio-net: fix num calculation on frag skb allocation failure Jason Wang
@ 2013-11-20 10:37 ` Michael S. Tsirkin
2013-11-20 12:08 ` Jason Wang
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2013-11-20 10:37 UTC (permalink / raw)
To: Jason Wang
Cc: Michael Dalton, netdev, linux-kernel, virtualization, Eric Dumazet
On Wed, Nov 20, 2013 at 05:07:26PM +0800, Jason Wang wrote:
> We need decrease the rq->num after we can get a buf through
> virtqueue_get_buf() even if we could not allocate frag skb. Otherwise, the
> refill routine won't be triggered under heavy memory stress since the driver may
> still think there's enough room.
>
> This bug was introduced by commit 2613af0ed18a11d5c566a81f9a6510b73180660a
> (virtio_net: migrate mergeable rx buffers to page frag allocators).
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Michael Dalton <mwdalton@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
So let's wrap virtqueue_get_buf to make sure we get it right?
> ---
> The patch was needed for 3.12 stable.
> ---
> drivers/net/virtio_net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 24fd502..de1d6ca 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -333,6 +333,7 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
> head_skb->dev->stats.rx_length_errors++;
> return -EINVAL;
> }
> + --rq->num;
> if (unlikely(len > MERGE_BUFFER_LEN)) {
> pr_debug("%s: rx error: merge buffer too long\n",
> head_skb->dev->name);
> @@ -367,7 +368,6 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
> skb_add_rx_frag(curr_skb, num_skb_frags, page,
> offset, len, MERGE_BUFFER_LEN);
> }
> - --rq->num;
> }
> return 0;
> }
> --
> 1.8.3.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb
2013-11-20 10:34 ` [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb Michael S. Tsirkin
@ 2013-11-20 12:08 ` Jason Wang
2013-11-20 13:27 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Jason Wang @ 2013-11-20 12:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet, Shirley Ma
----- 原始邮件 -----
> On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
> > When mergeable buffer were used, we only put the first page buf leave the
> > rest
> > of buffers in the virt queue. This will cause the driver could not get the
> > correct head buffer any more. Fix this by dropping the rest of buffers for
> > this
> > packet.
> >
> > The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> > (virtio_net: Defer skb allocation in receive path).
> >
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Michael Dalton <mwdalton@google.com>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Shirley Ma <xma@us.ibm.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> > This patch was needed for stable
> > ---
> > drivers/net/virtio_net.c | 18 +++++++++++++++++-
> > 1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 7bab4de..24fd502 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -222,6 +222,17 @@ static void skb_xmit_done(struct virtqueue *vq)
> > netif_wake_subqueue(vi->dev, vq2txq(vq));
> > }
> >
> > +static void drop_mergeable_buffer(struct receive_queue *rq, int num_buf)
> > +{
> > + char *buf;
> > + int len;
> > +
> > + while (--num_buf && (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
> > + --rq->num;
> > + put_page(virt_to_head_page(buf));
> > + }
> > +}
> > +
>
> This is the same code we have in receive_mergeable anyway.
> So let's reuse that.
>
>
receive_mergeable() was called after page_to_skb() was called and
there's lots of conditions check there. I'm not sure how could we
reuse them.
> > /* Called from bottom half context */
> > static struct sk_buff *page_to_skb(struct receive_queue *rq,
> > struct page *page, unsigned int offset,
> > @@ -237,8 +248,13 @@ static struct sk_buff *page_to_skb(struct
> > receive_queue *rq,
> >
> > /* copy small packet so we can reuse these pages for small data */
> > skb = netdev_alloc_skb_ip_align(vi->dev, GOOD_COPY_LEN);
> > - if (unlikely(!skb))
> > + if (unlikely(!skb)) {
> > + if (vi->mergeable_rx_bufs) {
> > + hdr = (struct skb_vnet_hdr *)p;
> > + drop_mergeable_buffer(rq, hdr->mhdr.num_buffers);
> > + }
> > return NULL;
> > + }
> >
> > hdr = skb_vnet_hdr(skb);
> >
> > --
> > 1.8.3.2
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 2/3] virtio-net: fix num calculation on frag skb allocation failure
2013-11-20 10:37 ` Michael S. Tsirkin
@ 2013-11-20 12:08 ` Jason Wang
2013-11-20 13:37 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Jason Wang @ 2013-11-20 12:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Dalton, netdev, linux-kernel, virtualization, Eric Dumazet
----- 原始邮件 -----
> On Wed, Nov 20, 2013 at 05:07:26PM +0800, Jason Wang wrote:
> > We need decrease the rq->num after we can get a buf through
> > virtqueue_get_buf() even if we could not allocate frag skb. Otherwise, the
> > refill routine won't be triggered under heavy memory stress since the
> > driver may
> > still think there's enough room.
> >
> > This bug was introduced by commit 2613af0ed18a11d5c566a81f9a6510b73180660a
> > (virtio_net: migrate mergeable rx buffers to page frag allocators).
> >
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Michael Dalton <mwdalton@google.com>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> So let's wrap virtqueue_get_buf to make sure we get it right?
>
Ok. good idea.
> > ---
> > The patch was needed for 3.12 stable.
> > ---
> > drivers/net/virtio_net.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 24fd502..de1d6ca 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -333,6 +333,7 @@ static int receive_mergeable(struct receive_queue *rq,
> > struct sk_buff *head_skb)
> > head_skb->dev->stats.rx_length_errors++;
> > return -EINVAL;
> > }
> > + --rq->num;
> > if (unlikely(len > MERGE_BUFFER_LEN)) {
> > pr_debug("%s: rx error: merge buffer too long\n",
> > head_skb->dev->name);
> > @@ -367,7 +368,6 @@ static int receive_mergeable(struct receive_queue *rq,
> > struct sk_buff *head_skb)
> > skb_add_rx_frag(curr_skb, num_skb_frags, page,
> > offset, len, MERGE_BUFFER_LEN);
> > }
> > - --rq->num;
> > }
> > return 0;
> > }
> > --
> > 1.8.3.2
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RFC] virtio_net: fix error handling for mergeable buffers
2013-11-20 9:07 [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb Jason Wang
` (2 preceding siblings ...)
2013-11-20 10:34 ` [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb Michael S. Tsirkin
@ 2013-11-20 13:26 ` Michael S. Tsirkin
2013-11-20 13:54 ` Jason Wang
3 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2013-11-20 13:26 UTC (permalink / raw)
To: Jason Wang
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet, Shirley Ma
On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
> When mergeable buffer were used, we only put the first page buf leave the rest
> of buffers in the virt queue. This will cause the driver could not get the
> correct head buffer any more. Fix this by dropping the rest of buffers for this
> packet.
>
> The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> (virtio_net: Defer skb allocation in receive path).
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Michael Dalton <mwdalton@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Shirley Ma <xma@us.ibm.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Just to clarify my previous comment: it was not about the
idea of adding drop_mergeable_buffer - rather, I think that
adding knowledge about mergeable buffers into page_to_skb creates an
ugly internal API.
Let's move the call to page_to_skb within receive_mergeable instead:
it's also nice that int offset = buf - page_address(page) logic
is not spread around like it was.
Also, it's not nice that we ignore length errors when we drop
packets because of OOM.
So I came up with the following - it seems to work but I didn't
stress test yet.
commit ebffb3fe4335ffe07124e4518e76d6e05844fa18
Author: Michael S. Tsirkin <mst@redhat.com>
Date: Wed Nov 20 14:41:29 2013 +0200
virtio_net: fix error handling for mergeable buffers
Eric Dumazet noticed that if we encounter an error
when processing a mergeable buffer, we don't
dequeue all of the buffers from this packet,
the result is almost sure to be loss of networking.
Jason Wang noticed that we also leak a page and that we don't decrement
the rq buf count, so we won't repost buffers (a resource leak).
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael Dalton <mwdalton@google.com>
Reported-by: Eric Dumazet <edumazet@google.com>
Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 01f4eb5..42f6a1e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -296,41 +296,53 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
return skb;
}
-static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
+static struct sk_buff *receive_mergeable(struct net_device *dev,
+ struct receive_queue *rq,
+ void *buf,
+ unsigned int len)
{
- struct skb_vnet_hdr *hdr = skb_vnet_hdr(head_skb);
+ struct skb_vnet_hdr *hdr = buf;
+ int num_buf = hdr->mhdr.num_buffers;
+ struct page *page = virt_to_head_page(buf);
+ int offset = buf - page_address(page);
+ struct sk_buff *head_skb = page_to_skb(rq, page, offset, len,
+ MAX_PACKET_LEN);
struct sk_buff *curr_skb = head_skb;
- char *buf;
- struct page *page;
- int num_buf, len, offset;
- num_buf = hdr->mhdr.num_buffers;
- while (--num_buf) {
- int num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
+ if (unlikely(!curr_skb))
+ goto err_skb;
+
+ while (--num_buf) {
+ int num_skb_frags;
+
buf = virtqueue_get_buf(rq->vq, &len);
if (unlikely(!buf)) {
- pr_debug("%s: rx error: %d buffers missing\n",
- head_skb->dev->name, hdr->mhdr.num_buffers);
- head_skb->dev->stats.rx_length_errors++;
- return -EINVAL;
+ pr_debug("%s: rx error: %d buffers out of %d missing\n",
+ dev->name, num_buf, hdr->mhdr.num_buffers);
+ dev->stats.rx_length_errors++;
+ goto err_buf;
}
if (unlikely(len > MAX_PACKET_LEN)) {
pr_debug("%s: rx error: merge buffer too long\n",
- head_skb->dev->name);
+ dev->name);
len = MAX_PACKET_LEN;
}
+
+ page = virt_to_head_page(buf);
+ --rq->num;
+
+ num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
- if (unlikely(!nskb)) {
- head_skb->dev->stats.rx_dropped++;
- return -ENOMEM;
- }
+
+ if (unlikely(!nskb))
+ goto err_skb;
if (curr_skb == head_skb)
skb_shinfo(curr_skb)->frag_list = nskb;
else
curr_skb->next = nskb;
- curr_skb = nskb;
head_skb->truesize += nskb->truesize;
+ curr_skb = nskb;
num_skb_frags = 0;
}
if (curr_skb != head_skb) {
@@ -338,8 +350,7 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
head_skb->len += len;
head_skb->truesize += MAX_PACKET_LEN;
}
- page = virt_to_head_page(buf);
- offset = buf - (char *)page_address(page);
+ offset = buf - page_address(page);
if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
put_page(page);
skb_coalesce_rx_frag(curr_skb, num_skb_frags - 1,
@@ -349,9 +360,28 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
offset, len,
MAX_PACKET_LEN);
}
+ }
+
+ return head_skb;
+
+err_skb:
+ put_page(page);
+err_buf:
+ dev->stats.rx_dropped++;
+ dev_kfree_skb(head_skb);
+ while (--num_buf) {
+ buf = virtqueue_get_buf(rq->vq, &len);
+ if (unlikely(!buf)) {
+ pr_debug("%s: rx error: %d buffers missing\n",
+ dev->name, num_buf);
+ dev->stats.rx_length_errors++;
+ break;
+ }
+ page = virt_to_head_page(buf);
+ put_page(page);
--rq->num;
}
- return 0;
+ return NULL;
}
static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
@@ -380,19 +410,9 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
len -= sizeof(struct virtio_net_hdr);
skb_trim(skb, len);
} else if (vi->mergeable_rx_bufs) {
- struct page *page = virt_to_head_page(buf);
- skb = page_to_skb(rq, page,
- (char *)buf - (char *)page_address(page),
- len, MAX_PACKET_LEN);
- if (unlikely(!skb)) {
- dev->stats.rx_dropped++;
- put_page(page);
+ skb = receive_mergeable(dev, rq, buf, len);
+ if (unlikely(!skb))
return;
- }
- if (receive_mergeable(rq, skb)) {
- dev_kfree_skb(skb);
- return;
- }
} else {
page = buf;
skb = page_to_skb(rq, page, 0, len, PAGE_SIZE);
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb
2013-11-20 12:08 ` Jason Wang
@ 2013-11-20 13:27 ` Michael S. Tsirkin
0 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2013-11-20 13:27 UTC (permalink / raw)
To: Jason Wang
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet, Shirley Ma
On Wed, Nov 20, 2013 at 07:08:02AM -0500, Jason Wang wrote:
>
>
> ----- 原始邮件 -----
> > On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
> > > When mergeable buffer were used, we only put the first page buf leave the
> > > rest
> > > of buffers in the virt queue. This will cause the driver could not get the
> > > correct head buffer any more. Fix this by dropping the rest of buffers for
> > > this
> > > packet.
> > >
> > > The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> > > (virtio_net: Defer skb allocation in receive path).
> > >
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Michael Dalton <mwdalton@google.com>
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Cc: Shirley Ma <xma@us.ibm.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > > This patch was needed for stable
> > > ---
> > > drivers/net/virtio_net.c | 18 +++++++++++++++++-
> > > 1 file changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 7bab4de..24fd502 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -222,6 +222,17 @@ static void skb_xmit_done(struct virtqueue *vq)
> > > netif_wake_subqueue(vi->dev, vq2txq(vq));
> > > }
> > >
> > > +static void drop_mergeable_buffer(struct receive_queue *rq, int num_buf)
> > > +{
> > > + char *buf;
> > > + int len;
> > > +
> > > + while (--num_buf && (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
> > > + --rq->num;
> > > + put_page(virt_to_head_page(buf));
> > > + }
> > > +}
> > > +
> >
> > This is the same code we have in receive_mergeable anyway.
> > So let's reuse that.
> >
> >
>
> receive_mergeable() was called after page_to_skb() was called and
> there's lots of conditions check there. I'm not sure how could we
> reuse them.
I posted a patch showing how :)
> > > /* Called from bottom half context */
> > > static struct sk_buff *page_to_skb(struct receive_queue *rq,
> > > struct page *page, unsigned int offset,
> > > @@ -237,8 +248,13 @@ static struct sk_buff *page_to_skb(struct
> > > receive_queue *rq,
> > >
> > > /* copy small packet so we can reuse these pages for small data */
> > > skb = netdev_alloc_skb_ip_align(vi->dev, GOOD_COPY_LEN);
> > > - if (unlikely(!skb))
> > > + if (unlikely(!skb)) {
> > > + if (vi->mergeable_rx_bufs) {
> > > + hdr = (struct skb_vnet_hdr *)p;
> > > + drop_mergeable_buffer(rq, hdr->mhdr.num_buffers);
> > > + }
> > > return NULL;
> > > + }
> > >
> > > hdr = skb_vnet_hdr(skb);
> > >
> > > --
> > > 1.8.3.2
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 2/3] virtio-net: fix num calculation on frag skb allocation failure
2013-11-20 12:08 ` Jason Wang
@ 2013-11-20 13:37 ` Michael S. Tsirkin
2013-11-20 13:56 ` Jason Wang
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2013-11-20 13:37 UTC (permalink / raw)
To: Jason Wang
Cc: Michael Dalton, netdev, linux-kernel, virtualization, Eric Dumazet
On Wed, Nov 20, 2013 at 07:08:50AM -0500, Jason Wang wrote:
>
>
> ----- 原始邮件 -----
> > On Wed, Nov 20, 2013 at 05:07:26PM +0800, Jason Wang wrote:
> > > We need decrease the rq->num after we can get a buf through
> > > virtqueue_get_buf() even if we could not allocate frag skb. Otherwise, the
> > > refill routine won't be triggered under heavy memory stress since the
> > > driver may
> > > still think there's enough room.
> > >
> > > This bug was introduced by commit 2613af0ed18a11d5c566a81f9a6510b73180660a
> > > (virtio_net: migrate mergeable rx buffers to page frag allocators).
> > >
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Michael Dalton <mwdalton@google.com>
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > So let's wrap virtqueue_get_buf to make sure we get it right?
> >
>
> Ok. good idea.
So I did this (below) but the compiler is not smart enough to
avoid two branches on data path.
So I'm not sure anymore: with my patch it's pretty clear how
the code works.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
but I don't think we need to apply this.
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 11d9cc9..1cc2e43 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -296,6 +296,14 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
return skb;
}
+static void *rq_get_buf(struct receive_queue *rq, unsigned int *len)
+{
+ void *buf = virtqueue_get_buf(rq->vq, len);
+ if (buf)
+ rq->num--;
+ return buf;
+}
+
static struct sk_buff *receive_mergeable(struct net_device *dev,
struct receive_queue *rq,
void *buf,
@@ -315,7 +323,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
while (--num_buf) {
int num_skb_frags;
- buf = virtqueue_get_buf(rq->vq, &len);
+ buf = rq_get_buf(rq, &len);
if (unlikely(!buf)) {
pr_debug("%s: rx error: %d buffers out of %d missing\n",
dev->name, num_buf, hdr->mhdr.num_buffers);
@@ -329,7 +337,6 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
}
page = virt_to_head_page(buf);
- --rq->num;
num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
@@ -370,7 +377,7 @@ err_buf:
dev->stats.rx_dropped++;
dev_kfree_skb(head_skb);
while (--num_buf) {
- buf = virtqueue_get_buf(rq->vq, &len);
+ buf = rq_get_buf(rq, &len);
if (unlikely(!buf)) {
pr_debug("%s: rx error: %d buffers missing\n",
dev->name, num_buf);
@@ -379,7 +386,6 @@ err_buf:
}
page = virt_to_head_page(buf);
put_page(page);
- --rq->num;
}
return NULL;
}
@@ -675,9 +681,8 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
again:
while (received < budget &&
- (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
+ (buf = rq_get_buf(rq, &len)) != NULL) {
receive_buf(rq, buf, len);
- --rq->num;
received++;
}
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] virtio_net: fix error handling for mergeable buffers
2013-11-20 13:26 ` [PATCH RFC] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
@ 2013-11-20 13:54 ` Jason Wang
2013-11-20 14:20 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Jason Wang @ 2013-11-20 13:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet, Shirley Ma
----- 原始邮件 -----
> On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
> > When mergeable buffer were used, we only put the first page buf leave the
> > rest
> > of buffers in the virt queue. This will cause the driver could not get the
> > correct head buffer any more. Fix this by dropping the rest of buffers for
> > this
> > packet.
> >
> > The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> > (virtio_net: Defer skb allocation in receive path).
> >
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Michael Dalton <mwdalton@google.com>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Shirley Ma <xma@us.ibm.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> Just to clarify my previous comment: it was not about the
> idea of adding drop_mergeable_buffer - rather, I think that
> adding knowledge about mergeable buffers into page_to_skb creates an
> ugly internal API.
>
> Let's move the call to page_to_skb within receive_mergeable instead:
> it's also nice that int offset = buf - page_address(page) logic
> is not spread around like it was.
>
> Also, it's not nice that we ignore length errors when we drop
> packets because of OOM.
>
> So I came up with the following - it seems to work but I didn't
> stress test yet.
I've no objection on this. But I've rather like my small and direct patch
to be applied to -net first. It has lower risk and was much more easier to
be backported to stable trees. Then we can do the re-factor like this in
net-next.
>
> commit ebffb3fe4335ffe07124e4518e76d6e05844fa18
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date: Wed Nov 20 14:41:29 2013 +0200
>
> virtio_net: fix error handling for mergeable buffers
>
> Eric Dumazet noticed that if we encounter an error
> when processing a mergeable buffer, we don't
> dequeue all of the buffers from this packet,
> the result is almost sure to be loss of networking.
>
> Jason Wang noticed that we also leak a page and that we don't decrement
> the rq buf count, so we won't repost buffers (a resource leak).
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael Dalton <mwdalton@google.com>
> Reported-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 01f4eb5..42f6a1e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -296,41 +296,53 @@ static struct sk_buff *page_to_skb(struct receive_queue
> *rq,
> return skb;
> }
>
> -static int receive_mergeable(struct receive_queue *rq, struct sk_buff
> *head_skb)
> +static struct sk_buff *receive_mergeable(struct net_device *dev,
> + struct receive_queue *rq,
> + void *buf,
> + unsigned int len)
> {
> - struct skb_vnet_hdr *hdr = skb_vnet_hdr(head_skb);
> + struct skb_vnet_hdr *hdr = buf;
> + int num_buf = hdr->mhdr.num_buffers;
> + struct page *page = virt_to_head_page(buf);
> + int offset = buf - page_address(page);
> + struct sk_buff *head_skb = page_to_skb(rq, page, offset, len,
> + MAX_PACKET_LEN);
> struct sk_buff *curr_skb = head_skb;
> - char *buf;
> - struct page *page;
> - int num_buf, len, offset;
>
> - num_buf = hdr->mhdr.num_buffers;
> - while (--num_buf) {
> - int num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
> + if (unlikely(!curr_skb))
> + goto err_skb;
> +
> + while (--num_buf) {
> + int num_skb_frags;
> +
> buf = virtqueue_get_buf(rq->vq, &len);
> if (unlikely(!buf)) {
> - pr_debug("%s: rx error: %d buffers missing\n",
> - head_skb->dev->name, hdr->mhdr.num_buffers);
> - head_skb->dev->stats.rx_length_errors++;
> - return -EINVAL;
> + pr_debug("%s: rx error: %d buffers out of %d missing\n",
> + dev->name, num_buf, hdr->mhdr.num_buffers);
> + dev->stats.rx_length_errors++;
> + goto err_buf;
> }
> if (unlikely(len > MAX_PACKET_LEN)) {
> pr_debug("%s: rx error: merge buffer too long\n",
> - head_skb->dev->name);
> + dev->name);
> len = MAX_PACKET_LEN;
> }
> +
> + page = virt_to_head_page(buf);
> + --rq->num;
> +
> + num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
> if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
> struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
> - if (unlikely(!nskb)) {
> - head_skb->dev->stats.rx_dropped++;
> - return -ENOMEM;
> - }
> +
> + if (unlikely(!nskb))
> + goto err_skb;
> if (curr_skb == head_skb)
> skb_shinfo(curr_skb)->frag_list = nskb;
> else
> curr_skb->next = nskb;
> - curr_skb = nskb;
> head_skb->truesize += nskb->truesize;
> + curr_skb = nskb;
> num_skb_frags = 0;
> }
> if (curr_skb != head_skb) {
> @@ -338,8 +350,7 @@ static int receive_mergeable(struct receive_queue *rq,
> struct sk_buff *head_skb)
> head_skb->len += len;
> head_skb->truesize += MAX_PACKET_LEN;
> }
> - page = virt_to_head_page(buf);
> - offset = buf - (char *)page_address(page);
> + offset = buf - page_address(page);
> if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
> put_page(page);
> skb_coalesce_rx_frag(curr_skb, num_skb_frags - 1,
> @@ -349,9 +360,28 @@ static int receive_mergeable(struct receive_queue *rq,
> struct sk_buff *head_skb)
> offset, len,
> MAX_PACKET_LEN);
> }
> + }
> +
> + return head_skb;
> +
> +err_skb:
> + put_page(page);
> +err_buf:
> + dev->stats.rx_dropped++;
> + dev_kfree_skb(head_skb);
> + while (--num_buf) {
> + buf = virtqueue_get_buf(rq->vq, &len);
> + if (unlikely(!buf)) {
> + pr_debug("%s: rx error: %d buffers missing\n",
> + dev->name, num_buf);
> + dev->stats.rx_length_errors++;
> + break;
> + }
> + page = virt_to_head_page(buf);
> + put_page(page);
> --rq->num;
> }
> - return 0;
> + return NULL;
> }
>
> static void receive_buf(struct receive_queue *rq, void *buf, unsigned int
> len)
> @@ -380,19 +410,9 @@ static void receive_buf(struct receive_queue *rq, void
> *buf, unsigned int len)
> len -= sizeof(struct virtio_net_hdr);
> skb_trim(skb, len);
> } else if (vi->mergeable_rx_bufs) {
> - struct page *page = virt_to_head_page(buf);
> - skb = page_to_skb(rq, page,
> - (char *)buf - (char *)page_address(page),
> - len, MAX_PACKET_LEN);
> - if (unlikely(!skb)) {
> - dev->stats.rx_dropped++;
> - put_page(page);
> + skb = receive_mergeable(dev, rq, buf, len);
> + if (unlikely(!skb))
> return;
> - }
> - if (receive_mergeable(rq, skb)) {
> - dev_kfree_skb(skb);
> - return;
> - }
> } else {
> page = buf;
> skb = page_to_skb(rq, page, 0, len, PAGE_SIZE);
> --
> 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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net 2/3] virtio-net: fix num calculation on frag skb allocation failure
2013-11-20 13:37 ` Michael S. Tsirkin
@ 2013-11-20 13:56 ` Jason Wang
0 siblings, 0 replies; 14+ messages in thread
From: Jason Wang @ 2013-11-20 13:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Dalton, netdev, linux-kernel, virtualization, Eric Dumazet
----- 原始邮件 -----
> On Wed, Nov 20, 2013 at 07:08:50AM -0500, Jason Wang wrote:
> >
> >
> > ----- 原始邮件 -----
> > > On Wed, Nov 20, 2013 at 05:07:26PM +0800, Jason Wang wrote:
> > > > We need decrease the rq->num after we can get a buf through
> > > > virtqueue_get_buf() even if we could not allocate frag skb. Otherwise,
> > > > the
> > > > refill routine won't be triggered under heavy memory stress since the
> > > > driver may
> > > > still think there's enough room.
> > > >
> > > > This bug was introduced by commit
> > > > 2613af0ed18a11d5c566a81f9a6510b73180660a
> > > > (virtio_net: migrate mergeable rx buffers to page frag allocators).
> > > >
> > > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Michael Dalton <mwdalton@google.com>
> > > > Cc: Eric Dumazet <edumazet@google.com>
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >
> > > So let's wrap virtqueue_get_buf to make sure we get it right?
> > >
> >
> > Ok. good idea.
>
> So I did this (below) but the compiler is not smart enough to
> avoid two branches on data path.
> So I'm not sure anymore: with my patch it's pretty clear how
> the code works.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> but I don't think we need to apply this.
>
True, another point is we'd better to handle both increasing and decreasing in the
same way. We do not increase it in a helper.
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 11d9cc9..1cc2e43 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -296,6 +296,14 @@ static struct sk_buff *page_to_skb(struct receive_queue
> *rq,
> return skb;
> }
>
> +static void *rq_get_buf(struct receive_queue *rq, unsigned int *len)
> +{
> + void *buf = virtqueue_get_buf(rq->vq, len);
> + if (buf)
> + rq->num--;
> + return buf;
> +}
> +
> static struct sk_buff *receive_mergeable(struct net_device *dev,
> struct receive_queue *rq,
> void *buf,
> @@ -315,7 +323,7 @@ static struct sk_buff *receive_mergeable(struct
> net_device *dev,
> while (--num_buf) {
> int num_skb_frags;
>
> - buf = virtqueue_get_buf(rq->vq, &len);
> + buf = rq_get_buf(rq, &len);
> if (unlikely(!buf)) {
> pr_debug("%s: rx error: %d buffers out of %d missing\n",
> dev->name, num_buf, hdr->mhdr.num_buffers);
> @@ -329,7 +337,6 @@ static struct sk_buff *receive_mergeable(struct
> net_device *dev,
> }
>
> page = virt_to_head_page(buf);
> - --rq->num;
>
> num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
> if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
> @@ -370,7 +377,7 @@ err_buf:
> dev->stats.rx_dropped++;
> dev_kfree_skb(head_skb);
> while (--num_buf) {
> - buf = virtqueue_get_buf(rq->vq, &len);
> + buf = rq_get_buf(rq, &len);
> if (unlikely(!buf)) {
> pr_debug("%s: rx error: %d buffers missing\n",
> dev->name, num_buf);
> @@ -379,7 +386,6 @@ err_buf:
> }
> page = virt_to_head_page(buf);
> put_page(page);
> - --rq->num;
> }
> return NULL;
> }
> @@ -675,9 +681,8 @@ static int virtnet_poll(struct napi_struct *napi, int
> budget)
>
> again:
> while (received < budget &&
> - (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
> + (buf = rq_get_buf(rq, &len)) != NULL) {
> receive_buf(rq, buf, len);
> - --rq->num;
> received++;
> }
>
> --
> 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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] virtio_net: fix error handling for mergeable buffers
2013-11-20 13:54 ` Jason Wang
@ 2013-11-20 14:20 ` Michael S. Tsirkin
2013-11-21 3:27 ` Jason Wang
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2013-11-20 14:20 UTC (permalink / raw)
To: Jason Wang
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet, Shirley Ma
On Wed, Nov 20, 2013 at 08:54:02AM -0500, Jason Wang wrote:
>
>
> ----- 原始邮件 -----
> > On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
> > > When mergeable buffer were used, we only put the first page buf leave the
> > > rest
> > > of buffers in the virt queue. This will cause the driver could not get the
> > > correct head buffer any more. Fix this by dropping the rest of buffers for
> > > this
> > > packet.
> > >
> > > The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> > > (virtio_net: Defer skb allocation in receive path).
> > >
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Michael Dalton <mwdalton@google.com>
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Cc: Shirley Ma <xma@us.ibm.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > Just to clarify my previous comment: it was not about the
> > idea of adding drop_mergeable_buffer - rather, I think that
> > adding knowledge about mergeable buffers into page_to_skb creates an
> > ugly internal API.
> >
> > Let's move the call to page_to_skb within receive_mergeable instead:
> > it's also nice that int offset = buf - page_address(page) logic
> > is not spread around like it was.
> >
> > Also, it's not nice that we ignore length errors when we drop
> > packets because of OOM.
> >
> > So I came up with the following - it seems to work but I didn't
> > stress test yet.
>
> I've no objection on this. But I've rather like my small and direct patch
> to be applied to -net first. It has lower risk and was much more easier to
> be backported to stable trees. Then we can do the re-factor like this in
> net-next.
It makes the interfaces too messy. We are not in code freeze in -net -
only feature freeze, so no reason to make code like spagetty,
and it's only 25 lines changed as compared to 40.
It's not a huge refactoring.
It's just as easy to backport my patch too.
You just drop the goto in the new code path we added.
Let me show you (untested) - your patch is not smaller.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
commit 9b442fe970d5c71311d4314edef26ee2eb16e7fb
Author: Michael S. Tsirkin <mst@redhat.com>
Date: Wed Nov 20 12:44:14 2013 +0200
virtio_net: fix resource leak on alloc failure
virtio net got confused, started dropping packets.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9fbdfcd..df4b9d0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -297,13 +297,22 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
return skb;
}
-static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
+static struct sk_buff *receive_mergeable(struct net_device *dev,
+ struct receive_queue *rq,
+ struct page *page,
+ unsigned int len)
{
- struct skb_vnet_hdr *hdr = skb_vnet_hdr(skb);
- struct page *page;
- int num_buf, i, len;
+ struct skb_vnet_hdr *hdr = page_address(buf);
+ int num_buf = hdr->mhdr.num_buffers;
+ struct sk_buff *skb = page_to_skb(rq, page, len);
+ int i;
+
+ skb = page_to_skb(rq, page, len);
+
+ if (unlikely(!skb))
+ goto err_skb;
+
- num_buf = hdr->mhdr.num_buffers;
while (--num_buf) {
i = skb_shinfo(skb)->nr_frags;
if (i >= MAX_SKB_FRAGS) {
@@ -313,10 +322,10 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
}
page = virtqueue_get_buf(rq->vq, &len);
if (!page) {
- pr_debug("%s: rx error: %d buffers missing\n",
- skb->dev->name, hdr->mhdr.num_buffers);
- skb->dev->stats.rx_length_errors++;
- return -EINVAL;
+ pr_debug("%s: rx error: %d buffers %d missing\n",
+ dev->name, hdr->mhdr.num_buffers, num_buf);
+ dev->stats.rx_length_errors++;
+ goto err_buf;
}
if (len > PAGE_SIZE)
@@ -326,7 +335,25 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
--rq->num;
}
- return 0;
+ return skb;
+err_skb:
+ put_page(page);
+err_buf:
+ dev->stats.rx_dropped++;
+ dev_kfree_skb(head_skb);
+ while (--num_buf) {
+ buf = virtqueue_get_buf(rq->vq, &len);
+ if (unlikely(!buf)) {
+ pr_debug("%s: rx error: %d buffers missing\n",
+ dev->name, num_buf);
+ dev->stats.rx_length_errors++;
+ break;
+ }
+ page = buf;
+ give_pages(rq, page);
+ --rq->num;
+ }
+ return NULL;
}
static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
@@ -354,17 +381,18 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
skb_trim(skb, len);
} else {
page = buf;
- skb = page_to_skb(rq, page, len);
- if (unlikely(!skb)) {
- dev->stats.rx_dropped++;
- give_pages(rq, page);
- return;
- }
- if (vi->mergeable_rx_bufs)
- if (receive_mergeable(rq, skb)) {
- dev_kfree_skb(skb);
+ if (vi->mergeable_rx_bufs) {
+ skb = receive_mergeable(dev, rq, page, len);
+ if (unlikely(!skb))
+ return;
+ } else {
+ skb = page_to_skb(rq, page, len);
+ if (unlikely(!skb)) {
+ dev->stats.rx_dropped++;
+ give_pages(rq, page);
return;
}
+ }
}
hdr = skb_vnet_hdr(skb);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC] virtio_net: fix error handling for mergeable buffers
2013-11-20 14:20 ` Michael S. Tsirkin
@ 2013-11-21 3:27 ` Jason Wang
0 siblings, 0 replies; 14+ messages in thread
From: Jason Wang @ 2013-11-21 3:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet, Shirley Ma
On 11/20/2013 10:20 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 20, 2013 at 08:54:02AM -0500, Jason Wang wrote:
>>
>> ----- 原始邮件 -----
>>> On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
>>>> When mergeable buffer were used, we only put the first page buf leave the
>>>> rest
>>>> of buffers in the virt queue. This will cause the driver could not get the
>>>> correct head buffer any more. Fix this by dropping the rest of buffers for
>>>> this
>>>> packet.
>>>>
>>>> The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
>>>> (virtio_net: Defer skb allocation in receive path).
>>>>
>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> Cc: Michael Dalton <mwdalton@google.com>
>>>> Cc: Eric Dumazet <edumazet@google.com>
>>>> Cc: Shirley Ma <xma@us.ibm.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> Just to clarify my previous comment: it was not about the
>>> idea of adding drop_mergeable_buffer - rather, I think that
>>> adding knowledge about mergeable buffers into page_to_skb creates an
>>> ugly internal API.
>>>
>>> Let's move the call to page_to_skb within receive_mergeable instead:
>>> it's also nice that int offset = buf - page_address(page) logic
>>> is not spread around like it was.
>>>
>>> Also, it's not nice that we ignore length errors when we drop
>>> packets because of OOM.
>>>
>>> So I came up with the following - it seems to work but I didn't
>>> stress test yet.
>> I've no objection on this. But I've rather like my small and direct patch
>> to be applied to -net first. It has lower risk and was much more easier to
>> be backported to stable trees. Then we can do the re-factor like this in
>> net-next.
> It makes the interfaces too messy.
Do you mean receive_mergeable() should call page_to_skb()? It has been
there several years. And even with your changes, for big and small
packets, page_to_skb() were still called directly receive_buf() and the
error handling were done there.
> We are not in code freeze in -net -
> only feature freeze, so no reason to make code like spagetty,
> and it's only 25 lines changed as compared to 40.
> It's not a huge refactoring.
The problem is your patch mixes several bugs fixing with re-factoring,
only one of the bug fixing was really needed for stable. At least we
should make incremental patches on this.
>
> It's just as easy to backport my patch too.
> You just drop the goto in the new code path we added.
It may not be so easy for the people who are not familiar with
virtio-net and the s/skb->dev/dev/g changes looks irrelevant here.
> Let me show you (untested) - your patch is not smaller.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
>
> commit 9b442fe970d5c71311d4314edef26ee2eb16e7fb
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date: Wed Nov 20 12:44:14 2013 +0200
>
> virtio_net: fix resource leak on alloc failure
>
> virtio net got confused, started dropping packets.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 9fbdfcd..df4b9d0 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -297,13 +297,22 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
> return skb;
> }
>
> -static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
> +static struct sk_buff *receive_mergeable(struct net_device *dev,
> + struct receive_queue *rq,
> + struct page *page,
> + unsigned int len)
> {
> - struct skb_vnet_hdr *hdr = skb_vnet_hdr(skb);
> - struct page *page;
> - int num_buf, i, len;
> + struct skb_vnet_hdr *hdr = page_address(buf);
> + int num_buf = hdr->mhdr.num_buffers;
> + struct sk_buff *skb = page_to_skb(rq, page, len);
> + int i;
> +
> + skb = page_to_skb(rq, page, len);
> +
> + if (unlikely(!skb))
> + goto err_skb;
> +
>
> - num_buf = hdr->mhdr.num_buffers;
> while (--num_buf) {
> i = skb_shinfo(skb)->nr_frags;
> if (i >= MAX_SKB_FRAGS) {
> @@ -313,10 +322,10 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
> }
> page = virtqueue_get_buf(rq->vq, &len);
> if (!page) {
> - pr_debug("%s: rx error: %d buffers missing\n",
> - skb->dev->name, hdr->mhdr.num_buffers);
> - skb->dev->stats.rx_length_errors++;
> - return -EINVAL;
> + pr_debug("%s: rx error: %d buffers %d missing\n",
> + dev->name, hdr->mhdr.num_buffers, num_buf);
> + dev->stats.rx_length_errors++;
> + goto err_buf;
> }
>
> if (len > PAGE_SIZE)
> @@ -326,7 +335,25 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
>
> --rq->num;
> }
> - return 0;
> + return skb;
> +err_skb:
> + put_page(page);
> +err_buf:
> + dev->stats.rx_dropped++;
> + dev_kfree_skb(head_skb);
> + while (--num_buf) {
> + buf = virtqueue_get_buf(rq->vq, &len);
> + if (unlikely(!buf)) {
> + pr_debug("%s: rx error: %d buffers missing\n",
> + dev->name, num_buf);
> + dev->stats.rx_length_errors++;
> + break;
> + }
> + page = buf;
> + give_pages(rq, page);
> + --rq->num;
> + }
> + return NULL;
> }
>
> static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> @@ -354,17 +381,18 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> skb_trim(skb, len);
> } else {
> page = buf;
> - skb = page_to_skb(rq, page, len);
> - if (unlikely(!skb)) {
> - dev->stats.rx_dropped++;
> - give_pages(rq, page);
> - return;
> - }
> - if (vi->mergeable_rx_bufs)
> - if (receive_mergeable(rq, skb)) {
> - dev_kfree_skb(skb);
> + if (vi->mergeable_rx_bufs) {
> + skb = receive_mergeable(dev, rq, page, len);
> + if (unlikely(!skb))
> + return;
> + } else {
> + skb = page_to_skb(rq, page, len);
> + if (unlikely(!skb)) {
> + dev->stats.rx_dropped++;
> + give_pages(rq, page);
> return;
> }
> + }
> }
>
> hdr = skb_vnet_hdr(skb);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-11-21 3:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-20 9:07 [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb Jason Wang
2013-11-20 9:07 ` [PATCH net 2/3] virtio-net: fix num calculation on frag skb allocation failure Jason Wang
2013-11-20 10:37 ` Michael S. Tsirkin
2013-11-20 12:08 ` Jason Wang
2013-11-20 13:37 ` Michael S. Tsirkin
2013-11-20 13:56 ` Jason Wang
2013-11-20 9:07 ` [PATCH net 3/3] virtio-net: fix resources leaking when fail to allocate frag skb Jason Wang
2013-11-20 10:34 ` [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb Michael S. Tsirkin
2013-11-20 12:08 ` Jason Wang
2013-11-20 13:27 ` Michael S. Tsirkin
2013-11-20 13:26 ` [PATCH RFC] virtio_net: fix error handling for mergeable buffers Michael S. Tsirkin
2013-11-20 13:54 ` Jason Wang
2013-11-20 14:20 ` Michael S. Tsirkin
2013-11-21 3:27 ` Jason Wang
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).