Re: [PATCH net-next V2 3/3] virtio-net: rx busy polling support

[Jason Wang <jasowang@redhat.com>]

On 07/17/2014 11:27 AM, Varka Bhadram wrote:
>
> On Thursday 17 July 2014 08:25 AM, Jason Wang wrote:
>> On 07/16/2014 04:38 PM, Varka Bhadram wrote:
>>> On 07/16/2014 11:51 AM, Jason Wang wrote:
>>>> Add basic support for rx busy polling.
>>>>
>>>> Test was done between a kvm guest and an external host. Two hosts were
>>>> connected through 40gb mlx4 cards. With both busy_poll and busy_read
>>>> are set to 50 in guest, 1 byte netperf tcp_rr shows 116% improvement:
>>>> transaction rate was increased from 9151.94 to 19787.37.
>>>>
>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> Cc: Vlad Yasevich <vyasevic@redhat.com>
>>>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>>    drivers/net/virtio_net.c | 190
>>>> ++++++++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 187 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index e417d93..4830713 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -27,6 +27,7 @@
>>>>    #include <linux/slab.h>
>>>>    #include <linux/cpu.h>
>>>>    #include <linux/average.h>
>>>> +#include <net/busy_poll.h>
>>>>      static int napi_weight = NAPI_POLL_WEIGHT;
>>>>    module_param(napi_weight, int, 0444);
>>>> @@ -94,8 +95,143 @@ struct receive_queue {
>>>>          /* Name of this receive queue: input.$index */
>>>>        char name[40];
>>>> +
>>>> +#ifdef CONFIG_NET_RX_BUSY_POLL
>>>> +    unsigned int state;
>>>> +#define VIRTNET_RQ_STATE_IDLE        0
>>>> +#define VIRTNET_RQ_STATE_NAPI         1    /* NAPI or refill owns
>>>> this RQ */
>>>> +#define VIRTNET_RQ_STATE_POLL         2    /* poll owns this RQ */
>>>> +#define VIRTNET_RQ_STATE_DISABLED    4    /* RQ is disabled */
>>>> +#define VIRTNET_RQ_OWNED (VIRTNET_RQ_STATE_NAPI |
>>>> VIRTNET_RQ_STATE_POLL)
>>>> +#define VIRTNET_RQ_LOCKED (VIRTNET_RQ_OWNED |
>>>> VIRTNET_RQ_STATE_DISABLED)
>>>> +#define VIRTNET_RQ_STATE_NAPI_YIELD  8    /* NAPI or refill yielded
>>>> this RQ */
>>>> +#define VIRTNET_RQ_STATE_POLL_YIELD  16   /* poll yielded this RQ */
>>>> +    spinlock_t lock;
>>>> +#endif  /* CONFIG_NET_RX_BUSY_POLL */
>>>>    };
>>>>    +#ifdef CONFIG_NET_RX_BUSY_POLL
>>>> +static inline void virtnet_rq_init_lock(struct receive_queue *rq)
>>>> +{
>>>> +
>>>> +    spin_lock_init(&rq->lock);
>>>> +    rq->state = VIRTNET_RQ_STATE_IDLE;
>>>> +}
>>>> +
>>>> +/* called from the device poll routine or refill routine to get
>>>> ownership of a
>>>> + * receive queue.
>>>> + */
>>>> +static inline bool virtnet_rq_lock_napi_refill(struct receive_queue
>>>> *rq)
>>>> +{
>>>> +    int rc = true;
>>>> +
>>> bool instead of int...?
>> Yes, it was better.
>>>> +    spin_lock(&rq->lock);
>>>> +    if (rq->state & VIRTNET_RQ_LOCKED) {
>>>> +        WARN_ON(rq->state & VIRTNET_RQ_STATE_NAPI);
>>>> +        rq->state |= VIRTNET_RQ_STATE_NAPI_YIELD;
>>>> +        rc = false;
>>>> +    } else
>>>> +        /* we don't care if someone yielded */
>>>> +        rq->state = VIRTNET_RQ_STATE_NAPI;
>>>> +    spin_unlock(&rq->lock);
>>> Lock for rq->state ...?
>>>
>>> If yes:
>>> spin_lock(&rq->lock);
>>> if (rq->state & VIRTNET_RQ_LOCKED) {
>>>      rq->state |= VIRTNET_RQ_STATE_NAPI_YIELD;
>>>      spin_unlock(&rq->lock);
>>>      WARN_ON(rq->state & VIRTNET_RQ_STATE_NAPI);
>>>      rc = false;
>>> } else {
>>>      /* we don't care if someone yielded */
>>>      rq->state = VIRTNET_RQ_STATE_NAPI;
>>>      spin_unlock(&rq->lock);
>>> }
>> I didn't see any differences. Is this used to catch the bug of driver
>> earlier? btw, several other rx busy polling capable driver does the same
>> thing.
>
> We need not to include WARN_ON() & rc=false under critical section.
>

Ok. but unless there's a bug in the driver itself, WARN_ON() should be
just a condition check for a branch, so there should not be noticeable
differences.

Also we should not check rq->state outside the protection of lock.