qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Peter Xu <peterx@redhat.com>
To: Chuang Xu <xuchuangxclwt@bytedance.com>
Cc: qemu-devel@nongnu.org, dgilbert@redhat.com, quintela@redhat.com,
	pbonzini@redhat.com, david@redhat.com, philmd@linaro.org,
	zhouyibo@bytedance.com
Subject: Re: [PATCH RESEND v6 0/5] migration: reduce time of loading non-iterable vmstate
Date: Tue, 7 Mar 2023 12:04:36 -0500	[thread overview]
Message-ID: <ZAdupAAJjbSbJiss@x1n> (raw)
In-Reply-To: <f6e36da0-d20b-af80-4239-5bb59b97f530@bytedance.com>

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	other threads:[~2023-03-07 17:04 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-03 10:56 [PATCH RESEND v6 0/5] migration: reduce time of loading non-iterable vmstate Chuang Xu
2023-03-03 10:56 ` [PATCH RESEND v6 1/5] memory: Reference as->current_map directly in memory commit Chuang Xu
2023-03-03 10:56 ` [PATCH RESEND v6 2/5] rcu: Introduce rcu_read_is_locked() Chuang Xu
2023-03-03 10:56 ` [PATCH RESEND v6 3/5] memory: Introduce memory_region_transaction_do_commit() Chuang Xu
2023-03-03 10:56 ` [PATCH RESEND v6 4/5] memory: Add sanity check in address_space_to_flatview Chuang Xu
2023-03-03 10:56 ` [PATCH RESEND v6 5/5] migration: Reduce time of loading non-iterable vmstate Chuang Xu
2023-03-05 22:05 ` [PATCH RESEND v6 0/5] migration: reduce " Peter Xu
2023-03-06 12:48   ` Chuang Xu
2023-03-06 20:48     ` Peter Xu
2023-03-07 13:24       ` Chuang Xu
2023-03-07 17:04         ` Peter Xu [this message]
2023-03-08 14:03           ` Chuang Xu
2023-03-08 14:58             ` Peter Xu
2023-03-08 15:27               ` Chuang Xu
2023-03-08 15:46                 ` Peter Xu
2023-03-09 14:52                   ` Chuang Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZAdupAAJjbSbJiss@x1n \
    --to=peterx@redhat.com \
    --cc=david@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=philmd@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    --cc=xuchuangxclwt@bytedance.com \
    --cc=zhouyibo@bytedance.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).