From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 797C1C35FE4 for ; Tue, 17 Sep 2024 06:49:23 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1sqS0j-0004dy-6N; Tue, 17 Sep 2024 02:48:29 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1sqS0g-0004dP-GE for qemu-devel@nongnu.org; Tue, 17 Sep 2024 02:48:26 -0400 Received: from mail-ot1-x32c.google.com ([2607:f8b0:4864:20::32c]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1sqS0d-00026y-KC for qemu-devel@nongnu.org; Tue, 17 Sep 2024 02:48:26 -0400 Received: by mail-ot1-x32c.google.com with SMTP id 46e09a7af769-710f0415ac8so2997605a34.1 for ; Mon, 16 Sep 2024 23:48:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=smartx-com.20230601.gappssmtp.com; s=20230601; t=1726555701; x=1727160501; darn=nongnu.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=jGKo0x8y/Urg5U2kUSiOyJdftvMwPNC5ZvSXQGTbFng=; b=DIiAlRSPUpUUwwCvKxPN1BjkW41I0Uc0W3b6BGFiudsf+rxSxHR9qKVxPXGtrw7SwA AwSmnyINH3Bso9FmnIG8I2UuHtYBKJA08q94AN+TUxpbiic9IMH86G6vU04Dajx2fKGz nW19TzCico18KDc7Xw3bafWQrgYKmo6XpuE7eq+iyH9NqLJb9Y6v9YBm7d2bicTdWzSU 0elhed5O7pjt46rKRgrjeDQvA2luQV+RZ8VApvwKKeysBShfhdUo7027L4C57QjW3Jnb WKS5CpPs8i4AqLzFDnX4MhNsVWfJ1HIVsYACnf7K0yN399uop5o2SKxWl0U9Nm5AfAuW QWrQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726555701; x=1727160501; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=jGKo0x8y/Urg5U2kUSiOyJdftvMwPNC5ZvSXQGTbFng=; b=iZEPeNqBHSDKJh/uE058s3z2j2UKVFDjgiyPwhPiWelgw8BsN6wscq//7LeXLLOxtJ rVBsdWzbq6tp/zgbszaH6XcmnigY2TlxC9AfySH2d8ybw2blBtf7wwhhfxceXo+pNitA 1k67dDNUGkdMvQgLHIbIZrT20HIPjJmSlJEQpunWMTjMtVDLOnzDs2JXZY/NoXC07cU5 em6DEJa0HjuCwex10ENPsz9T0jvsFxENAeXKuK1owXq+hSvUC2J3NF1gV9IKYiKSP8d3 Cv9L5aV57+FK6XXTuF6wKaTNz8znImvi8PL1F2IL7XZnr9sN59P8EECQV1PAPkO/Grj4 rHvA== X-Gm-Message-State: AOJu0YzSDZeCtry+T6F4hgbS/kf5+QTU63gCYHpvQvraXCogNFOr3n5T UZAI2KTE48qzrz7mB9tuYpUpfIeY59m4/GcRcibwEw+kPL6SbZDshPWUtaB4erE565ZafGYwgt0 ubr+BYbbbzNTZg3a1anPxZ+yIdojqQ+auFjFziw== X-Google-Smtp-Source: AGHT+IEzVLgzq3lWhx9zxnhW1GDSklwB/pRQQo7rYk/r1MkgejNyuLrGvQW5QAa2OexdKU7OYh7OR0qKnDBVp+LEsAE= X-Received: by 2002:a05:6870:c14e:b0:277:d195:ab88 with SMTP id 586e51a60fabf-27c3f627623mr10223690fac.32.1726555700205; Mon, 16 Sep 2024 23:48:20 -0700 (PDT) MIME-Version: 1.0 References: <531750c8d7b6c09f877b5f335a60fab402c168be.1726390098.git.yong.huang@smartx.com> <87msk7z4l3.fsf@suse.de> In-Reply-To: <87msk7z4l3.fsf@suse.de> From: Yong Huang Date: Tue, 17 Sep 2024 14:48:03 +0800 Message-ID: Subject: Re: [PATCH v1 1/7] migration: Introduce structs for background sync To: Fabiano Rosas Cc: qemu-devel@nongnu.org, Peter Xu , Eric Blake , Markus Armbruster , David Hildenbrand , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= , Paolo Bonzini Content-Type: multipart/alternative; boundary="00000000000032372f06224b13b5" Received-SPF: pass client-ip=2607:f8b0:4864:20::32c; envelope-from=yong.huang@smartx.com; helo=mail-ot1-x32c.google.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org --00000000000032372f06224b13b5 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Sep 17, 2024 at 5:11=E2=80=AFAM Fabiano Rosas wro= te: > Hyman Huang writes: > > > shadow_bmap, iter_bmap and iter_dirty_pages are introduced > > to satisfy the need for background sync. > > > > Meanwhile, introduce enumeration of sync method. > > > > Signed-off-by: Hyman Huang > > --- > > include/exec/ramblock.h | 45 +++++++++++++++++++++++++++++++++++++++++ > > migration/ram.c | 6 ++++++ > > 2 files changed, 51 insertions(+) > > > > diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h > > index 0babd105c0..0e327bc0ae 100644 > > --- a/include/exec/ramblock.h > > +++ b/include/exec/ramblock.h > > @@ -24,6 +24,30 @@ > > #include "qemu/rcu.h" > > #include "exec/ramlist.h" > > > > +/* Possible bits for cpu_physical_memory_sync_dirty_bitmap */ > > + > > +/* > > + * The old-fashioned sync, which is, in turn, used for CPU > > + * throttle and memory transfer. > Using the traditional sync method, the page sending logic iterates the "bmap" to transfer dirty pages while the CPU throttle logic counts the amount of new dirty pages and detects convergence. There are two uses for "bmap". Using the modern sync method, "bmap" is used for transfer dirty pages and "iter_bmap" is used to track new dirty pages. > I'm not sure I follow what "in turn" is supposed to mean in this > sentence. Could you clarify? > Here I want to express "in sequence". But failed obviously. :( > > > + */ > > +#define RAMBLOCK_SYN_LEGACY_ITER (1U << 0) > > So ITER is as opposed to background? I'm a bit confused with the terms. > Yes. > > > + > > +/* > > + * The modern sync, which is, in turn, used for CPU throttle > > + * and memory transfer. > > + */ > > +#define RAMBLOCK_SYN_MODERN_ITER (1U << 1) > > + > > +/* The modern sync, which is used for CPU throttle only */ > > +#define RAMBLOCK_SYN_MODERN_BACKGROUND (1U << 2) > > What's the plan for the "legacy" part? To be removed soon? Do we want to > remove it now? Maybe better to not use the modern/legacy terms unless we > want to give the impression that the legacy one is discontinued. > The bitmap they utilized to track the dirty page information was the distinction between the "legacy iteration" and the "modern iteration." The "iter_bmap" field is used by the "modern iteration" while the "bmap" field is used by the "legacy iteration." Since the refinement is now transparent and there is no API available to change the sync method, I actually want to remove it right now in order to simplify the logic. I'll include it in the next version. > > > + > > +#define RAMBLOCK_SYN_MASK (0x7) > > + > > +typedef enum RAMBlockSynMode { > > + RAMBLOCK_SYN_LEGACY, /* Old-fashined mode */ > > + RAMBLOCK_SYN_MODERN, /* Background-sync-supported mode */ > > +} RAMBlockSynMode; > > I'm also wondering wheter we need this enum + the flags or one of them > would suffice. I'm looking at code like this in the following patches, > for instance: > If we drop the "legacy modern", we can simplify the following logic too. > + if (sync_mode =3D=3D RAMBLOCK_SYN_MODERN) { > + if (background) { > + flag =3D RAMBLOCK_SYN_MODERN_BACKGROUND; > + } else { > + flag =3D RAMBLOCK_SYN_MODERN_ITER; > + } > + } Couldn't we use LEGACY/BG/ITER? > > + > > struct RAMBlock { > > struct rcu_head rcu; > > struct MemoryRegion *mr; > > @@ -89,6 +113,27 @@ struct RAMBlock { > > * could not have been valid on the source. > > */ > > ram_addr_t postcopy_length; > > + > > + /* > > + * Used to backup the bmap during background sync to see whether > any dirty > > + * pages were sent during that time. > > + */ > > + unsigned long *shadow_bmap; > > + > > + /* > > + * The bitmap "bmap," which was initially used for both sync and > memory > > + * transfer, will be replaced by two bitmaps: the previously used > "bmap" > > + * and the recently added "iter_bmap." Only the memory transfer is > > + * conducted with the previously used "bmap"; the recently added > > + * "iter_bmap" is utilized for dirty bitmap sync. > > + */ > > + unsigned long *iter_bmap; > > + > > + /* Number of new dirty pages during iteration */ > > + uint64_t iter_dirty_pages; > > + > > + /* If background sync has shown up during iteration */ > > + bool background_sync_shown_up; > > }; > > #endif > > #endif > > diff --git a/migration/ram.c b/migration/ram.c > > index 67ca3d5d51..f29faa82d6 100644 > > --- a/migration/ram.c > > +++ b/migration/ram.c > > @@ -2362,6 +2362,10 @@ static void ram_bitmaps_destroy(void) > > block->bmap =3D NULL; > > g_free(block->file_bmap); > > block->file_bmap =3D NULL; > > + g_free(block->shadow_bmap); > > + block->shadow_bmap =3D NULL; > > + g_free(block->iter_bmap); > > + block->iter_bmap =3D NULL; > > } > > } > > > > @@ -2753,6 +2757,8 @@ static void ram_list_init_bitmaps(void) > > } > > block->clear_bmap_shift =3D shift; > > block->clear_bmap =3D bitmap_new(clear_bmap_size(pages, > shift)); > > + block->shadow_bmap =3D bitmap_new(pages); > > + block->iter_bmap =3D bitmap_new(pages); > > } > > } > > } > --=20 Best regards --00000000000032372f06224b13b5 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Tue, Sep 17, 20= 24 at 5:11=E2=80=AFAM Fabiano Rosas <= farosas@suse.de> wrote:
Hyman Huang <yong.huang@smartx.= com> writes:

> shadow_bmap, iter_bmap and iter_dirty_pages are introduced
> to satisfy the need for background sync.
>
> Meanwhile, introduce enumeration of sync method.
>
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> ---
>=C2=A0 include/exec/ramblock.h | 45 +++++++++++++++++++++++++++++++++++= ++++++
>=C2=A0 migration/ram.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 6 +++++= +
>=C2=A0 2 files changed, 51 insertions(+)
>
> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
> index 0babd105c0..0e327bc0ae 100644
> --- a/include/exec/ramblock.h
> +++ b/include/exec/ramblock.h
> @@ -24,6 +24,30 @@
>=C2=A0 #include "qemu/rcu.h"
>=C2=A0 #include "exec/ramlist.h"
>=C2=A0
> +/* Possible bits for cpu_physical_memory_sync_dirty_bitmap */
> +
> +/*
> + * The old-fashioned sync, which is, in turn, used for CPU
> + * throttle and memory transfer.

Using the traditional sync method, the page sending logic iterates<= /div>
the "bmap" to transfer dirty pages while the CP= U throttle logic
counts the amount of new dirty pages and= detects convergence.
There are two uses for "bmap&q= uot;.

Using the modern sync method= , "bmap" is used for transfer
dirty pages and &= quot;iter_bmap" is used to track new dirty pages.

=

I'm not sure I follow what "in turn" is supposed to mean in t= his
sentence. Could you clarify?

Here I want to express "in sequence".=C2=A0 But failed obvious= ly. :(
=C2=A0

> + */
> +#define RAMBLOCK_SYN_LEGACY_ITER=C2=A0 =C2=A0(1U << 0)

So ITER is as opposed to background? I'm a bit confused with the terms.=

Yes.=C2=A0
=C2=A0

> +
> +/*
> + * The modern sync, which is, in turn, used for CPU throttle
> + * and memory transfer.
> + */
> +#define RAMBLOCK_SYN_MODERN_ITER=C2=A0 =C2=A0(1U << 1)
> +
> +/* The modern sync, which is used for CPU throttle only */
> +#define RAMBLOCK_SYN_MODERN_BACKGROUND=C2=A0 =C2=A0 (1U << 2)
What's the plan for the "legacy" part? To be removed soon? Do= we want to
remove it now? Maybe better to not use the modern/legacy terms unless we want to give the impression that the legacy one is discontinued.

The bitmap they utilized to track th= e dirty page information was the
distinction between the = "legacy iteration" and the "modern iteration."
The "iter_bmap" field is used by the "modern itera= tion" while the "bmap"
field is used by th= e "legacy iteration."

Since the refinement is now transparent and there is no API available to=
change the sync method, I actually want to remove it rig= ht now in order
to simplify the logic. I'll include i= t in the next version.
=C2=A0

> +
> +#define RAMBLOCK_SYN_MASK=C2=A0 (0x7)
> +
> +typedef enum RAMBlockSynMode {
> +=C2=A0 =C2=A0 RAMBLOCK_SYN_LEGACY, /* Old-fashined mode */
> +=C2=A0 =C2=A0 RAMBLOCK_SYN_MODERN, /* Background-sync-supported mode = */
> +} RAMBlockSynMode;

I'm also wondering wheter we need this enum + the flags or one of them<= br> would suffice. I'm looking at code like this in the following patches,<= br> for instance:

If we drop the "legacy modern&= quot;, we can simplify the following
logic=C2=A0too.=C2=A0<= /span>


+=C2=A0 =C2=A0 if (sync_mode =3D=3D RAMBLOCK_SYN_MODERN) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (background) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 flag =3D RAMBLOCK_SYN_MODERN_BAC= KGROUND;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 } else {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 flag =3D RAMBLOCK_SYN_MODERN_ITE= R;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
+=C2=A0 =C2=A0 }
Couldn't we use LEGACY/BG/ITER?

