Re: [PATCH v3 13/14] migration/multifd: Register nocomp ops dynamically

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Aug 01, 2024 at 09:35:15AM -0300, Fabiano Rosas wrote:
>> Prior to moving the ram code into multifd-ram.c, change the code to
>> register the nocomp ops dynamically so we don't need to have the ops
>> structure defined in multifd.c.
>> 
>> While here, rename s/nocomp/ram/ and remove the docstrings which are
>> mostly useless (if anything, it's the function pointers in multifd.h
>> that should be documented like that).
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/multifd.c | 101 ++++++++++++--------------------------------
>>  1 file changed, 28 insertions(+), 73 deletions(-)
>> 
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index c25ab4924c..d5be784b6f 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -167,15 +167,7 @@ static void multifd_set_file_bitmap(MultiFDSendParams 
>> *p)
>>      }
>>  }
>>  
>> -/* Multifd without compression */
>> -
>> -/**
>> - * nocomp_send_setup: setup send side
>> - *
>> - * @p: Params for the channel that we are using
>> - * @errp: pointer to an error
>> - */
>> -static int nocomp_send_setup(MultiFDSendParams *p, Error **errp)
>> +static int ram_send_setup(MultiFDSendParams *p, Error **errp)
>
> "ram" as a prefix sounds inaccurate to me.  Personally I even preferred the
> old name "nocomp" because it says there's no compression.
>
> Here "ram_send_setup" is at the same level against e.g. "zlib_send_setup".
> It sounds like zlib isn't for ram, but it is..
>
> Do you perhaps dislike the "nocomp" term?  How about:

I don't mind. I almost left nocomp intact, but thought it would be
better to match the new file name (multifd-ram.c).

>
>   multifd_plain_send_setup()
>
> Just to do s/nocomp/plain/?  Or "raw"?
>
> We do have two flavours here at least:
>
> *** migration/multifd-qpl.c:
> <global>[755]                  .send_setup = multifd_qpl_send_setup,
>
> *** migration/multifd-ram.c:
> <global>[387]                  .send_setup = ram_send_setup,
>
> *** migration/multifd-uadk.c:
> <global>[364]                  .send_setup = multifd_uadk_send_setup,
>
> *** migration/multifd-zlib.c:
> <global>[338]                  .send_setup = zlib_send_setup,
>
> *** migration/multifd-zstd.c:
> <global>[326]                  .send_setup = zstd_send_setup,
>
> It might makes sense to all prefix them with "multifd_", just to follow
> gpl/uadk?

Yep.

