Re: [PATCH] migration: Move bitmap_mutex out of migration_bitmap_clear_dirty()

[Peter Xu <peterx@redhat.com>]

On Thu, Jul 08, 2021 at 02:49:51AM +0000, Wang, Wei W wrote:
> On Thursday, July 8, 2021 12:44 AM, Peter Xu wrote:
> > > > Not to mention the hard migration issues are mostly with non-idle
> > > > guest, in that case having the balloon in the guest will be
> > > > disastrous from this pov since it'll start to take mutex for each
> > > > page, while balloon would hardly report anything valid since most guest pages
> > are being used.
> > >
> > > If no pages are reported, migration thread wouldn't wait on the lock then.
> > 
> > Yes I think this is the place I didn't make myself clear.  It's not about sleeping, it's
> > about the cmpxchg being expensive already when the vm is huge.
> 
> OK.
> How did you root cause that it's caused by cmpxchg, instead of lock contention (i.e. syscall and sleep) or
> some other code inside pthread_mutex_lock(). Do you have cycles about cmpxchg v.s. cycles of pthread_mutex_lock()?

We've got "perf top -g" showing a huge amount of stacks lying in
pthread_mutex_lock().

> 
> I check the implementation of pthread_mutex_lock(). The code path for lock acquire is long. QemuSpin looks more efficient.
> (probably we also don’t want migration thread to sleep in any case)

I didn't check it, but I really hoped it should be very close to a spinlock
version when it's not contended.  We should be careful on using spin locks in
userspace, e.g., with that moving clear log into critical section will be too
much and actuall close to "wrong", imho, because the kvm memslot lock inside is
sleepable.

I think it's very fine to have migration thread sleep.  IMHO we need explicit
justification for each mutex to be converted to a spinlock in userspace.  So
far I don't see it yet for this bitmap lock.

Frankly I also don't know how spinlock would work reliably without being able
to disable scheduling, then could the thread got scheduled out duing taking the
spinlock?  Would another thread trying to take this lock spinning on this
sleeping task?

> 
> I think it's also better to see the comparison of migration throughput data (i.e. pages per second) in the following cases, before we make a decision:
> - per-page mutex
> - per-page spinlock
> - 50-ms mutex

I don't have the machines, so I can't do this.  I also see this as separate
issues to solve to use spinlock, as I said before I would prefer some
justification first to use it rather than blindly running tests and pick a
patch with higher number.

I also hope we can reach a consensus that we fix one issue at a time.  This
patch already proves itself with some real workloads, I hope we can merge it
first, either with 50ms period or less.

Per-page locking is already against at least my instinction of how this should
be done; I just regreted I didn't see that an issue when reviewing the balloon
patch as I offered the r-b myself.  However I want to make it work as before
then we figure out a better approach on how the lock is taken, and which lock
we should use.  I don't see it a must to do all things in one patch.

Thanks,

-- 
Peter Xu