Re: [PATCH] vhost: make vhost_zerocopy_callback more efficient by poll_queue base on vhost status

[Qin Chuanyu <qinchuanyu@huawei.com>]

On 2014/2/25 16:13, Jason Wang wrote:
> On 02/25/2014 03:53 PM, Qin Chuanyu wrote:
>> On 2014/2/25 15:38, Jason Wang wrote:
>>> On 02/25/2014 02:55 PM, Qin Chuanyu wrote:
>>>> 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  |
>>>> +-------+----------+---------+----------+---------+
>>>
>>> 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.
>
> Thanks
>>>>
>>>> Signed-off-by: Chuanyu Qin <qinchuanyu@huawei.com>
>>>> ---
>>>>    drivers/vhost/net.c |   10 +++++++++-
>>>>    1 files changed, 9 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>>>> index a0fa5de..9bc0a15 100644
>>>> --- a/drivers/vhost/net.c
>>>> +++ b/drivers/vhost/net.c
>>>> @@ -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();
>>>
>>> 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?
>> I wanner make sure the used_flags is updated, is smp_rmb() enough?
>> or a smp_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.
>
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 ?

>>>> +    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) {
>>>
>>>
>>> .
>>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> .
>