[net-next,v2,5/5] virtio-net: keep tx interrupts disabled unless kick

From: Willem de Bruijn <willemb@google.com>

Tx napi mode increases the rate of transmit interrupts. Suppress some
by masking interrupts while more packets are expected. The interrupts
will be reenabled before the last packet is sent.

This optimization reduces the througput drop with tx napi for
unidirectional flows such as UDP_STREAM that do not benefit from
cleaning tx completions in the the receive napi handler.

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/virtio_net.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

On 2017年04月19日 04:21, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Tx napi mode increases the rate of transmit interrupts. Suppress some
> by masking interrupts while more packets are expected. The interrupts
> will be reenabled before the last packet is sent.
>
> This optimization reduces the througput drop with tx napi for
> unidirectional flows such as UDP_STREAM that do not benefit from
> cleaning tx completions in the the receive napi handler.
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>   drivers/net/virtio_net.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b14c82ce0032..b107ae011632 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1196,8 +1196,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>   	bool use_napi = sq->napi.weight;
>   
>   	/* Free up any pending old buffers before queueing new ones. */
> -	if (!use_napi)
> +	if (use_napi) {
> +		if (kick)
> +			virtqueue_enable_cb_delayed(sq->vq);
> +		else
> +			virtqueue_disable_cb(sq->vq);

Since virtqueue_disable_cb() do nothing for event idx. I wonder whether 
or not just calling enable_cb_dealyed() is ok here.

Btw, it does not disable interrupt at all, I propose a patch in the past 
which can do more than this:

https://patchwork.kernel.org/patch/6472601/

Thanks



> +	} else {
>   		free_old_xmit_skbs(sq);
> +	}
>   
>   	/* timestamp packet in software */
>   	skb_tx_timestamp(skb);

>> -       if (!use_napi)
>> +       if (use_napi) {
>> +               if (kick)
>> +                       virtqueue_enable_cb_delayed(sq->vq);
>> +               else
>> +                       virtqueue_disable_cb(sq->vq);
>
>
> Since virtqueue_disable_cb() do nothing for event idx. I wonder whether or
> not just calling enable_cb_dealyed() is ok here.

Good point.

> Btw, it does not disable interrupt at all, I propose a patch in the past
> which can do more than this:
>
> https://patchwork.kernel.org/patch/6472601/

Interesting. Yes, let me evaluate that variant.

Thanks for reviewing,

  Willem

On Thu, Apr 20, 2017 at 10:03 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>>> -       if (!use_napi)
>>> +       if (use_napi) {
>>> +               if (kick)
>>> +                       virtqueue_enable_cb_delayed(sq->vq);
>>> +               else
>>> +                       virtqueue_disable_cb(sq->vq);
>>
>>
>> Since virtqueue_disable_cb() do nothing for event idx. I wonder whether or
>> not just calling enable_cb_dealyed() is ok here.
>
> Good point.
>
>> Btw, it does not disable interrupt at all, I propose a patch in the past
>> which can do more than this:
>>
>> https://patchwork.kernel.org/patch/6472601/
>
> Interesting. Yes, let me evaluate that variant.

In initial tests I don't see a significant change, but we can look
into this more closely as a follow-on patch.