Message-ID: <1f4701dc-a81c-beed-e458-5bee4ab6f3e5@redhat.com>
Date:   Tue, 8 Jan 2019 18:09:49 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     netdev@...r.kernel.org
Subject: Re: thoughts stac/clac and get user for vhost


On 2019/1/7 下午10:45, Michael S. Tsirkin wrote:
> On Mon, Jan 07, 2019 at 02:54:19PM +0800, Jason Wang wrote:
>> On 2019/1/7 下午1:42, Michael S. Tsirkin wrote:
>>> On Mon, Jan 07, 2019 at 12:26:51PM +0800, Jason Wang wrote:
>>>> On 2019/1/5 上午5:25, Michael S. Tsirkin wrote:
>>>>> On Wed, Jan 02, 2019 at 11:25:14AM +0800, Jason Wang wrote:
>>>>>> On 2018/12/31 上午2:40, Michael S. Tsirkin wrote:
>>>>>>> On Thu, Dec 27, 2018 at 05:55:52PM +0800, Jason Wang wrote:
>>>>>>>> On 2018/12/26 下午11:06, Michael S. Tsirkin wrote:
>>>>>>>>> On Wed, Dec 26, 2018 at 12:03:50PM +0800, Jason Wang wrote:
>>>>>>>>>> On 2018/12/26 上午12:41, Michael S. Tsirkin wrote:
>>>>>>>>>>> Hi!
>>>>>>>>>>> I was just wondering: packed ring batches things naturally.
>>>>>>>>>>> E.g.
>>>>>>>>>>>
>>>>>>>>>>> user_access_begin
>>>>>>>>>>> check descriptor valid
>>>>>>>>>>> smp_rmb
>>>>>>>>>>> copy descriptor
>>>>>>>>>>> user_access_end
>>>>>>>>>> But without speculation on the descriptor (which may only work for in-order
>>>>>>>>>> or even a violation of spec). Only one two access of a single descriptor
>>>>>>>>>> could be batched. For split ring, we can batch more since we know how many
>>>>>>>>>> descriptors is pending. (avail_idx - last_avail_idx).
>>>>>>>>>>
>>>>>>>>>> Anything I miss?
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>>
>>>>>>>>> just check more descriptors in a loop:
>>>>>>>>>
>>>>>>>>>       user_access_begin
>>>>>>>>>       for (i = 0; i < 16; ++i) {
>>>>>>>>> 	 if (!descriptor valid)
>>>>>>>>> 		break;
>>>>>>>>> 	 smp_rmb
>>>>>>>>> 	 copy descriptor
>>>>>>>>>       }
>>>>>>>>>       user_access_end
>>>>>>>>>
>>>>>>>>> you don't really need to know how many there are
>>>>>>>>> ahead of the time as you still copy them 1 by one.
>>>>>>>> So let's see the case of split ring
>>>>>>>>
>>>>>>>>
>>>>>>>> user_access_begin
>>>>>>>>
>>>>>>>> n = avail_idx - last_avail_idx (1)
>>>>>>>>
>>>>>>>> n = MIN(n, 16)
>>>>>>>>
>>>>>>>> smp_rmb
>>>>>>>>
>>>>>>>> read n entries from avail_ring (2)
>>>>>>>>
>>>>>>>> for (i =0; i <n; i++)
>>>>>>>>
>>>>>>>>         copy descriptor (3)
>>>>>>>>
>>>>>>>> user_access_end
>>>>>>>>
>>>>>>>>
>>>>>>>> Consider for the case of heavy workload. So for packed ring, we have 32
>>>>>>>> times of userspace access and 16 times of smp_rmb()
>>>>>>>>
>>>>>>>> For split ring we have
>>>>>>>>
>>>>>>>> (1) 1 time
>>>>>>>>
>>>>>>>> (2) 2 times at most
>>>>>>>>
>>>>>>>> (3) 16 times
>>>>>>>>
>>>>>>>> 19 times of userspace access and 1 times of smp_rmb(). In fact 2 could be
>>>>>>>> eliminated with in order. 3 could be batched completely with in order and
>>>>>>>> partially when out of order.
>>>>>>>>
>>>>>>>> I don't see how packed ring help here especially consider lfence on x86 is
>>>>>>>> more than memory fence, it prevents speculation in fact.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>> So on x86 at least RMB is free, this is why I never bothered optimizing
>>>>>>> it out. Is smp_rmb still worth optimizing out for ARM? Does it cost
>>>>>>> more than the extra indirection in the split ring?
>>>>>> I don't know, but obviously, RMB has a chance to damage the performance more
>>>>>> or less. But even on arch where the RMB is free, packed ring still does not
>>>>>> show obvious advantage.
>>>>> People do measure gains with a PMD on host+guest.
>>>>> So it's a question of optimizing the packed ring implementation in Linux.
>>>> Well, 2%-3% difference is not quite a lot.
>>> People reported a 10% gain with tiny packets, others reported more.
>>
>> Good to know this, any pointer. 2%-3% is the number I got from Jens' cover
>> letter.
> Oh interesting.  Also Jens' cover letter only from an earlier version,
> Jan 29. What happened between these two dates I don't know, worth
> investigating.


