| Message ID | 20240801123516.4498-14-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | migration/multifd: Remove multifd_send_state->pages | expand |
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: 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 <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 >>
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) { 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; } @@ -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);
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(-)