On Fri, Dec 06, 2024 at 10:43:31AM -0300, Fabiano Rosas wrote:
> Peter Xu <pet...@redhat.com> writes:
>
> > Firstly, we're going to use the multifd flag soon in multifd code, so ram.c
> > isn't gonna work.
> >
> > Secondly, we have a separate RDMA flag dangling around, which is definitely
> > not obvious. There's one comment that helps, but not too much.
> >
> > We should just put it altogether, so nothing will get overlooked.
> >
> > Signed-off-by: Peter Xu <pet...@redhat.com>
>
> Reviewed-by: Fabiano Rosas <faro...@suse.de>
>
> just some comments about preexisting stuff:
>
> > ---
> > migration/ram.h | 25 +++++++++++++++++++++++++
> > migration/rdma.h | 7 -------
> > migration/ram.c | 21 ---------------------
> > 3 files changed, 25 insertions(+), 28 deletions(-)
> >
> > diff --git a/migration/ram.h b/migration/ram.h
> > index 0d1981f888..cfdcccd266 100644
> > --- a/migration/ram.h
> > +++ b/migration/ram.h
> > @@ -33,6 +33,31 @@
> > #include "exec/cpu-common.h"
> > #include "io/channel.h"
> >
> > +/*
> > + * RAM_SAVE_FLAG_ZERO used to be named RAM_SAVE_FLAG_COMPRESS, it
> > + * worked for pages that were filled with the same char. We switched
> > + * it to only search for the zero value. And to avoid confusion with
> > + * RAM_SAVE_FLAG_COMPRESS_PAGE just rename it.
> > + *
> > + * RAM_SAVE_FLAG_FULL was obsoleted in 2009.
> > + *
> > + * RAM_SAVE_FLAG_COMPRESS_PAGE (0x100) was removed in QEMU 9.1.
>
> Aren't these already covered by git log? The comment makes it seem like
> some special situation, but I think we're just documenting history here,
> no?
I guess so.
Maybe still useful when we hit a bug that some ancient QEMU manually
migrates to a new one and hit a weird 0x100 message.
>
> > + */
> > +#define RAM_SAVE_FLAG_FULL 0x01
> > +#define RAM_SAVE_FLAG_ZERO 0x02
> > +#define RAM_SAVE_FLAG_MEM_SIZE 0x04
> > +#define RAM_SAVE_FLAG_PAGE 0x08
> > +#define RAM_SAVE_FLAG_EOS 0x10
> > +#define RAM_SAVE_FLAG_CONTINUE 0x20
> > +#define RAM_SAVE_FLAG_XBZRLE 0x40
> > +/*
> > + * ONLY USED IN RDMA: Whenever this is found in the data stream, the flags
> > + * will be passed to rdma functions in the incoming-migration side.
> > + */
> > +#define RAM_SAVE_FLAG_HOOK 0x80
>
> No 0x100?
You just asked about it one min ago! ^^^^
>
> > +#define RAM_SAVE_FLAG_MULTIFD_FLUSH 0x200
> > +/* We can't use any flag that is bigger than 0x200 */
>
> Where does that limitation come from again? I know that
> RAM_SAVE_FLAG_MEM_SIZE shares a u64 with something else:
>
> qemu_put_be64(f, ram_bytes_total_with_ignored() |
> RAM_SAVE_FLAG_MEM_SIZE);
>
> For RAM_SAVE_FLAG_ZERO and RAM_SAVE_FLAG_PAGE, it might be a u32 (offset
> is ram_addr_t):
>
> save_page_header(pss, file, pss->block, offset | RAM_SAVE_FLAG_ZERO);
>
> But others just go by themselves:
>
> qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
No matter if it goes by itself, I am guessing migration was initially
developed by assuming each initial 8 bytes is an address, see:
ram_load_precopy():
addr = qemu_get_be64(f);
...
flags = addr & ~TARGET_PAGE_MASK;
addr &= TARGET_PAGE_MASK;
So it must be no more than 0x200, probably because it wants to work with
whatever architectures that have PAGE_SIZE>=1K (which is 0x400). Then the
offset will never use the last 10 bits.
Wanna me to add a comment for that in this patch?
>
>
> > +
> > extern XBZRLECacheStats xbzrle_counters;
> >
> > /* Should be holding either ram_list.mutex, or the RCU lock. */
> > diff --git a/migration/rdma.h b/migration/rdma.h
> > index a8d27f33b8..f55f28bbed 100644
> > --- a/migration/rdma.h
> > +++ b/migration/rdma.h
> > @@ -33,13 +33,6 @@ void rdma_start_incoming_migration(InetSocketAddress
> > *host_port, Error **errp);
> > #define RAM_CONTROL_ROUND 1
> > #define RAM_CONTROL_FINISH 3
> >
> > -/*
> > - * Whenever this is found in the data stream, the flags
> > - * will be passed to rdma functions in the incoming-migration
> > - * side.
> > - */
> > -#define RAM_SAVE_FLAG_HOOK 0x80
> > -
> > #define RAM_SAVE_CONTROL_NOT_SUPP -1000
> > #define RAM_SAVE_CONTROL_DELAYED -2000
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 7284c34bd8..44010ff325 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -71,27 +71,6 @@
> > /***********************************************************/
> > /* ram save/restore */
> >
> > -/*
> > - * RAM_SAVE_FLAG_ZERO used to be named RAM_SAVE_FLAG_COMPRESS, it
> > - * worked for pages that were filled with the same char. We switched
> > - * it to only search for the zero value. And to avoid confusion with
> > - * RAM_SAVE_FLAG_COMPRESS_PAGE just rename it.
> > - *
> > - * RAM_SAVE_FLAG_FULL was obsoleted in 2009.
> > - *
> > - * RAM_SAVE_FLAG_COMPRESS_PAGE (0x100) was removed in QEMU 9.1.
> > - */
> > -#define RAM_SAVE_FLAG_FULL 0x01
> > -#define RAM_SAVE_FLAG_ZERO 0x02
> > -#define RAM_SAVE_FLAG_MEM_SIZE 0x04
> > -#define RAM_SAVE_FLAG_PAGE 0x08
> > -#define RAM_SAVE_FLAG_EOS 0x10
> > -#define RAM_SAVE_FLAG_CONTINUE 0x20
> > -#define RAM_SAVE_FLAG_XBZRLE 0x40
> > -/* 0x80 is reserved in rdma.h for RAM_SAVE_FLAG_HOOK */
> > -#define RAM_SAVE_FLAG_MULTIFD_FLUSH 0x200
> > -/* We can't use any flag that is bigger than 0x200 */
> > -
> > /*
> > * mapped-ram migration supports O_DIRECT, so we need to make sure the
> > * userspace buffer, the IO operation size and the file offset are
>
--
Peter Xu