Btw, the increased times of userspace memory access looks like the root 
cause of regression of packed ring implementation in vhost. If I manage 
to do some out of spec stuffs to reduce the time, packed ring will be at 
most as fast as split ring. I wonder whether or not it's the same case 
of hardware implementation consider each PCI transaction is not free.


>
>>> Again, packed ring is faster sometimes by a factor of 3x but
>>> virtio is just virtio, there's a lot going on besides
>>> just passing the buffer addresses guest to host,
>>> and a different ring layout won't help with that.
>>>
>>>
>>>> I think it's not hard to let split ring faster have some small optimizations
>>>> on the code itself.
>>>>
>>>> Thanks
>>> Speed up the split ring support in virtio pmd in dpdk? There have been
>>> several people working on that for a while now. It seems more likely
>>> that we can speed up the newer packed ring code.  E.g. things like
>>> prefetch have a much better chance to work will with the packed layout,
>>> with split one it was a wash IIRC.
>>
>> But what happen when in order is implemented for packed ring?
>>
>> I post a patch that increase 10% of PPS with less than 10 lines of code for
>> vhost (bypass the avail ring reading). I have similar patch for dpdk but
>> just don't have time to test it. Similar optimization could be applied to
>> used ring for TX easily.
>>
>> Thanks
> Oh I have no doubt we can speed things up with interface extensions.
>

Ok, let me repost the in order series for vhost.

Thanks


>>>>>>> But my point was really fundamental - if ring accesses are expensive
>>>>>>> then we should batch them.
>>>>>> I don't object the batching, the reason that they are expensive could be:
>>>>>>
>>>>>> 1) unnecessary overhead caused by speculation barrier and check likes SMAP
>>>>>> 2) cache contention
>>>>>>
>>>>>> So it does not conflict with the effort that I did to remove 1). My plan is:
>>>>>> for metadata, try to eliminate all the 1) completely. For data, we can do
>>>>>> batch copying to amortize its effort. For avail/descriptor batching, we can
>>>>>> try to it on top.
>>>>>>
>>>>>>
>>>>>>>      Right now we have an API that gets
>>>>>>> an iovec directly. That limits the optimizations you can do.
>>>>>>>
>>>>>>> The translation works like this:
>>>>>>>
>>>>>>> ring -> valid descriptors -> iovecs
>>>>>>>
>>>>>>> We should have APIs for each step that work in batches.
>>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>>
>>>>>>>>>>> So packed layout should show the gain with this approach.
>>>>>>>>>>> That could be motivation enough to finally enable vhost packed ring
>>>>>>>>>>> support.
>>>>>>>>>>>
>>>>>>>>>>> Thoughts?
>>>>>>>>>>>

Powered by blists - more mailing lists