[PATCH] virtio_net: Check for room in the vq before adding buffer

[PATCH] virtio_net: Check for room in the vq before adding buffer

[Amit Shah]

Saves us one cycle of alloc-add-free if the queue was full.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---

Rusty: Tested this. 256 buffers are allocated with the patch,
257 without (and one is later freed).

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

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e94f817..bb54a1f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -280,7 +280,7 @@ static bool try_fill_recv_maxbufs(struct virtnet_info *vi, gfp_t gfp)
 	bool oom = false;
 
 	sg_init_table(sg, 2+MAX_SKB_FRAGS);
-	for (;;) {
+	do {
 		struct skb_vnet_hdr *hdr;
 
 		skb = netdev_alloc_skb(vi->dev, MAX_PACKET_LEN + NET_IP_ALIGN);
@@ -323,7 +323,7 @@ static bool try_fill_recv_maxbufs(struct virtnet_info *vi, gfp_t gfp)
 			break;
 		}
 		vi->num++;
-	}
+	} while (err > 0);
 	if (unlikely(vi->num > vi->max))
 		vi->max = vi->num;
 	vi->rvq->vq_ops->kick(vi->rvq);
@@ -341,7 +341,7 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
 	if (!vi->mergeable_rx_bufs)
 		return try_fill_recv_maxbufs(vi, gfp);
 
-	for (;;) {
+	do {
 		skb_frag_t *f;
 
 		skb = netdev_alloc_skb(vi->dev, GOOD_COPY_LEN + NET_IP_ALIGN);
@@ -375,7 +375,7 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
 			break;
 		}
 		vi->num++;
-	}
+	} while (err > 0);
 	if (unlikely(vi->num > vi->max))
 		vi->max = vi->num;
 	vi->rvq->vq_ops->kick(vi->rvq);
-- 
1.6.2.5

Re: [PATCH] virtio_net: Check for room in the vq before adding buffer

[Rusty Russell]

On Wed, 26 Aug 2009 06:58:28 pm Amit Shah wrote:
> Saves us one cycle of alloc-add-free if the queue was full.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Thanks, applied with one change:

> @@ -323,7 +323,7 @@ static bool try_fill_recv_maxbufs(struct virtnet_info *vi, gfp_t gfp)
>  			break;
>  		}
>  		vi->num++;
> -	}
> +	} while (err > 0);

This is better as "while (err >= num)".  The other one is right (we only need
1 buffer), this one we need to be able to fit "num" entries.

Thanks!
Rusty.

Re: [PATCH] virtio_net: Check for room in the vq before adding buffer

[Amit Shah]

On (Thu) Aug 27 2009 [19:17:20], Rusty Russell wrote:
> On Wed, 26 Aug 2009 06:58:28 pm Amit Shah wrote:
> > Saves us one cycle of alloc-add-free if the queue was full.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> Thanks, applied with one change:
> 
> > @@ -323,7 +323,7 @@ static bool try_fill_recv_maxbufs(struct virtnet_info *vi, gfp_t gfp)
> >  			break;
> >  		}
> >  		vi->num++;
> > -	}
> > +	} while (err > 0);
> 
> This is better as "while (err >= num)".  The other one is right (we only need
> 1 buffer), this one we need to be able to fit "num" entries.

I'm missing something though: the value of 'num' changes in the loop and
the value of num when it will be compared will just have been used by
add_buf. The next iteration of the loop will probably use a different
value (-- from just reading the code, not going over the cases in which
this function is called).

		Amit

Re: [PATCH] virtio_net: Check for room in the vq before adding buffer

[Rusty Russell]

On Thu, 27 Aug 2009 07:59:06 pm Amit Shah wrote:
> On (Thu) Aug 27 2009 [19:17:20], Rusty Russell wrote:
> > On Wed, 26 Aug 2009 06:58:28 pm Amit Shah wrote:
> > > Saves us one cycle of alloc-add-free if the queue was full.
> > > 
> > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > 
> > Thanks, applied with one change:
> > 
> > > @@ -323,7 +323,7 @@ static bool try_fill_recv_maxbufs(struct virtnet_info *vi, gfp_t gfp)
> > >  			break;
> > >  		}
> > >  		vi->num++;
> > > -	}
> > > +	} while (err > 0);
> > 
> > This is better as "while (err >= num)".  The other one is right (we only need
> > 1 buffer), this one we need to be able to fit "num" entries.
> 
> I'm missing something though: the value of 'num' changes in the loop and
> the value of num when it will be compared will just have been used by
> add_buf. The next iteration of the loop will probably use a different
> value (-- from just reading the code, not going over the cases in which
> this function is called).

Here's the code:

		num = skb_to_sgvec(skb, sg+1, 0, skb->len) + 1;
		skb_queue_head(&vi->recv, skb);

		err = vi->rvq->vq_ops->add_buf(vi->rvq, sg, 0, num, skb);
		if (err < 0) {
			skb_unlink(skb, &vi->recv);
			trim_pages(vi, skb);
			kfree_skb(skb);
			break;
		}
		vi->num++;
	} while (err >= num);

Now, it turns out that we always allocate packets of the same size.

So the number of descriptors required by the next packet will be the same as
for this packet.

Clear?
Rusty.

Re: [PATCH] virtio_net: Check for room in the vq before adding buffer

[Amit Shah]

On (Thu) Aug 27 2009 [20:36:07], Rusty Russell wrote:
> > 
> > I'm missing something though: the value of 'num' changes in the loop and
> > the value of num when it will be compared will just have been used by
> > add_buf. The next iteration of the loop will probably use a different
> > value (-- from just reading the code, not going over the cases in which
> > this function is called).
> 
> Here's the code:
> 
> 		num = skb_to_sgvec(skb, sg+1, 0, skb->len) + 1;
> 		skb_queue_head(&vi->recv, skb);
> 
> 		err = vi->rvq->vq_ops->add_buf(vi->rvq, sg, 0, num, skb);
> 		if (err < 0) {
> 			skb_unlink(skb, &vi->recv);
> 			trim_pages(vi, skb);
> 			kfree_skb(skb);
> 			break;
> 		}
> 		vi->num++;
> 	} while (err >= num);
> 
> Now, it turns out that we always allocate packets of the same size.

OK; works since the packet size is the same.

> So the number of descriptors required by the next packet will be the same as
> for this packet.
> 
> Clear?

Thanks!

> Rusty.

		Amit