> +
>=C2=A0 struct RAMBlock {
>=C2=A0 =C2=A0 =C2=A0 struct rcu_head rcu;
>=C2=A0 =C2=A0 =C2=A0 struct MemoryRegion *mr;
> @@ -89,6 +113,27 @@ struct RAMBlock {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0* could not have been valid on the source. >=C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>=C2=A0 =C2=A0 =C2=A0 ram_addr_t postcopy_length;
> +
> +=C2=A0 =C2=A0 /*
> +=C2=A0 =C2=A0 =C2=A0* Used to backup the bmap during background sync = to see whether any dirty
> +=C2=A0 =C2=A0 =C2=A0* pages were sent during that time.
> +=C2=A0 =C2=A0 =C2=A0*/
> +=C2=A0 =C2=A0 unsigned long *shadow_bmap;
> +
> +=C2=A0 =C2=A0 /*
> +=C2=A0 =C2=A0 =C2=A0* The bitmap "bmap," which was initiall= y used for both sync and memory
> +=C2=A0 =C2=A0 =C2=A0* transfer, will be replaced by two bitmaps: the = previously used "bmap"
> +=C2=A0 =C2=A0 =C2=A0* and the recently added "iter_bmap." O= nly the memory transfer is
> +=C2=A0 =C2=A0 =C2=A0* conducted with the previously used "bmap&q= uot;; the recently added
> +=C2=A0 =C2=A0 =C2=A0* "iter_bmap" is utilized for dirty bit= map sync.
> +=C2=A0 =C2=A0 =C2=A0*/
> +=C2=A0 =C2=A0 unsigned long *iter_bmap;
> +
> +=C2=A0 =C2=A0 /* Number of new dirty pages during iteration */
> +=C2=A0 =C2=A0 uint64_t iter_dirty_pages;
> +
> +=C2=A0 =C2=A0 /* If background sync has shown up during iteration */<= br> > +=C2=A0 =C2=A0 bool background_sync_shown_up;
>=C2=A0 };
>=C2=A0 #endif
>=C2=A0 #endif
> diff --git a/migration/ram.c b/migration/ram.c
> index 67ca3d5d51..f29faa82d6 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2362,6 +2362,10 @@ static void ram_bitmaps_destroy(void)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 block->bmap =3D NULL;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 g_free(block->file_bmap);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 block->file_bmap =3D NULL;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_free(block->shadow_bmap);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 block->shadow_bmap =3D NULL;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_free(block->iter_bmap);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 block->iter_bmap =3D NULL;
>=C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 }
>=C2=A0
> @@ -2753,6 +2757,8 @@ static void ram_list_init_bitmaps(void)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 block->clear_bmap_s= hift =3D shift;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 block->clear_bmap = =3D bitmap_new(clear_bmap_size(pages, shift));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 block->shadow_bmap =3D b= itmap_new(pages);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 block->iter_bmap =3D bit= map_new(pages);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 }


--
Best regards=
--00000000000032372f06224b13b5--