Re: [PATCH net 2/3] virtio-net: fix num calculation on frag skballocation failure

[Jason Wang]

----- ååéä -----
> 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@xxxxxxxxxxxxxxx>
> > > > Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > > > Cc: Michael Dalton <mwdalton@xxxxxxxxxx>
> > > > Cc: Eric Dumazet <edumazet@xxxxxxxxxx>
> > > > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/