[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RFC 0/4] memory: Fix (/ Discuss) a few rcu issues
From: |
Peter Xu |
Subject: |
Re: [PATCH RFC 0/4] memory: Fix (/ Discuss) a few rcu issues |
Date: |
Thu, 2 Mar 2023 09:56:59 -0500 |
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
- Re: [PATCH RFC 0/4] memory: Fix (/ Discuss) a few rcu issues, Peter Xu, 2023/03/01
- Re: [PATCH RFC 0/4] memory: Fix (/ Discuss) a few rcu issues, David Hildenbrand, 2023/03/02
- Re: [PATCH RFC 0/4] memory: Fix (/ Discuss) a few rcu issues, Peter Xu, 2023/03/02
- Re: [PATCH RFC 0/4] memory: Fix (/ Discuss) a few rcu issues,
Peter Xu <=
- Re: [PATCH RFC 0/4] memory: Fix (/ Discuss) a few rcu issues, David Hildenbrand, 2023/03/02
- Re: [PATCH RFC 0/4] memory: Fix (/ Discuss) a few rcu issues, Peter Xu, 2023/03/02
- Re: [PATCH RFC 0/4] memory: Fix (/ Discuss) a few rcu issues, David Hildenbrand, 2023/03/03
- Re: [PATCH RFC 0/4] memory: Fix (/ Discuss) a few rcu issues, Peter Xu, 2023/03/03
- Re: [PATCH RFC 0/4] memory: Fix (/ Discuss) a few rcu issues, David Hildenbrand, 2023/03/03