vhost: poll vhost_net only when tx notification is enabled
Subsystems (4)
PATCH 1/1 vhost: poll vhost_net only when tx notification is enabled
guest kick host base on avail_ring flags value and get perfermance improved, vhost_zerocopy_callback could do the same thing. As virtqueue_enable_cb need one more check after modifying the value of avail_ring flags, vhost also need do the same thing after vhost_enable_notify.
test result list as below:
guest and host: suse11sp3, netperf, intel CPU 2.4GHz
+------+----------+--------+----------+--------+--------+---------+ | | old | new | +------+----------+--------+----------+--------+--------+---------+ | UDP | Gbit/s | PPS |CPU idle% | Gbit/s | PPS |CPU idle%| | 256 | 0.74805 | 321309 | 87.16 | 0.77933| 334743 | 90.71 | | 512 | 1.42 | 328475 | 87.03 | 1.44 | 333550 | 90.43 | | 1024 | 2.79 | 334426 | 89.09 | 2.81 | 336986 | 89.55 | | 1460 | 3.71 | 316215 | 87.53 | 4.02 | 342325 | 89.58 | +------+----------+--------+----------+--------+--------+---------+
--- drivers/vhost/net.c | 13 ++++++++++++- 1 files changed, 12 insertions(+), 1 deletions(-)
drivers/vhost/net.c +12 -1
@@ -315,6 +315,10 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) VHOST_DMA_DONE_LEN :VHOST_DMA_FAILED_LEN ; cnt =vhost_net_ubuf_put (ubufs); +/* make sure len has been updated because handle_tx would use it +* and used_flags should also been checked. +*/ +smp_mb ();/* * Trigger polling thread if guest stopped submitting new buffers: * in this case, the refcount after decrement will eventually reach 1.
@@ -322,7 +326,8 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)* (the value 16 here is more or less arbitrary, it's tuned to trigger * less than 10% of times). */ -if (cnt <=1 || !(cnt %16 )) +if ((!(vq->used_flags &VRING_USED_F_NO_NOTIFY )) + && (cnt <=1 || !(cnt %16 )))vhost_poll_queue (&vq->poll);rcu_read_unlock_bh ();
@@ -386,6 +391,12 @@ static void handle_tx(struct vhost_net *net)vhost_disable_notify (&net->dev, vq);continue ; } +/* there might skb been freed between last +* vhost_zerocopy_signal_used and vhost_enable_notify, +* so one more check is needed. +*/ +if (zcopy) +vhost_zerocopy_signal_used (net, vq);break ; }if (in) {
Signature · 1 line
1.7.3.1.msysgit.0
Quoted 49 lines · Qin Chuanyu
guest kick host base on
avail_ringflags value and get perfermance improved,vhost_zerocopy_callbackcould do the same thing. Asvirtqueue_enable_cbneed one more check after modifying the value ofavail_ringflags, vhost also need do the same thing aftervhost_enable_notify.test result list as below:
guest and host: suse11sp3, netperf, intel CPU 2.4GHz+------+----------+--------+----------+--------+--------+---------+ | | old | new | +------+----------+--------+----------+--------+--------+---------+ | UDP | Gbit/s | PPS |CPU idle% | Gbit/s | PPS |CPU idle%| | 256 | 0.74805 | 321309 | 87.16 | 0.77933| 334743 | 90.71 | | 512 | 1.42 | 328475 | 87.03 | 1.44 | 333550 | 90.43 | | 1024 | 2.79 | 334426 | 89.09 | 2.81 | 336986 | 89.55 | | 1460 | 3.71 | 316215 | 87.53 | 4.02 | 342325 | 89.58 | +------+----------+--------+----------+--------+--------+---------+Signed-off-by Chuanyu Qin <[email protected]>--- drivers/vhost/net.c | 13 ++++++++++++- 1 files changed, 12 insertions(+), 1 deletions(-)drivers/vhost/net.c +6
-1@@ -315,6 +315,10 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) VHOST_DMA_DONE_LEN :VHOST_DMA_FAILED_LEN ; cnt =vhost_net_ubuf_put (ubufs); +/* make sure len has been updated because handle_tx would use it +* and used_flags should also been checked. +*/ +smp_mb ();/* * Trigger polling thread if guest stopped submitting new buffers: * in this case, the refcount after decrement will eventually reach 1. @@ -322,7 +326,8 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)* (the value 16 here is more or less arbitrary, it's tuned to trigger * less than 10% of times). */ -if (cnt <=1 || !(cnt %16 )) +if ((!(vq->used_flags &VRING_USED_F_NO_NOTIFY )) + && (cnt <=1 || !(cnt %16 )))vhost_poll_queue (&vq->poll);rcu_read_unlock_bh ();
drivers/vhost/net.c static void handle_tx(struct vhost_net *net) · line 394
Quoted 4 lines
drivers/vhost/net.c +1
-0@@ -386,6 +391,12 @@ static void handle_tx(struct vhost_net *net)vhost_disable_notify (&net->dev, vq);continue ; } +/* there might skb been freed between last
Not aligned here.
-Ding
Quoted 8 lines
drivers/vhost/net.c +5
-0+* vhost_zerocopy_signal_used and vhost_enable_notify, +* so one more check is needed. +*/ +if (zcopy) +vhost_zerocopy_signal_used (net, vq);break ; }if (in) {
Michael S. Tsirkin
maintainerPlease see MAINTAINERS and copy all relevant lists.
Quoted 1 line · Qin Chuanyu
guest kick host base on
avail_ringflags value and get perfermance
typo
Quoted 17 lines
improved,
vhost_zerocopy_callbackcould do the same thing. Asvirtqueue_enable_cbneed one more check after modifying the value ofavail_ringflags, vhost also need do the same thing aftervhost_enable_notify.test result list as below:
guest and host: suse11sp3, netperf, intel CPU 2.4GHz+------+----------+--------+----------+--------+--------+---------+ | | old | new | +------+----------+--------+----------+--------+--------+---------+ | UDP | Gbit/s | PPS |CPU idle% | Gbit/s | PPS |CPU idle%| | 256 | 0.74805 | 321309 | 87.16 | 0.77933| 334743 | 90.71 | | 512 | 1.42 | 328475 | 87.03 | 1.44 | 333550 | 90.43 | | 1024 | 2.79 | 334426 | 89.09 | 2.81 | 336986 | 89.55 | | 1460 | 3.71 | 316215 | 87.53 | 4.02 | 342325 | 89.58 | +------+----------+--------+----------+--------+--------+---------+
Quoted 1 line
Signed-off-by Chuanyu Qin <[email protected]>
It's an interesting optimization, thanks! However, it looks like this might delay updating used ring indefinitely if we are unlucky. Some guests (e.g. windows) tend to crash if this happens.
Maybe use a new flag for this?
It also looks like there are potential race conditions below.
Quoted 19 lines
--- drivers/vhost/net.c | 13 ++++++++++++- 1 files changed, 12 insertions(+), 1 deletions(-)drivers/vhost/net.c +4
-0@@ -315,6 +315,10 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN; cnt =vhost_net_ubuf_put (ubufs); +/* make sure len has been updated because handle_tx would use it +* and used_flags should also been checked. +*/ +smp_mb ();/* * Trigger polling thread if guest stopped submitting new buffers:
Quoted 1 line
* in this case, the refcount after decrement will eventually reach 1.
this barrier is very suspect.
Quoted 11 lines
drivers/vhost/net.c +2
-1@@ -322,7 +326,8 @@ static void vhost_zerocopy_callback(structubuf_info *ubuf, bool success) * (the value 16 here is more or less arbitrary, it's tuned to trigger * less than 10% of times). */ -if (cnt <=1 || !(cnt %16 )) +if ((!(vq->used_flags &VRING_USED_F_NO_NOTIFY )) + && (cnt <=1 || !(cnt %16 )))vhost_poll_queue (&vq->poll);
rcu_read_unlock_bh();
looks like a potential race to me
Quoted 18 lines
drivers/vhost/net.c +6
-0@@ -386,6 +391,12 @@ static void handle_tx(struct vhost_net *net)vhost_disable_notify (&net->dev, vq);continue ; } +/* there might skb been freed between last +* vhost_zerocopy_signal_used and vhost_enable_notify, +* so one more check is needed. +*/ +if (zcopy) +vhost_zerocopy_signal_used (net, vq);break ;drivers/vhost/net.c +0
-0}if (in) {Signature · 1 line
1.7.3.1.msysgit.0
Quoted 23 lines · Michael S. Tsirkin
Please see
MAINTAINERSand copy all relevant lists.Quoted 1 line · Qin Chuanyu
guest kick host base on
avail_ringflags value and get perfermancetypo
Quoted 17 lines
improved,
vhost_zerocopy_callbackcould do the same thing. Asvirtqueue_enable_cbneed one more check after modifying the value ofavail_ringflags, vhost also need do the same thing aftervhost_enable_notify.test result list as below:
guest and host: suse11sp3, netperf, intel CPU 2.4GHz+------+----------+--------+----------+--------+--------+---------+ | | old | new | +------+----------+--------+----------+--------+--------+---------+ | UDP | Gbit/s | PPS |CPU idle% | Gbit/s | PPS |CPU idle%| | 256 | 0.74805 | 321309 | 87.16 | 0.77933| 334743 | 90.71 | | 512 | 1.42 | 328475 | 87.03 | 1.44 | 333550 | 90.43 | | 1024 | 2.79 | 334426 | 89.09 | 2.81 | 336986 | 89.55 | | 1460 | 3.71 | 316215 | 87.53 | 4.02 | 342325 | 89.58 | +------+----------+--------+----------+--------+--------+---------+
Quoted 6 lines
Quoted 1 line
Signed-off-by Chuanyu Qin <[email protected]>It's an interesting optimization, thanks!
However, it looks like this might delay
updating used ring indefinitely if we are
unlucky. Some guests (e.g. windows)
tend to crash if this happens.
Looks like it does not change this.
When tx notification is disabled, it means handle_tx() is running which will update the used ring before each tx descriptor fetching. And the patch does used updating after each time when tx notification is enabled.
Quoted 37 lines
Maybe use a new flag for this?
It also looks like there are potential race conditions below.
Quoted 19 lines
--- drivers/vhost/net.c | 13 ++++++++++++- 1 files changed, 12 insertions(+), 1 deletions(-)drivers/vhost/net.c +4
-0@@ -315,6 +315,10 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) VHOST_DMA_DONE_LEN : VHOST_DMA_FAILED_LEN; cnt =vhost_net_ubuf_put (ubufs); +/* make sure len has been updated because handle_tx would use it +* and used_flags should also been checked. +*/ +smp_mb ();/* * Trigger polling thread if guest stopped submitting new buffers: Quoted 1 line
* in this case, the refcount after decrement will eventually reach 1.
this barrier is very suspect.
Quoted 10 lines
drivers/vhost/net.c +2
-1@@ -322,7 +326,8 @@ static void vhost_zerocopy_callback(structubuf_info *ubuf, bool success) * (the value 16 here is more or less arbitrary, it's tuned to trigger * less than 10% of times). */ -if (cnt <=1 || !(cnt %16 )) +if ((!(vq->used_flags &VRING_USED_F_NO_NOTIFY )) + && (cnt <=1 || !(cnt %16 )))vhost_poll_queue (&vq->poll);
Quoted 2 lines
Quoted 1 line
rcu_read_unlock_bh();looks like a potential race to me
Could you please explain the race a little bit more?
Quoted 21 lines
Quoted 13 lines
drivers/vhost/net.c +6
-0@@ -386,6 +391,12 @@ static void handle_tx(struct vhost_net *net)vhost_disable_notify (&net->dev, vq);continue ; } +/* there might skb been freed between last +* vhost_zerocopy_signal_used and vhost_enable_notify, +* so one more check is needed. +*/ +if (zcopy) +vhost_zerocopy_signal_used (net, vq);break ; }Quoted 4 lines
if (in) {
Signature · 1 line
1.7.3.1.msysgit.0--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html