vhost: make vhost_zerocopy_callback more efficient by poll_queue base on vhost status
Subsystems (2)
PATCH 1/1 vhost: make vhost_zerocopy_callback more efficient by poll_queue base on vhost status
guest kick vhost base on vring flag status and get perfermance improved, vhost_zerocopy_callback could do this in the same way, as virtqueue_enable_cb need one more check after change the status of avail_ring flags, vhost also do the same thing after vhost_enable_notify
test result list as below:
guest and host: suse11sp3, netperf, intel 2.4GHz
+-------+----------+---------+----------+---------+ | | old | new | +-------+----------+---------+----------+---------+ | UDP | Gbit/s | PPS | Gbit/s | PPS | | 256 | 0.74805 | 321309 | 0.77933 | 334743 | | 512 | 1.42 | 328475 | 1.44 | 333550 | | 1024 | 2.79 | 334426 | 2.81 | 336986 | | 1460 | 3.71 | 316215 | 4.02 | 342325 | +-------+----------+---------+----------+---------+
--- drivers/vhost/net.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
drivers/vhost/net.c +9 -1
@@ -322,7 +322,9 @@ 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 )) +smp_rmb (); +if ((!(vq->used_flags &VRING_USED_F_NO_NOTIFY )) + && (cnt <=1 || !(cnt %16 )))vhost_poll_queue (&vq->poll);rcu_read_unlock_bh ();
@@ -386,6 +388,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 7 lines · Qin Chuanyu
guest kick vhost base on vring flag status and get perfermance improved,
vhost_zerocopy_callbackcould do this in the same way, asvirtqueue_enable_cbneed one more check after change the status ofavail_ringflags, vhost also do the same thing aftervhost_enable_notifytest result list as below:
guest and host: suse11sp3, netperf, intel 2.4GHz
Quoted 9 lines
+-------+----------+---------+----------+---------+ | | old | new | +-------+----------+---------+----------+---------+ | UDP | Gbit/s | PPS | Gbit/s | PPS | | 256 | 0.74805 | 321309 | 0.77933 | 334743 | | 512 | 1.42 | 328475 | 1.44 | 333550 | | 1024 | 2.79 | 334426 | 2.81 | 336986 | | 1460 | 3.71 | 316215 | 4.02 | 342325 | +-------+----------+---------+----------+---------+
Looks good, do you have cpu utilization number?
Quoted 16 lines
Signed-off-by Chuanyu Qin <[email protected]>--- drivers/vhost/net.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)drivers/vhost/net.c +0
-0@@ -322,7 +322,9 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) * (the value 16 here is more or less arbitrary, it's tuned totrigger
* less than 10% of times).
*/
drivers/vhost/net.c static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) · line 325
Quoted 2 lines
drivers/vhost/net.c +1
-1-if (cnt <=1 || !(cnt %16 )) +smp_rmb ();
Better add a comment to explain why this is needed.
Looks like what you need is a smp_mb() here to make sure the len is updated before testing vq->used_flags?
Quoted 18 lines
drivers/vhost/net.c +8
-0+if ((!(vq->used_flags &VRING_USED_F_NO_NOTIFY )) + && (cnt <=1 || !(cnt %16 )))vhost_poll_queue (&vq->poll);rcu_read_unlock_bh ();@@ -386,6 +388,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) {
Quoted 18 lines · Jason Wang
Quoted 7 lines · Qin Chuanyu
guest kick vhost base on vring flag status and get perfermance improved,
vhost_zerocopy_callbackcould do this in the same way, asvirtqueue_enable_cbneed one more check after change the status ofavail_ringflags, vhost also do the same thing aftervhost_enable_notifytest result list as below:
guest and host: suse11sp3, netperf, intel 2.4GHzQuoted 9 lines
+-------+----------+---------+----------+---------+ | | old | new | +-------+----------+---------+----------+---------+ | UDP | Gbit/s | PPS | Gbit/s | PPS | | 256 | 0.74805 | 321309 | 0.77933 | 334743 | | 512 | 1.42 | 328475 | 1.44 | 333550 | | 1024 | 2.79 | 334426 | 2.81 | 336986 | | 1460 | 3.71 | 316215 | 4.02 | 342325 | +-------+----------+---------+----------+---------+
Quoted 1 line
Looks good, do you have cpu utilization number?
+------+----------+--------+----------+--------+--------+---------+ | | 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 | +------+----------+--------+----------+--------+--------+---------+
after change, less cpu has been used.
Quoted 21 lines
Quoted 16 lines
Signed-off-by Chuanyu Qin <[email protected]>--- drivers/vhost/net.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)drivers/vhost/net.c +0
-0@@ -322,7 +322,9 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) * (the value 16 here is more or less arbitrary, it's tuned totrigger
* less than 10% of times).
*/drivers/vhost/net.c static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) · line 325
Quoted 2 lines
drivers/vhost/net.c +1
-1-if (cnt <=1 || !(cnt %16 )) +smp_rmb ();Better add a comment to explain why this is needed.
Quoted 2 lines
Looks like what you need is a
smp_mb()here to make sure the len is updated before testingvq->used_flags?
I wanner make sure the used_flags is updated, is smp_rmb() enough? or a smp_mb() is needed?
Quoted 22 lines
Quoted 8 lines
drivers/vhost/net.c +2
-0+if ((!(vq->used_flags &VRING_USED_F_NO_NOTIFY )) + && (cnt <=1 || !(cnt %16 )))vhost_poll_queue (&vq->poll);rcu_read_unlock_bh ();@@ -386,6 +388,12 @@ static void handle_tx(struct vhost_net *net)vhost_disable_notify (&net->dev, vq);continue ;drivers/vhost/net.c static void handle_tx(struct vhost_net *net) · line 399
Quoted 10 lines
drivers/vhost/net.c +6
-0} +/* 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) {.
Quoted 31 lines · Qin Chuanyu
Quoted 20 lines · Jason Wang
Quoted 9 lines · Qin Chuanyu
guest kick vhost base on vring flag status and get perfermance improved,
vhost_zerocopy_callbackcould do this in the same way, asvirtqueue_enable_cbneed one more check after change the status ofavail_ringflags, vhost also do the same thing aftervhost_enable_notifytest result list as below:
guest and host: suse11sp3, netperf, intel 2.4GHzQuoted 9 lines
+-------+----------+---------+----------+---------+ | | old | new | +-------+----------+---------+----------+---------+ | UDP | Gbit/s | PPS | Gbit/s | PPS | | 256 | 0.74805 | 321309 | 0.77933 | 334743 | | 512 | 1.42 | 328475 | 1.44 | 333550 | | 1024 | 2.79 | 334426 | 2.81 | 336986 | | 1460 | 3.71 | 316215 | 4.02 | 342325 | +-------+----------+---------+----------+---------+Quoted 1 line
Looks good, do you have cpu utilization number?
+------+----------+--------+----------+--------+--------+---------+ | | 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
after change, less cpu has been used.
Thanks
Quoted 21 lines
Quoted 21 lines
Quoted 16 lines
Signed-off-by Chuanyu Qin <[email protected]>--- drivers/vhost/net.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)drivers/vhost/net.c +0
-0@@ -322,7 +322,9 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) * (the value 16 here is more or less arbitrary, it's tuned totrigger
* less than 10% of times).
*/drivers/vhost/net.c static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) · line 325
Quoted 2 lines
drivers/vhost/net.c +1
-1-if (cnt <=1 || !(cnt %16 )) +smp_rmb ();Better add a comment to explain why this is needed.
Quoted 4 lines
Quoted 2 lines
Looks like what you need is a
smp_mb()here to make sure the len is updated before testingvq->used_flags?I wanner make sure the
used_flagsis updated, issmp_rmb()enough? or asmp_mb()is needed?
used_flags was guaranteed to be updated after smp_mb() in vhost_enable_notify(). And vhost_net_ubuf_put() does a atomic_sub_return() which implements memory barrier. So looks like there's no need for an extra one.
Quoted 28 lines
Quoted 22 lines
Quoted 8 lines
drivers/vhost/net.c +2
-0+if ((!(vq->used_flags &VRING_USED_F_NO_NOTIFY )) + && (cnt <=1 || !(cnt %16 )))vhost_poll_queue (&vq->poll);rcu_read_unlock_bh ();@@ -386,6 +388,12 @@ static void handle_tx(struct vhost_net *net)vhost_disable_notify (&net->dev, vq);continue ;drivers/vhost/net.c static void handle_tx(struct vhost_net *net) · line 399
Quoted 10 lines
drivers/vhost/net.c +6
-0} +/* 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 · 3 lines
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
Quoted 58 lines · Jason Wang
Quoted 31 lines · Qin Chuanyu
Quoted 20 lines · Jason Wang
Quoted 9 lines · Qin Chuanyu
guest kick vhost base on vring flag status and get perfermance improved,
vhost_zerocopy_callbackcould do this in the same way, asvirtqueue_enable_cbneed one more check after change the status ofavail_ringflags, vhost also do the same thing aftervhost_enable_notifytest result list as below:
guest and host: suse11sp3, netperf, intel 2.4GHzQuoted 9 lines
+-------+----------+---------+----------+---------+ | | old | new | +-------+----------+---------+----------+---------+ | UDP | Gbit/s | PPS | Gbit/s | PPS | | 256 | 0.74805 | 321309 | 0.77933 | 334743 | | 512 | 1.42 | 328475 | 1.44 | 333550 | | 1024 | 2.79 | 334426 | 2.81 | 336986 | | 1460 | 3.71 | 316215 | 4.02 | 342325 | +-------+----------+---------+----------+---------+Quoted 1 line
Looks good, do you have cpu utilization number?
+------+----------+--------+----------+--------+--------+---------+ | | 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
after change, less cpu has been used.
Thanks
Quoted 23 lines
Quoted 23 lines
Quoted 16 lines
Signed-off-by Chuanyu Qin <[email protected]>--- drivers/vhost/net.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)drivers/vhost/net.c +0
-0@@ -322,7 +322,9 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) * (the value 16 here is more or less arbitrary, it's tuned totrigger
* less than 10% of times).
*/drivers/vhost/net.c static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) · line 325
Quoted 2 lines
drivers/vhost/net.c +1
-1-if (cnt <=1 || !(cnt %16 )) +smp_rmb ();Better add a comment to explain why this is needed.
Looks like what you need is a
smp_mb()here to make sure the len is updated before testingvq->used_flags?
Quoted 8 lines
Quoted 2 lines
I wanner make sure the
used_flagsis updated, issmp_rmb()enough? or asmp_mb()is needed?
used_flagswas guaranteed to be updated aftersmp_mb()invhost_enable_notify(). Andvhost_net_ubuf_put()does aatomic_sub_return()which implements memory barrier. So looks like there's no need for an extra one.
atomic_sub_return only do as below:
asm volatile(LOCK_PREFIX "xaddl %0, %1" : "+r" (i), "+m" (v->counter) : : "memory"); return i + __i;
google as below:
The "memory" clobber makes GCC assume that any memory may be arbitrarily read or written by the asm block, so will prevent the compiler from reordering loads or stores across it:
This will cause GCC to not keep memory values cached in registers
across the assembler instruction and not optimize stores or loads to that memory.
(That does not prevent a CPU from reordering loads and stores with respect to another CPU, though; you need real memory barrier instructions for that.)
-----------------------------------------------------------------vhost_zerocopy_callback might be involved in softirq context in another
cpu ,so I think smp_rmb() is still needed, is it right ?
Quoted 32 lines
Quoted 24 lines
Quoted 13 lines
Quoted 13 lines
drivers/vhost/net.c +6
-0+if ((!(vq->used_flags &VRING_USED_F_NO_NOTIFY )) + && (cnt <=1 || !(cnt %16 )))vhost_poll_queue (&vq->poll);rcu_read_unlock_bh ();@@ -386,6 +388,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. +*/ Quoted 9 lines
drivers/vhost/net.c static void handle_tx(struct vhost_net *net) · line 399
Quoted 5 lines
drivers/vhost/net.c +2
-0+if (zcopy) +vhost_zerocopy_signal_used (net, vq);break ; }if (in) {.
Quoted 4 lines
--
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.
Quoted 78 lines · Qin Chuanyu
Quoted 58 lines · Jason Wang
Quoted 31 lines · Qin Chuanyu
Quoted 20 lines · Jason Wang
Quoted 9 lines · Qin Chuanyu
guest kick vhost base on vring flag status and get perfermance improved,
vhost_zerocopy_callbackcould do this in the same way, asvirtqueue_enable_cbneed one more check after change the status ofavail_ringflags, vhost also do the same thing aftervhost_enable_notifytest result list as below:
guest and host: suse11sp3, netperf, intel 2.4GHzQuoted 9 lines
+-------+----------+---------+----------+---------+ | | old | new | +-------+----------+---------+----------+---------+ | UDP | Gbit/s | PPS | Gbit/s | PPS | | 256 | 0.74805 | 321309 | 0.77933 | 334743 | | 512 | 1.42 | 328475 | 1.44 | 333550 | | 1024 | 2.79 | 334426 | 2.81 | 336986 | | 1460 | 3.71 | 316215 | 4.02 | 342325 | +-------+----------+---------+----------+---------+Quoted 1 line
Looks good, do you have cpu utilization number?
+------+----------+--------+----------+--------+--------+---------+ | | 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
after change, less cpu has been used.
Thanks
Quoted 23 lines
Quoted 23 lines
Quoted 16 lines
Signed-off-by Chuanyu Qin <[email protected]>--- drivers/vhost/net.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)drivers/vhost/net.c +0
-0@@ -322,7 +322,9 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) * (the value 16 here is more or less arbitrary, it's tuned totrigger
* less than 10% of times).
*/drivers/vhost/net.c static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success) · line 325
Quoted 2 lines
drivers/vhost/net.c +1
-1-if (cnt <=1 || !(cnt %16 )) +smp_rmb ();Better add a comment to explain why this is needed.
Looks like what you need is a
smp_mb()here to make sure the len is updated before testingvq->used_flags?Quoted 8 lines
Quoted 2 lines
I wanner make sure the
used_flagsis updated, issmp_rmb()enough? or asmp_mb()is needed?
used_flagswas guaranteed to be updated aftersmp_mb()invhost_enable_notify(). Andvhost_net_ubuf_put()does aatomic_sub_return()which implements memory barrier. So looks like there's no need for an extra one.
atomic_sub_returnonly do as below:asm volatile(LOCK_PREFIX "xaddl %0, %1" : "+r" (i), "+m" (v->counter) : : "memory"); return i + __i;google as below:
The "memory" clobber makes GCC assume that any memory may be arbitrarily read or written by the asm block, so will prevent the compiler from reordering loads or stores across it:
Quoted 7 lines
This will cause GCC to not keep memory values cached in registers
across the assembler instruction and not optimize stores or loads to that memory.(That does not prevent a CPU from reordering loads and stores with respect to another CPU, though; you need real memory barrier instructions for that.)
Ok, I thought lock prefix should do this but recheck the manual, it was not a serialize instruction.
Quoted 4 lines
-----------------------------------------------------------------
vhost_zerocopy_callbackmight be involved in softirq context in another cpu ,so I thinksmp_rmb()is still needed, is it right ?
rmb is only used to prevent cpu from reordering read. You need a mb here to prevent the used_flags to be checked before updating the len. Otherwise the you may lost a vhost_poll() consider:
vhost_disable_notify()
[CPU0]
if ((!(vq->used_flags & VRING_USED_F_NO_NOTIFY)) [CPU1] vhost_enable_notify() [CPU0] vhost_zerocopy_singal_used() [CPU0] vq->heads[ubuf->desc].len = success ? [CPU1]
So I think you need a smp_wmb() here.
Quoted 38 lines
Quoted 32 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
drivers/vhost/net.c +7
-0+if ((!(vq->used_flags &VRING_USED_F_NO_NOTIFY )) + && (cnt <=1 || !(cnt %16 )))vhost_poll_queue (&vq->poll);rcu_read_unlock_bh ();@@ -386,6 +388,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)Quoted 14 lines
Quoted 8 lines
drivers/vhost/net.c static void handle_tx(struct vhost_net *net) · line 399
Quoted 4 lines
drivers/vhost/net.c +1
-0+vhost_zerocopy_signal_used (net, vq);break ; }if (in) {.
Signature · 3 lines
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.
Signature · 3 lines
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