All of lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.