Re: [Qemu-devel] [PATCH 2/4] migration: introduce lockless multithreads model

[Xiao Guangrong <guangrong.xiao@gmail.com>]

On 10/28/2018 03:50 PM, Paolo Bonzini wrote:
> On 27/10/2018 01:33, Emilio G. Cota wrote:
>> On Wed, Oct 17, 2018 at 12:10:15 +0200, Paolo Bonzini wrote:
>>> On 16/10/2018 13:10, guangrong.xiao@gmail.com wrote:
>>
>>> An idea: the total number of requests is going to be very small, and a
>>> PtrRing is not the nicest data structure for multiple producer/single
>>> consumer.  So you could instead:
>> (snip)
>>> - now that you have request indices, you can replace the completion
>>> ptr_ring with a bitmap, and set a bit in the bitmap with set_bit_atomic
>>> to report completion.  On the writer side you use find_next_bit to find
>> (snip)
>>> Emilio, can you review the above ideas?
>>
>> Sorry it took me a while to go through this.
>>
>> I like your suggestions. Just one nit; I'm not sure I understood
>> the use case very well, but I think using a bitmap to signal
>> completion might be suboptimal, since we'd have several
>> thread spinning on the same cacheline yet caring about
>> different bits.
> 
> Requests are asynchronous, the bitmap is only used to find a free
> submission slot.  You're right that the bitmap can bounce across
> processors, but I'm not sure how else you would do that because you
> don't know in advance how many submitting threads you have.  It wouldn't
> be any worse if there was a spinlock.
> 
> However, in the migration case there is only one submitting thread, so
> it's okay. :)
> 

Yup.

The cache contention only exists in the work threads, the sumbiter thread
is totally free who is the main migration thread. Making the main thread
be faster is good.

> Paolo
> 
>> Xiao: a couple of suggestions
>>
>> - Since you'll be adding a generic module, make its commit and
>>    description self-contained. That is, mentioning in the
>>    log that this will be used for migration is fine, but please
>>    describe the module (and the assumptions it makes about its
>>    users) in general, so that someone that doesn't know anything
>>    about migration can still understand this module (and hopefully
>>    adopt it for other use cases).

Good to me, i will add more detailed description for this module in
the next version.

>>
>> - I'd like to see a simple test program (or rather, benchmark)
>>    that shows how this works. This benchmark would be completely
>>    unrelated to migration; it should just be a simple test of
>>    the performance/scalability of this module.
>>    Having this benchmark would help (1) discuss and quantitately
>>    evaluate modifications to the module, and (2) help others to
>>    quickly understand what the module does.
>>    See tests/qht-bench.c for an example.
>>

Can not agree with you more, will do. :)

Thank you, Emilio and Paolo!