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

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> 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! ^^^^

Ah, so RAM_SAVE_FLAG_FULL was left behind but
RAM_SAVE_FLAG_COMPRESS_PAGE was removed, I missed that.

>
>> 
>> > +#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?

Yes, please.

>
>> 
>> 
>> > +
>> >  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
>>