Re: [PATCH RFC 0/4] memory: Fix (/ Discuss) a few rcu issues

[Peter Xu]

On Fri, Mar 03, 2023 at 10:10:12AM +0100, David Hildenbrand wrote:
> On 02.03.23 22:50, Peter Xu wrote:
> > On Thu, Mar 02, 2023 at 04:11:56PM +0100, David Hildenbrand wrote:
> > > I guess the main concern here would be overhead from gabbing/releasing the
> > > BQL very often, and blocking the BQL while we're eventually in the kernel,
> > > clearing bitmaps, correct?
> > 
> > More or less yes.  I think it's pretty clear we move on with RCU unless
> > extremely necessary (which I don't think..), then it's about how to fix the
> > bug so rcu safety guaranteed.
> 
> What about an additional simple lock?
> 
> Like:
> 
> * register/unregister requires that new notifier lock + BQL
> * traversing notifiers requires either that new lock or the BQL

This will work, but this will also brings us backstep a bit.

I think we shouldn't allow concurrency for notifiers, more below.  It's
more about sometimes QEMU walking the two lists has nothing to do with
notifiers (like memory_region_find_rcu), that's the major uncertainty to
me.  Also on the future plans of using more RCU in QEMU code.

> We simply take the new lock in that problematic function. That would work as
> long as we don't require traversal of the notifiers concurrently -- and as
> long as we have a lot of bouncing back and forth (I don't think we have,
> even in the migration context, or am I wrong?).
> 
> That way we also make sure that each notifier is only called once. I'm not
> 100% sure if all notifiers would expect to be called concurrently.

Yes I think so.  AFAIU most of the notifiers should only be called with BQL
then they'll already be serialized (and hooks normally has yet another
layer of protection like kvm).

Clear log is something special. Afaik it's protected by RAMState's
bitmap_mutex so far, but not always..

The unaccuracy is because clear log can also be triggered outside migration
where there's no context of bitmap_mutex.

But AFAICT concurrent clear log is also fine because it was (somehow
tailored...) for kvm, so it'll anyway be serialized at kvm_slots_lock().
We'll need to be careful when growing log_clear support, though.

-- 
Peter Xu