Jason Wang <[email protected]> writes:
> It looks like there's no need for those two fields:
>
> - Unless there's a failure for the first refill try, rq->max should be always
> equal to the vring size.
> - rq->num is only used to determine the condition that we need to do the refill,
> we could check vq->num_free instead.
> - rq->num was required to be increased or decreased explicitly after each
> get/put which results a bad API.
>
> So this patch removes them both to make the code simpler.
Nice. These fields date from when the vq struct was opaque.
Applied,
Rusty.
> Cc: Rusty Russell <[email protected]>
> Cc: Michael S. Tsirkin <[email protected]>
> Signed-off-by: Jason Wang <[email protected]>
> ---
> drivers/net/virtio_net.c | 16 +++-------------
> 1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index c51a988..4e1bce3 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -72,9 +72,6 @@ struct receive_queue {
>
> struct napi_struct napi;
>
> - /* Number of input buffers, and max we've ever had. */
> - unsigned int num, max;
> -
> /* Chain pages by the private ptr. */
> struct page *pages;
>
> @@ -360,7 +357,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)) {
> @@ -406,7 +402,6 @@ err_skb:
> }
> page = virt_to_head_page(buf);
> put_page(page);
> - --rq->num;
> }
> err_buf:
> dev->stats.rx_dropped++;
> @@ -628,10 +623,7 @@ static bool try_fill_recv(struct receive_queue *rq, gfp_t gfp)
> oom = err == -ENOMEM;
> if (err)
> break;
> - ++rq->num;
> } while (rq->vq->num_free);
> - if (unlikely(rq->num > rq->max))
> - rq->max = rq->num;
> if (unlikely(!virtqueue_kick(rq->vq)))
> return false;
> return !oom;
> @@ -699,11 +691,10 @@ again:
> while (received < budget &&
> (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
> receive_buf(rq, buf, len);
> - --rq->num;
> received++;
> }
>
> - if (rq->num < rq->max / 2) {
> + if (rq->vq->num_free > virtqueue_get_vring_size(rq->vq) / 2) {
> if (!try_fill_recv(rq, GFP_ATOMIC))
> schedule_delayed_work(&vi->refill, 0);
> }
> @@ -1398,9 +1389,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
> give_pages(&vi->rq[i], buf);
> else
> dev_kfree_skb(buf);
> - --vi->rq[i].num;
> }
> - BUG_ON(vi->rq[i].num != 0);
> }
> }
>
> @@ -1671,7 +1660,8 @@ static int virtnet_probe(struct virtio_device *vdev)
> try_fill_recv(&vi->rq[i], GFP_KERNEL);
>
> /* If we didn't even get one input buffer, we're useless. */
> - if (vi->rq[i].num == 0) {
> + if (vi->rq[i].vq->num_free ==
> + virtqueue_get_vring_size(vi->rq[i].vq)) {
> free_unused_bufs(vi);
> err = -ENOMEM;
> goto free_recv_bufs;
> --
> 1.8.3.2
Rusty Russell <[email protected]> writes:
> Jason Wang <[email protected]> writes:
>> It looks like there's no need for those two fields:
>>
>> - Unless there's a failure for the first refill try, rq->max should be always
>> equal to the vring size.
>> - rq->num is only used to determine the condition that we need to do the refill,
>> we could check vq->num_free instead.
>> - rq->num was required to be increased or decreased explicitly after each
>> get/put which results a bad API.
>>
>> So this patch removes them both to make the code simpler.
>
> Nice. These fields date from when the vq struct was opaque.
>
> Applied,
> Rusty.
Oops, this doesn't require any core virtio changes, so it's for DaveM:
Acked-by: Rusty Russell <[email protected]>
Thanks,
Rusty.
>> Cc: Rusty Russell <[email protected]>
>> Cc: Michael S. Tsirkin <[email protected]>
>> Signed-off-by: Jason Wang <[email protected]>
>> ---
>> drivers/net/virtio_net.c | 16 +++-------------
>> 1 file changed, 3 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index c51a988..4e1bce3 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -72,9 +72,6 @@ struct receive_queue {
>>
>> struct napi_struct napi;
>>
>> - /* Number of input buffers, and max we've ever had. */
>> - unsigned int num, max;
>> -
>> /* Chain pages by the private ptr. */
>> struct page *pages;
>>
>> @@ -360,7 +357,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)) {
>> @@ -406,7 +402,6 @@ err_skb:
>> }
>> page = virt_to_head_page(buf);
>> put_page(page);
>> - --rq->num;
>> }
>> err_buf:
>> dev->stats.rx_dropped++;
>> @@ -628,10 +623,7 @@ static bool try_fill_recv(struct receive_queue *rq, gfp_t gfp)
>> oom = err == -ENOMEM;
>> if (err)
>> break;
>> - ++rq->num;
>> } while (rq->vq->num_free);
>> - if (unlikely(rq->num > rq->max))
>> - rq->max = rq->num;
>> if (unlikely(!virtqueue_kick(rq->vq)))
>> return false;
>> return !oom;
>> @@ -699,11 +691,10 @@ again:
>> while (received < budget &&
>> (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
>> receive_buf(rq, buf, len);
>> - --rq->num;
>> received++;
>> }
>>
>> - if (rq->num < rq->max / 2) {
>> + if (rq->vq->num_free > virtqueue_get_vring_size(rq->vq) / 2) {
>> if (!try_fill_recv(rq, GFP_ATOMIC))
>> schedule_delayed_work(&vi->refill, 0);
>> }
>> @@ -1398,9 +1389,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
>> give_pages(&vi->rq[i], buf);
>> else
>> dev_kfree_skb(buf);
>> - --vi->rq[i].num;
>> }
>> - BUG_ON(vi->rq[i].num != 0);
>> }
>> }
>>
>> @@ -1671,7 +1660,8 @@ static int virtnet_probe(struct virtio_device *vdev)
>> try_fill_recv(&vi->rq[i], GFP_KERNEL);
>>
>> /* If we didn't even get one input buffer, we're useless. */
>> - if (vi->rq[i].num == 0) {
>> + if (vi->rq[i].vq->num_free ==
>> + virtqueue_get_vring_size(vi->rq[i].vq)) {
>> free_unused_bufs(vi);
>> err = -ENOMEM;
>> goto free_recv_bufs;
>> --
>> 1.8.3.2
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
From: Rusty Russell <[email protected]>
Date: Thu, 16 Jan 2014 10:25:26 +1030
> Rusty Russell <[email protected]> writes:
>> Jason Wang <[email protected]> writes:
>>> It looks like there's no need for those two fields:
>>>
>>> - Unless there's a failure for the first refill try, rq->max should be always
>>> equal to the vring size.
>>> - rq->num is only used to determine the condition that we need to do the refill,
>>> we could check vq->num_free instead.
>>> - rq->num was required to be increased or decreased explicitly after each
>>> get/put which results a bad API.
>>>
>>> So this patch removes them both to make the code simpler.
>>
>> Nice. These fields date from when the vq struct was opaque.
>>
>> Applied,
>> Rusty.
>
> Oops, this doesn't require any core virtio changes, so it's for DaveM:
>
> Acked-by: Rusty Russell <[email protected]>
Jason please repost this with Rusty's ACK, thanks.
On 01/16/2014 08:46 AM, David Miller wrote:
> From: Rusty Russell<[email protected]>
> Date: Thu, 16 Jan 2014 10:25:26 +1030
>
>> Rusty Russell<[email protected]> writes:
>>> Jason Wang<[email protected]> writes:
>>>> It looks like there's no need for those two fields:
>>>>
>>>> - Unless there's a failure for the first refill try, rq->max should be always
>>>> equal to the vring size.
>>>> - rq->num is only used to determine the condition that we need to do the refill,
>>>> we could check vq->num_free instead.
>>>> - rq->num was required to be increased or decreased explicitly after each
>>>> get/put which results a bad API.
>>>>
>>>> So this patch removes them both to make the code simpler.
>>> Nice. These fields date from when the vq struct was opaque.
>>>
>>> Applied,
>>> Rusty.
>> Oops, this doesn't require any core virtio changes, so it's for DaveM:
>>
>> Acked-by: Rusty Russell<[email protected]>
> Jason please repost this with Rusty's ACK, thanks.
Sure, will repost.