qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH RESEND v6 0/5] migration: reduce time of loading non-iterable


From: Peter Xu
Subject: Re: [PATCH RESEND v6 0/5] migration: reduce time of loading non-iterable vmstate
Date: Tue, 7 Mar 2023 12:04:36 -0500

On Tue, Mar 07, 2023 at 09:24:31PM +0800, Chuang Xu wrote:
> > Why do we need address_space_get_flatview_rcu()?  I'm not sure whether you
> 
> address_space_cahce_init() uses address_space_get_flatview() to acquire
> a ref-ed flatview. If we want to use address_space_to_flatview_rcu() and
> make the flatview ref-ed, maybe we need to add 
> address_space_get_flatview_rcu()?
> Or if we use address_space_to_flatview_rcu() directly, then we'll get a
> unref-ed flatview, I'm not sure wheather it's safe in other cases.. At least
> I don't want the assertion to be triggered one day.

No we can't touch that, afaict.  It was a real fix (447b0d0b9e) to a real
bug.

What I meant is we make address_space_get_flatview() always use
address_space_to_flatview_rcu().

I think it's safe, if you see the current callers of it (after my patch 1
fixed version applied):

memory_region_sync_dirty_bitmap[2249] view = address_space_get_flatview(as);
memory_region_update_coalesced_range[2457] view = 
address_space_get_flatview(as);
memory_region_clear_dirty_bitmap[2285] view = address_space_get_flatview(as);
mtree_info_flatview[3418]      view = address_space_get_flatview(as);
address_space_cache_init[3350] cache->fv = address_space_get_flatview(as);

Case 5 is what we're targeting for which I think it's fine. Logically any
commit() hook should just use address_space_get_flatview_raw() to reference
the flatview, but at least address_space_cache_init() is also called in
non-BQL sections, so _rcu is the only thing we can use here, iiuc..

Case 1-3 is never called during vm load, so I think it's safe.

Case 4 can be accessing stalled flatview but I assume fine since it's just
for debugging. E.g. if someone tries "info mtree -f" during vm loading
after your patch applied, then one can see stalled info.  I don't think it
can even happen because so far "info mtree" should also take BQL.. so it'll
be blocked until vm load completes.

The whole thing is still tricky.  I didn't yet make my mind through on how
to make it very clean, I think it's slightly beyond what this series does
and I need some more thinkings (or suggestions from others).

-- 
Peter Xu




reply via email to

[Prev in Thread] Current Thread [Next in Thread]