>
>>  {
>>      uint32_t page_count = multifd_ram_page_count();
>>  
>> @@ -193,15 +185,7 @@ static int nocomp_send_setup(MultiFDSendParams *p, 
>> Error **errp)
>>      return 0;
>>  }
>>  
>> -/**
>> - * nocomp_send_cleanup: cleanup send side
>> - *
>> - * For no compression this function does nothing.
>> - *
>> - * @p: Params for the channel that we are using
>> - * @errp: pointer to an error
>> - */
>> -static void nocomp_send_cleanup(MultiFDSendParams *p, Error **errp)
>> +static void ram_send_cleanup(MultiFDSendParams *p, Error **errp)
>>  {
>>      g_free(p->iov);
>>      p->iov = NULL;
>> @@ -222,18 +206,7 @@ static void multifd_send_prepare_iovs(MultiFDSendParams 
>> *p)
>>      p->next_packet_size = pages->normal_num * page_size;
>>  }
>>  
>> -/**
>> - * nocomp_send_prepare: prepare date to be able to send
>> - *
>> - * For no compression we just have to calculate the size of the
>> - * packet.
>> - *
>> - * Returns 0 for success or -1 for error
>> - *
>> - * @p: Params for the channel that we are using
>> - * @errp: pointer to an error
>> - */
>> -static int nocomp_send_prepare(MultiFDSendParams *p, Error **errp)
>> +static int ram_send_prepare(MultiFDSendParams *p, Error **errp)
>>  {
>>      bool use_zero_copy_send = migrate_zero_copy_send();
>>      int ret;
>> @@ -272,46 +245,19 @@ static int nocomp_send_prepare(MultiFDSendParams *p, 
>> Error **errp)
>>      return 0;
>>  }
>>  
>> -/**
>> - * nocomp_recv_setup: setup receive side
>> - *
>> - * For no compression this function does nothing.
>> - *
>> - * Returns 0 for success or -1 for error
>> - *
>> - * @p: Params for the channel that we are using
>> - * @errp: pointer to an error
>> - */
>> -static int nocomp_recv_setup(MultiFDRecvParams *p, Error **errp)
>> +static int ram_recv_setup(MultiFDRecvParams *p, Error **errp)
>>  {
>>      p->iov = g_new0(struct iovec, multifd_ram_page_count());
>>      return 0;
>>  }
>>  
>> -/**
>> - * nocomp_recv_cleanup: setup receive side
>> - *
>> - * For no compression this function does nothing.
>> - *
>> - * @p: Params for the channel that we are using
>> - */
>> -static void nocomp_recv_cleanup(MultiFDRecvParams *p)
>> +static void ram_recv_cleanup(MultiFDRecvParams *p)
>>  {
>>      g_free(p->iov);
>>      p->iov = NULL;
>>  }
>>  
>> -/**
>> - * nocomp_recv: read the data from the channel
>> - *
>> - * For no compression we just need to read things into the correct place.
>> - *
>> - * Returns 0 for success or -1 for error
>> - *
>> - * @p: Params for the channel that we are using
>> - * @errp: pointer to an error
>> - */
>> -static int nocomp_recv(MultiFDRecvParams *p, Error **errp)
>> +static int ram_recv(MultiFDRecvParams *p, Error **errp)
>>  {
>>      uint32_t flags;
>>  
>> @@ -341,22 +287,15 @@ static int nocomp_recv(MultiFDRecvParams *p, Error 
>> **errp)
>>      return qio_channel_readv_all(p->c, p->iov, p->normal_num, errp);
>>  }
>>  
>> -static MultiFDMethods multifd_nocomp_ops = {
>> -    .send_setup = nocomp_send_setup,
>> -    .send_cleanup = nocomp_send_cleanup,
>> -    .send_prepare = nocomp_send_prepare,
>> -    .recv_setup = nocomp_recv_setup,
>> -    .recv_cleanup = nocomp_recv_cleanup,
>> -    .recv = nocomp_recv
>> -};
>> -
>> -static MultiFDMethods *multifd_ops[MULTIFD_COMPRESSION__MAX] = {
>> -    [MULTIFD_COMPRESSION_NONE] = &multifd_nocomp_ops,
>> -};
>> +static MultiFDMethods *multifd_ops[MULTIFD_COMPRESSION__MAX] = {};
>>  
>>  void multifd_register_ops(int method, MultiFDMethods *ops)
>>  {
>> -    assert(0 < method && method < MULTIFD_COMPRESSION__MAX);
>> +    if (method == MULTIFD_COMPRESSION_NONE) {
>> +        assert(!multifd_ops[method]);
>> +    } else {
>> +        assert(0 < method && method < MULTIFD_COMPRESSION__MAX);
>> +    }
>>      multifd_ops[method] = ops;
>>  }
>
> The new assertion is a bit paranoid to me.. while checking duplicated
> assignment should at least apply to all if to add.  So.. how about:
>
>   assert(method < MULTIFD_COMPRESSION__MAX);
>   assert(!multifd_ops[method]);
>   multifd_ops[method] = ops;
>
> ?

ok

>
>>  
>> @@ -1755,3 +1694,19 @@ bool multifd_send_prepare_common(MultiFDSendParams *p)
>>  
>>      return true;
>>  }
>> +
>> +static MultiFDMethods multifd_ram_ops = {
>> +    .send_setup = ram_send_setup,
>> +    .send_cleanup = ram_send_cleanup,
>> +    .send_prepare = ram_send_prepare,
>> +    .recv_setup = ram_recv_setup,
>> +    .recv_cleanup = ram_recv_cleanup,
>> +    .recv = ram_recv
>> +};
>> +
>> +static void multifd_ram_register(void)
>> +{
>> +    multifd_register_ops(MULTIFD_COMPRESSION_NONE, &multifd_ram_ops);
>> +}
>> +
>> +migration_init(multifd_ram_register);
>> -- 
>> 2.35.3
>>