From: Peter Xu <peterx@redhat.com>
To: Lukas Straub <lukasstraub2@web.de>
Cc: Hailiang Zhang <zhang.zhanghailiang@huawei.com>,
David Hildenbrand <david@redhat.com>,
Juan Quintela <quintela@redhat.com>,
qemu-devel@nongnu.org,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
Wei Wang <wei.w.wang@intel.com>,
Leonardo Bras Soares Passos <lsoaresp@redhat.com>
Subject: Re: [PATCH] migration: Move bitmap_mutex out of migration_bitmap_clear_dirty()
Date: Tue, 6 Jul 2021 14:37:11 -0400 [thread overview]
Message-ID: <YOSi13An2hTDNDhD@t490s> (raw)
In-Reply-To: <20210704161457.17505f40@gecko.fritz.box>
On Sun, Jul 04, 2021 at 04:14:57PM +0200, Lukas Straub wrote:
> On Sat, 3 Jul 2021 18:31:15 +0200
> Lukas Straub <lukasstraub2@web.de> wrote:
>
> > On Wed, 30 Jun 2021 16:08:05 -0400
> > Peter Xu <peterx@redhat.com> wrote:
> >
> > > Taking the mutex every time for each dirty bit to clear is too slow, especially
> > > we'll take/release even if the dirty bit is cleared. So far it's only used to
> > > sync with special cases with qemu_guest_free_page_hint() against migration
> > > thread, nothing really that serious yet. Let's move the lock to be upper.
> > >
> > > There're two callers of migration_bitmap_clear_dirty().
> > >
> > > For migration, move it into ram_save_iterate(). With the help of MAX_WAIT
> > > logic, we'll only run ram_save_iterate() for no more than 50ms-ish time, so
> > > taking the lock once there at the entry. It also means any call sites to
> > > qemu_guest_free_page_hint() can be delayed; but it should be very rare, only
> > > during migration, and I don't see a problem with it.
> > >
> > > For COLO, move it up to colo_flush_ram_cache(). I think COLO forgot to take
> > > that lock even when calling ramblock_sync_dirty_bitmap(), where another example
> > > is migration_bitmap_sync() who took it right. So let the mutex cover both the
> > > ramblock_sync_dirty_bitmap() and migration_bitmap_clear_dirty() calls.
> >
> > Hi,
> > I don't think COLO needs it, colo_flush_ram_cache() only runs on
> > the secondary (incoming) side and AFAIK the bitmap is only set in
> > ram_load_precopy() and they don't run in parallel.
> >
> > Although I'm not sure what ramblock_sync_dirty_bitmap() does. I guess
> > it's only there to make the rest of the migration code happy?
>
> To answer myself, it syncs the dirty bitmap of the guest itself with
> the ramblock. Of course not only changed pages on the primary need to
> be overwritten from the cache, but also changed pages on the secondary
> so the ram content exactly matches the primary's.
>
> Now, I still don't know what would run concurrently there since the
> guest is stopped when colo_flush_ram_cache() runs.
Indeed I know little on COLO so I don't know whether it's needed in practise.
It's just easier to always take the mutex as long as those protected fields are
modified; mutexes always work with single threaded apps anyways.
Or do you prefer me to drop it? I'll need to rely on your colo knowledge to
know whether it's safe.. I don't think common migration code will be run
during colo, then would qemu_guest_free_page_hint() be called for a colo SVM?
If not, it looks safe to drop the mutex indeed.
Thanks,
--
Peter Xu
next prev parent reply other threads:[~2021-07-06 18:38 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-30 20:08 [PATCH] migration: Move bitmap_mutex out of migration_bitmap_clear_dirty() Peter Xu
2021-07-01 4:42 ` Wang, Wei W
2021-07-01 12:51 ` Peter Xu
2021-07-01 14:21 ` David Hildenbrand
2021-07-02 2:48 ` Wang, Wei W
2021-07-02 7:06 ` David Hildenbrand
2021-07-03 2:53 ` Wang, Wei W
2021-07-05 13:41 ` David Hildenbrand
2021-07-06 9:41 ` Wang, Wei W
2021-07-06 10:05 ` David Hildenbrand
2021-07-06 17:39 ` Peter Xu
2021-07-07 12:45 ` Wang, Wei W
2021-07-07 16:45 ` Peter Xu
2021-07-07 23:25 ` Wang, Wei W
2021-07-08 0:21 ` Peter Xu
2021-07-06 17:47 ` Peter Xu
2021-07-07 8:34 ` Wang, Wei W
2021-07-07 16:54 ` Peter Xu
2021-07-08 2:55 ` Wang, Wei W
2021-07-08 18:10 ` Peter Xu
2021-07-02 2:29 ` Wang, Wei W
2021-07-06 17:59 ` Peter Xu
2021-07-07 8:33 ` Wang, Wei W
2021-07-07 16:44 ` Peter Xu
2021-07-08 2:49 ` Wang, Wei W
2021-07-08 18:30 ` Peter Xu
2021-07-09 8:58 ` Wang, Wei W
2021-07-09 14:48 ` Peter Xu
2021-07-13 8:20 ` Wang, Wei W
2021-07-03 16:31 ` Lukas Straub
2021-07-04 14:14 ` Lukas Straub
2021-07-06 18:37 ` Peter Xu [this message]
2021-07-13 8:40 ` Wang, Wei W
2021-07-13 10:22 ` David Hildenbrand
2021-07-14 5:03 ` Wang, Wei W
2021-07-13 15:59 ` Peter Xu
2021-07-14 5:04 ` Wang, Wei W
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=YOSi13An2hTDNDhD@t490s \
--to=peterx@redhat.com \
--cc=david@redhat.com \
--cc=dgilbert@redhat.com \
--cc=lsoaresp@redhat.com \
--cc=lukasstraub2@web.de \
--cc=qemu-devel@nongnu.org \
--cc=quintela@redhat.com \
--cc=wei.w.wang@intel.com \
--cc=zhang.zhanghailiang@huawei.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).