Re: [PATCH v2 3/7] migration/ram: Move RAM_SAVE_FLAG* into ram.h

[Peter Xu <peterx@redhat.com>]

On Fri, Dec 06, 2024 at 10:43:31AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@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 <peterx@redhat.com>
> 
> Reviewed-by: Fabiano Rosas <farosas@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