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

From: Jason Wang
Date: Wed Jul 16 2014 - 22:55:51 EST


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@xxxxxxxxxxxxxxx>
>> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
>> Cc: Vlad Yasevich <vyasevic@xxxxxxxxxx>
>> Cc: Eric Dumazet <eric.dumazet@xxxxxxxxx>
>> Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
>> ---
>> 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.
--
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/