* [PATCH net-next] virtio_net: Disable interrupts if napi_complete_done rescheduled napi
@ 2017-12-07 4:15 Toshiaki Makita
2017-12-07 5:09 ` Michael S. Tsirkin
0 siblings, 1 reply; 4+ messages in thread
From: Toshiaki Makita @ 2017-12-07 4:15 UTC (permalink / raw)
To: David S . Miller, Michael S . Tsirkin, Jason Wang
Cc: Toshiaki Makita, netdev, virtualization, Eric Dumazet
Since commit 39e6c8208d7b ("net: solve a NAPI race") napi has been able
to be rescheduled within napi_complete_done() even in non-busypoll case,
but virtnet_poll() always enabled interrupts before complete, and when
napi was rescheduled within napi_complete_done() it did not disable
interrupts.
This caused more interrupts when event idx is disabled.
According to commit cbdadbbf0c79 ("virtio_net: fix race in RX VQ
processing") we cannot place virtqueue_enable_cb_prepare() after
NAPI_STATE_SCHED is cleared, so disable interrupts again if
napi_complete_done() returned false.
Tested with vhost-user of OVS 2.7 on host, which does not have the event
idx feature.
* Before patch:
$ netperf -t UDP_STREAM -H 192.168.150.253 -l 60 -- -m 1472
MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.150.253 () port 0 AF_INET
Socket Message Elapsed Messages
Size Size Time Okay Errors Throughput
bytes bytes secs # # 10^6bits/sec
212992 1472 60.00 32763206 0 6430.32
212992 60.00 23384299 4589.56
Interrupts on guest: 9872369
Packets/interrupt: 2.37
* After patch
$ netperf -t UDP_STREAM -H 192.168.150.253 -l 60 -- -m 1472
MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.150.253 () port 0 AF_INET
Socket Message Elapsed Messages
Size Size Time Okay Errors Throughput
bytes bytes secs # # 10^6bits/sec
212992 1472 60.00 32794646 0 6436.49
212992 60.00 32793501 6436.27
Interrupts on guest: 4941299
Packets/interrupt: 6.64
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
drivers/net/virtio_net.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 19a985e..c0db48d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -261,9 +261,12 @@ static void virtqueue_napi_complete(struct napi_struct *napi,
int opaque;
opaque = virtqueue_enable_cb_prepare(vq);
- if (napi_complete_done(napi, processed) &&
- unlikely(virtqueue_poll(vq, opaque)))
- virtqueue_napi_schedule(napi, vq);
+ if (napi_complete_done(napi, processed)) {
+ if (unlikely(virtqueue_poll(vq, opaque)))
+ virtqueue_napi_schedule(napi, vq);
+ } else {
+ virtqueue_disable_cb(vq);
+ }
}
static void skb_xmit_done(struct virtqueue *vq)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net-next] virtio_net: Disable interrupts if napi_complete_done rescheduled napi
2017-12-07 4:15 [PATCH net-next] virtio_net: Disable interrupts if napi_complete_done rescheduled napi Toshiaki Makita
@ 2017-12-07 5:09 ` Michael S. Tsirkin
2017-12-07 7:08 ` Jason Wang
0 siblings, 1 reply; 4+ messages in thread
From: Michael S. Tsirkin @ 2017-12-07 5:09 UTC (permalink / raw)
To: Toshiaki Makita; +Cc: netdev, David S . Miller, Eric Dumazet, virtualization
On Thu, Dec 07, 2017 at 01:15:15PM +0900, Toshiaki Makita wrote:
> Since commit 39e6c8208d7b ("net: solve a NAPI race") napi has been able
> to be rescheduled within napi_complete_done() even in non-busypoll case,
> but virtnet_poll() always enabled interrupts before complete, and when
> napi was rescheduled within napi_complete_done() it did not disable
> interrupts.
> This caused more interrupts when event idx is disabled.
>
> According to commit cbdadbbf0c79 ("virtio_net: fix race in RX VQ
> processing") we cannot place virtqueue_enable_cb_prepare() after
> NAPI_STATE_SCHED is cleared, so disable interrupts again if
> napi_complete_done() returned false.
>
> Tested with vhost-user of OVS 2.7 on host, which does not have the event
> idx feature.
>
> * Before patch:
>
> $ netperf -t UDP_STREAM -H 192.168.150.253 -l 60 -- -m 1472
> MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.150.253 () port 0 AF_INET
> Socket Message Elapsed Messages
> Size Size Time Okay Errors Throughput
> bytes bytes secs # # 10^6bits/sec
>
> 212992 1472 60.00 32763206 0 6430.32
> 212992 60.00 23384299 4589.56
>
> Interrupts on guest: 9872369
> Packets/interrupt: 2.37
>
> * After patch
>
> $ netperf -t UDP_STREAM -H 192.168.150.253 -l 60 -- -m 1472
> MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.150.253 () port 0 AF_INET
> Socket Message Elapsed Messages
> Size Size Time Okay Errors Throughput
> bytes bytes secs # # 10^6bits/sec
>
> 212992 1472 60.00 32794646 0 6436.49
> 212992 60.00 32793501 6436.27
>
> Interrupts on guest: 4941299
> Packets/interrupt: 6.64
>
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
it might make sense in net and not -next since tx napi regressed performance
in some configs, this might bring it back at least partially.
Jason - what do you think?
> ---
> drivers/net/virtio_net.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 19a985e..c0db48d 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -261,9 +261,12 @@ static void virtqueue_napi_complete(struct napi_struct *napi,
> int opaque;
>
> opaque = virtqueue_enable_cb_prepare(vq);
> - if (napi_complete_done(napi, processed) &&
> - unlikely(virtqueue_poll(vq, opaque)))
> - virtqueue_napi_schedule(napi, vq);
> + if (napi_complete_done(napi, processed)) {
> + if (unlikely(virtqueue_poll(vq, opaque)))
> + virtqueue_napi_schedule(napi, vq);
> + } else {
> + virtqueue_disable_cb(vq);
> + }
> }
>
> static void skb_xmit_done(struct virtqueue *vq)
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next] virtio_net: Disable interrupts if napi_complete_done rescheduled napi
2017-12-07 5:09 ` Michael S. Tsirkin
@ 2017-12-07 7:08 ` Jason Wang
2017-12-08 18:19 ` David Miller
0 siblings, 1 reply; 4+ messages in thread
From: Jason Wang @ 2017-12-07 7:08 UTC (permalink / raw)
To: Michael S. Tsirkin, Toshiaki Makita
Cc: David S . Miller, netdev, virtualization, Eric Dumazet
On 2017年12月07日 13:09, Michael S. Tsirkin wrote:
> On Thu, Dec 07, 2017 at 01:15:15PM +0900, Toshiaki Makita wrote:
>> Since commit 39e6c8208d7b ("net: solve a NAPI race") napi has been able
>> to be rescheduled within napi_complete_done() even in non-busypoll case,
>> but virtnet_poll() always enabled interrupts before complete, and when
>> napi was rescheduled within napi_complete_done() it did not disable
>> interrupts.
>> This caused more interrupts when event idx is disabled.
>>
>> According to commit cbdadbbf0c79 ("virtio_net: fix race in RX VQ
>> processing") we cannot place virtqueue_enable_cb_prepare() after
>> NAPI_STATE_SCHED is cleared, so disable interrupts again if
>> napi_complete_done() returned false.
>>
>> Tested with vhost-user of OVS 2.7 on host, which does not have the event
>> idx feature.
>>
>> * Before patch:
>>
>> $ netperf -t UDP_STREAM -H 192.168.150.253 -l 60 -- -m 1472
>> MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.150.253 () port 0 AF_INET
>> Socket Message Elapsed Messages
>> Size Size Time Okay Errors Throughput
>> bytes bytes secs # # 10^6bits/sec
>>
>> 212992 1472 60.00 32763206 0 6430.32
>> 212992 60.00 23384299 4589.56
>>
>> Interrupts on guest: 9872369
>> Packets/interrupt: 2.37
>>
>> * After patch
>>
>> $ netperf -t UDP_STREAM -H 192.168.150.253 -l 60 -- -m 1472
>> MIGRATED UDP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.150.253 () port 0 AF_INET
>> Socket Message Elapsed Messages
>> Size Size Time Okay Errors Throughput
>> bytes bytes secs # # 10^6bits/sec
>>
>> 212992 1472 60.00 32794646 0 6436.49
>> 212992 60.00 32793501 6436.27
>>
>> Interrupts on guest: 4941299
>> Packets/interrupt: 6.64
>>
>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> it might make sense in net and not -next since tx napi regressed performance
> in some configs, this might bring it back at least partially.
> Jason - what do you think?
No sure, the regression I saw was tested with event idx on. And
virtqueue_disable_cb() does almost nothing for event idx (or even a
little bit slower).
The patch looks good.
Acked-by: Jason Wang <jasowang@redhat.com>
>
>> ---
>> drivers/net/virtio_net.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 19a985e..c0db48d 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -261,9 +261,12 @@ static void virtqueue_napi_complete(struct napi_struct *napi,
>> int opaque;
>>
>> opaque = virtqueue_enable_cb_prepare(vq);
>> - if (napi_complete_done(napi, processed) &&
>> - unlikely(virtqueue_poll(vq, opaque)))
>> - virtqueue_napi_schedule(napi, vq);
>> + if (napi_complete_done(napi, processed)) {
>> + if (unlikely(virtqueue_poll(vq, opaque)))
>> + virtqueue_napi_schedule(napi, vq);
>> + } else {
>> + virtqueue_disable_cb(vq);
>> + }
>> }
>>
>> static void skb_xmit_done(struct virtqueue *vq)
>> --
>> 1.8.3.1
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next] virtio_net: Disable interrupts if napi_complete_done rescheduled napi
2017-12-07 7:08 ` Jason Wang
@ 2017-12-08 18:19 ` David Miller
0 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2017-12-08 18:19 UTC (permalink / raw)
To: jasowang; +Cc: mst, makita.toshiaki, netdev, virtualization, edumazet
From: Jason Wang <jasowang@redhat.com>
Date: Thu, 7 Dec 2017 15:08:45 +0800
>
>
> On 2017年12月07日 13:09, Michael S. Tsirkin wrote:
>> On Thu, Dec 07, 2017 at 01:15:15PM +0900, Toshiaki Makita wrote:
>>> Since commit 39e6c8208d7b ("net: solve a NAPI race") napi has been
>>> able
>>> to be rescheduled within napi_complete_done() even in non-busypoll
>>> case,
>>> but virtnet_poll() always enabled interrupts before complete, and when
>>> napi was rescheduled within napi_complete_done() it did not disable
>>> interrupts.
>>> This caused more interrupts when event idx is disabled.
>>>
>>> According to commit cbdadbbf0c79 ("virtio_net: fix race in RX VQ
>>> processing") we cannot place virtqueue_enable_cb_prepare() after
>>> NAPI_STATE_SCHED is cleared, so disable interrupts again if
>>> napi_complete_done() returned false.
>>>
>>> Tested with vhost-user of OVS 2.7 on host, which does not have the
>>> event
>>> idx feature.
...
>>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> it might make sense in net and not -next since tx napi regressed
>> performance
>> in some configs, this might bring it back at least partially.
>> Jason - what do you think?
>
> No sure, the regression I saw was tested with event idx on. And
> virtqueue_disable_cb() does almost nothing for event idx (or even a
> little bit slower).
>
> The patch looks good.
>
> Acked-by: Jason Wang <jasowang@redhat.com>
I'm going to put this into net-next for now.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-12-08 18:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-07 4:15 [PATCH net-next] virtio_net: Disable interrupts if napi_complete_done rescheduled napi Toshiaki Makita
2017-12-07 5:09 ` Michael S. Tsirkin
2017-12-07 7:08 ` Jason Wang
2017-12-08 18:19 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).