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

[Xiao Guangrong]

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, address@hidden 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!