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 <faro...@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:
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?
> {
> 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;
?
>
> @@ -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
>
--
Peter Xu
