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

[Peter Xu <peterx@redhat.com>]

On Thu, Mar 02, 2023 at 09:45:35AM -0500, Peter Xu wrote:
> On Thu, Mar 02, 2023 at 10:46:56AM +0100, David Hildenbrand wrote:
> > On 25.02.23 17:31, Peter Xu wrote:
> > > [not for merging, but for discussion; this is something I found when
> > >   looking at another issue on Chuang's optimization for migration downtime]
> > > 
> > > Summary: we tried to access memory_listeners, address_spaces, etc. in RCU
> > > way.  However we didn't implement them with RCU-safety. This patchset is
> > > trying to do that; at least making it closer.
> > > 
> > > NOTE!  It's doing it wrongly for now, so please feel free to see this as a
> > > thread to start discussing this problem, as in subject.
> > > 
> > > The core problem here is how to make sure memory listeners will be freed in
> > > RCU ways, per when unlinking them from the global memory_listeners list.
> > 
> > Can you elaborate why we would want to do that? Is there a real reason we
> > cannot hold the BQL when unregistering a listener?
> 
> Yes afaict we must hold BQL when unregister any listener for now.  I added
> an explicit assert in patch 1 for that.
> 
> We want to do that because potentially we have RCU readers accessing these
> two lists, so here taking BQL only is not enough.  We need to release the
> objects after all users are gone.
> 
> We already do that for address spaces, but afaict the listener part was
> overlooked.  The challenge here is how to achieve the same for listeners.
> 
> > 
> > Or could we use any other, more fine-grained, lock to protect the memory
> > listeners?
> > 
> > Naive me would think that any interactions between someone updating the
> > memory listeners, and a listener getting removed, would require some careful
> > synchronization (to not rip a notifier out while someone else notifies --
> > what is the still registered notifier supposed to do with notifications
> > while it is already going away?), instead of doing it via RCU.
> > 
> > I'm all for using RCU if it improves performance and keeps things simple. If
> > RCU is neither required for performance reason and overcomplicates the
> > implementation, maybe using locking is the better choice.
> 
> For ASes, one major user RCU is memory_region_find_rcu().
> 
> For listeners, the only path that doesn't take BQL (afaict) is
> memory_region_clear_dirty_bitmap().  Maybe you'll have some points here on
> the side effect of taking it because it's in either virtio-mem or balloon
> path for page hinting iirc.

Ah I forgot the generic ram save migration also takes RCU here.  So it's
definitely even more challenging (we already hold RCU for ramblocks there,
though).

> 
> In short, so far I don't know whether it's possible to have all paths take
> BQL while not regress anything.
> 
> > 
> > TBH, so far I thought that any memory_listeners register/unregistering
> > *requires* the BQL, and everything else is a BUG.
> 
> Thanks,
> 
> -- 
> Peter Xu

-- 
Peter Xu