| Message ID | 20240711215244.19237-9-yichen.wang@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | WIP: Use Intel DSA accelerator to offload zero page checking in multifd live migration. | expand |
On Thu, Jul 11, 2024 at 2:53 PM Yichen Wang <yichen.wang@bytedance.com> wrote: > diff --git a/migration/options.c b/migration/options.c > index 645f55003d..f839493016 100644 > --- a/migration/options.c > +++ b/migration/options.c > @@ -29,6 +29,7 @@ > #include "ram.h" > #include "options.h" > #include "sysemu/kvm.h" > +#include <cpuid.h> > > /* Maximum migrate downtime set to 2000 seconds */ > #define MAX_MIGRATE_DOWNTIME_SECONDS 2000 > @@ -162,6 +163,10 @@ Property migration_properties[] = { > DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState, > parameters.zero_page_detection, > ZERO_PAGE_DETECTION_MULTIFD), > + /* DEFINE_PROP_ARRAY("dsa-accel-path", MigrationState, x, */ > + /* parameters.dsa_accel_path, qdev_prop_string, char *), */ > + /* DEFINE_PROP_STRING("dsa-accel-path", MigrationState, */ > + /* parameters.dsa_accel_path), */ > > /* Migration capabilities */ > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), I changed the dsa-accel-path to be a ['str'], i.e. strList* in C. However, I am having a hard time about how to define the proper properties here. I don't know what MARCO to use and I can't find good examples... Need some guidance about how to proceed. Basically I will need this to pass something like '-global migration.dsa-accel-path="/dev/dsa/wq0.0"' in cmdline, or "migrate_set_parameter dsa-accel-path" in QEMU CLI. Don't know how to pass strList there. Thanks very much!
Yichen Wang <yichen.wang@bytedance.com> writes: > On Thu, Jul 11, 2024 at 2:53 PM Yichen Wang <yichen.wang@bytedance.com> wrote: > >> diff --git a/migration/options.c b/migration/options.c >> index 645f55003d..f839493016 100644 >> --- a/migration/options.c >> +++ b/migration/options.c >> @@ -29,6 +29,7 @@ >> #include "ram.h" >> #include "options.h" >> #include "sysemu/kvm.h" >> +#include <cpuid.h> >> >> /* Maximum migrate downtime set to 2000 seconds */ >> #define MAX_MIGRATE_DOWNTIME_SECONDS 2000 >> @@ -162,6 +163,10 @@ Property migration_properties[] = { >> DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState, >> parameters.zero_page_detection, >> ZERO_PAGE_DETECTION_MULTIFD), >> + /* DEFINE_PROP_ARRAY("dsa-accel-path", MigrationState, x, */ >> + /* parameters.dsa_accel_path, qdev_prop_string, char *), */ This is mostly correct, I think, you just need to create a field in MigrationState to keep the length (instead of x). However, I found out just now that this only works with QMP. Let me ask for other's opinions... >> + /* DEFINE_PROP_STRING("dsa-accel-path", MigrationState, */ >> + /* parameters.dsa_accel_path), */ >> >> /* Migration capabilities */ >> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > > I changed the dsa-accel-path to be a ['str'], i.e. strList* in C. > However, I am having a hard time about how to define the proper > properties here. I don't know what MACRO to use and I can't find good > examples... Need some guidance about how to proceed. Basically I will > need this to pass something like '-global > migration.dsa-accel-path="/dev/dsa/wq0.0"' in cmdline, or > "migrate_set_parameter dsa-accel-path" in QEMU CLI. Don't know how to > pass strList there. > > Thanks very much! @Daniel, @Markus, any idea here? If I'm reading this commit[1] right, it seems we decided to disallow passing of arrays without JSON, which affects -global on the command-line and HMP. 1- b06f8b500d (qdev: Rework array properties based on list visitor, 2023-11-09) QMP shell: (QEMU) migrate-set-parameters dsa-accel-path=['a','b'] {"return": {}} HMP: (qemu) migrate_set_parameter dsa-accel-path "['a','b']" qemu-system-x86_64: ../qapi/string-input-visitor.c:343: parse_type_str: Assertion `siv->lm == LM_NONE' failed. Any recommendation? I believe all migration parameters so far can be set via those means, I don't think we can allow only this one to be QMP-only. Or am I just missing something?
Yichen Wang <yichen.wang@bytedance.com> writes: > From: Hao Xiang <hao.xiang@linux.dev> > > Intel DSA offloading is an optional feature that turns on if > proper hardware and software stack is available. To turn on > DSA offloading in multifd live migration: > > dsa-accel-path="[dsa_dev_path1] [dsa_dev_path2] ... [dsa_dev_pathX]" > > This feature is turned off by default. > > Signed-off-by: Hao Xiang <hao.xiang@linux.dev> > Signed-off-by: Yichen Wang <yichen.wang@bytedance.com> > --- > migration/migration-hmp-cmds.c | 15 ++++++++++- > migration/options.c | 47 ++++++++++++++++++++++++++++++++++ > migration/options.h | 1 + > qapi/migration.json | 32 ++++++++++++++++++++--- > 4 files changed, 90 insertions(+), 5 deletions(-) > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c > index 7d608d26e1..c422db4ecd 100644 > --- a/migration/migration-hmp-cmds.c > +++ b/migration/migration-hmp-cmds.c > @@ -312,7 +312,16 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) > monitor_printf(mon, "%s: '%s'\n", > MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ), > params->tls_authz); > - > + if (params->has_dsa_accel_path) { > + strList *dsa_accel_path = params->dsa_accel_path; > + monitor_printf(mon, "%s:", > + MigrationParameter_str(MIGRATION_PARAMETER_DSA_ACCEL_PATH)); > + while (dsa_accel_path) { > + monitor_printf(mon, " %s", dsa_accel_path->value); > + dsa_accel_path = dsa_accel_path->next; > + } > + monitor_printf(mon, "\n"); > + } > if (params->has_block_bitmap_mapping) { > const BitmapMigrationNodeAliasList *bmnal; > > @@ -563,6 +572,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > p->has_x_checkpoint_delay = true; > visit_type_uint32(v, param, &p->x_checkpoint_delay, &err); > break; > + case MIGRATION_PARAMETER_DSA_ACCEL_PATH: > + p->has_dsa_accel_path = true; > + visit_type_strList(v, param, &p->dsa_accel_path, &err); > + break; > case MIGRATION_PARAMETER_MULTIFD_CHANNELS: > p->has_multifd_channels = true; > visit_type_uint8(v, param, &p->multifd_channels, &err); > diff --git a/migration/options.c b/migration/options.c > index 645f55003d..f839493016 100644 > --- a/migration/options.c > +++ b/migration/options.c > @@ -29,6 +29,7 @@ > #include "ram.h" > #include "options.h" > #include "sysemu/kvm.h" > +#include <cpuid.h> > > /* Maximum migrate downtime set to 2000 seconds */ > #define MAX_MIGRATE_DOWNTIME_SECONDS 2000 > @@ -162,6 +163,10 @@ Property migration_properties[] = { > DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState, > parameters.zero_page_detection, > ZERO_PAGE_DETECTION_MULTIFD), > + /* DEFINE_PROP_ARRAY("dsa-accel-path", MigrationState, x, */ > + /* parameters.dsa_accel_path, qdev_prop_string, char *), */ > + /* DEFINE_PROP_STRING("dsa-accel-path", MigrationState, */ > + /* parameters.dsa_accel_path), */ > > /* Migration capabilities */ > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > @@ -815,6 +820,13 @@ const char *migrate_tls_creds(void) > return s->parameters.tls_creds; > } > > +const strList *migrate_dsa_accel_path(void) > +{ > + MigrationState *s = migrate_get_current(); > + > + return s->parameters.dsa_accel_path; > +} > + > const char *migrate_tls_hostname(void) > { > MigrationState *s = migrate_get_current(); > @@ -926,6 +938,7 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) > params->zero_page_detection = s->parameters.zero_page_detection; > params->has_direct_io = true; > params->direct_io = s->parameters.direct_io; > + params->dsa_accel_path = QAPI_CLONE(strList, s->parameters.dsa_accel_path); > > return params; > } > @@ -934,6 +947,7 @@ void migrate_params_init(MigrationParameters *params) > { > params->tls_hostname = g_strdup(""); > params->tls_creds = g_strdup(""); > + params->dsa_accel_path = NULL; > > /* Set has_* up only for parameter checks */ > params->has_throttle_trigger_threshold = true; > @@ -1137,6 +1151,22 @@ bool migrate_params_check(MigrationParameters *params, Error **errp) > return false; > } > > + if (params->has_zero_page_detection && > + params->zero_page_detection == ZERO_PAGE_DETECTION_DSA_ACCEL) { > +#ifdef CONFIG_DSA_OPT > + unsigned int eax, ebx, ecx, edx; > + /* ENQCMD is indicated by bit 29 of ecx in CPUID leaf 7, subleaf 0. */ > + if (!__get_cpuid_count(7, 0, &eax, &ebx, &ecx, &edx) || > + !(ecx & (1 << 29))) { > + error_setg(errp, "DSA acceleration is not supported by CPU"); > + return false; > + } This should be a function along with the others in dsa.h, then you wouldn't need the ifdef here. > +#else > + error_setg(errp, "DSA acceleration is not enabled"); > + return false; > +#endif > + } > + > return true; > } > > @@ -1247,6 +1277,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params, > if (params->has_direct_io) { > dest->direct_io = params->direct_io; > } > + > + if (params->has_dsa_accel_path) { > + dest->has_dsa_accel_path = true; > + dest->dsa_accel_path = params->dsa_accel_path; > + } > } > > static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > @@ -1376,6 +1411,12 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) > if (params->has_direct_io) { > s->parameters.direct_io = params->direct_io; > } > + if (params->has_dsa_accel_path) { > + qapi_free_strList(s->parameters.dsa_accel_path); > + s->parameters.has_dsa_accel_path = true; > + s->parameters.dsa_accel_path = > + QAPI_CLONE(strList, params->dsa_accel_path); > + } > } > > void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) > @@ -1401,6 +1442,12 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) > params->tls_authz->type = QTYPE_QSTRING; > params->tls_authz->u.s = strdup(""); > } > + /* if (params->dsa_accel_path */ > + /* && params->dsa_accel_path->type == QTYPE_QNULL) { */ > + /* qobject_unref(params->dsa_accel_path->u.n); */ > + /* params->dsa_accel_path->type = QTYPE_QLIST; */ > + /* params->dsa_accel_path->u.s = strdup(""); */ > + /* } */ > > migrate_params_test_apply(params, &tmp); > > diff --git a/migration/options.h b/migration/options.h > index a2397026db..78b9e4080b 100644 > --- a/migration/options.h > +++ b/migration/options.h > @@ -85,6 +85,7 @@ const char *migrate_tls_creds(void); > const char *migrate_tls_hostname(void); > uint64_t migrate_xbzrle_cache_size(void); > ZeroPageDetection migrate_zero_page_detection(void); > +const strList *migrate_dsa_accel_path(void); > > /* parameters helpers */ > > diff --git a/qapi/migration.json b/qapi/migration.json > index 1234bef888..ff41780347 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -619,10 +619,14 @@ > # multifd migration is enabled, else in the main migration thread > # as for @legacy. > # > +# @dsa-accel: Perform zero page checking with the DSA accelerator > +# offloading in multifd sender thread if multifd migration is > +# enabled, else in the main migration thread as for @legacy. > +# > # Since: 9.0 > ## > { 'enum': 'ZeroPageDetection', > - 'data': [ 'none', 'legacy', 'multifd' ] } > + 'data': [ 'none', 'legacy', 'multifd', 'dsa-accel' ] } > > ## > # @BitmapMigrationBitmapAliasTransform: > @@ -825,6 +829,12 @@ > # See description in @ZeroPageDetection. Default is 'multifd'. > # (since 9.0) > # > +# @dsa-accel-path: If enabled, use DSA accelerator offloading for > +# certain memory operations. Enable DSA accelerator for zero > +# page detection offloading by setting the @zero-page-detection > +# to dsa-accel. This parameter defines the dsa device path, and > +# defaults to an empty list. (since 9.2) > +# > # @direct-io: Open migration files with O_DIRECT when possible. This > # only has effect if the @mapped-ram capability is enabled. > # (Since 9.1) > @@ -843,7 +853,7 @@ > 'cpu-throttle-initial', 'cpu-throttle-increment', > 'cpu-throttle-tailslow', > 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', > - 'avail-switchover-bandwidth', 'downtime-limit', > + 'avail-switchover-bandwidth', 'downtime-limit', 'dsa-accel-path', > { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] }, > 'multifd-channels', > 'xbzrle-cache-size', 'max-postcopy-bandwidth', > @@ -1000,6 +1010,12 @@ > # See description in @ZeroPageDetection. Default is 'multifd'. > # (since 9.0) > # > +# @dsa-accel-path: If enabled, use DSA accelerator offloading for > +# certain memory operations. Enable DSA accelerator for zero > +# page detection offloading by setting the @zero-page-detection > +# to dsa-accel. This parameter defines the dsa device path, and > +# defaults to an empty list. (since 9.2) > +# > # @direct-io: Open migration files with O_DIRECT when possible. This > # only has effect if the @mapped-ram capability is enabled. > # (Since 9.1) > @@ -1044,7 +1060,8 @@ > '*vcpu-dirty-limit': 'uint64', > '*mode': 'MigMode', > '*zero-page-detection': 'ZeroPageDetection', > - '*direct-io': 'bool' } } > + '*direct-io': 'bool', > + '*dsa-accel-path': ['str'] } } > > ## > # @migrate-set-parameters: > @@ -1204,6 +1221,12 @@ > # See description in @ZeroPageDetection. Default is 'multifd'. > # (since 9.0) > # > +# @dsa-accel-path: If enabled, use DSA accelerator offloading for > +# certain memory operations. Enable DSA accelerator for zero > +# page detection offloading by setting the @zero-page-detection > +# to dsa-accel. This parameter defines the dsa device path, and > +# defaults to an empty list. (since 9.2) > +# > # @direct-io: Open migration files with O_DIRECT when possible. This > # only has effect if the @mapped-ram capability is enabled. > # (Since 9.1) > @@ -1245,7 +1268,8 @@ > '*vcpu-dirty-limit': 'uint64', > '*mode': 'MigMode', > '*zero-page-detection': 'ZeroPageDetection', > - '*direct-io': 'bool' } } > + '*direct-io': 'bool', > + '*dsa-accel-path': ['str'] } } > > ## > # @query-migrate-parameters:
Fabiano Rosas <farosas@suse.de> writes: > Yichen Wang <yichen.wang@bytedance.com> writes: > >> On Thu, Jul 11, 2024 at 2:53 PM Yichen Wang <yichen.wang@bytedance.com> wrote: >> >>> diff --git a/migration/options.c b/migration/options.c >>> index 645f55003d..f839493016 100644 >>> --- a/migration/options.c >>> +++ b/migration/options.c >>> @@ -29,6 +29,7 @@ >>> #include "ram.h" >>> #include "options.h" >>> #include "sysemu/kvm.h" >>> +#include <cpuid.h> >>> >>> /* Maximum migrate downtime set to 2000 seconds */ >>> #define MAX_MIGRATE_DOWNTIME_SECONDS 2000 >>> @@ -162,6 +163,10 @@ Property migration_properties[] = { >>> DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState, >>> parameters.zero_page_detection, >>> ZERO_PAGE_DETECTION_MULTIFD), >>> + /* DEFINE_PROP_ARRAY("dsa-accel-path", MigrationState, x, */ >>> + /* parameters.dsa_accel_path, qdev_prop_string, char *), */ > > This is mostly correct, I think, you just need to create a field in > MigrationState to keep the length (instead of x). However, I found out > just now that this only works with QMP. Let me ask for other's > opinions... > >>> + /* DEFINE_PROP_STRING("dsa-accel-path", MigrationState, */ >>> + /* parameters.dsa_accel_path), */ >>> >>> /* Migration capabilities */ >>> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), >> >> I changed the dsa-accel-path to be a ['str'], i.e. strList* in C. >> However, I am having a hard time about how to define the proper >> properties here. I don't know what MACRO to use and I can't find good >> examples... Need some guidance about how to proceed. Basically I will >> need this to pass something like '-global >> migration.dsa-accel-path="/dev/dsa/wq0.0"' in cmdline, or >> "migrate_set_parameter dsa-accel-path" in QEMU CLI. Don't know how to >> pass strList there. >> >> Thanks very much! > > @Daniel, @Markus, any idea here? > > If I'm reading this commit[1] right, it seems we decided to disallow > passing of arrays without JSON, which affects -global on the > command-line and HMP. > > 1- b06f8b500d (qdev: Rework array properties based on list visitor, > 2023-11-09) > > QMP shell: > (QEMU) migrate-set-parameters dsa-accel-path=['a','b'] > {"return": {}} > > HMP: > (qemu) migrate_set_parameter dsa-accel-path "['a','b']" > qemu-system-x86_64: ../qapi/string-input-visitor.c:343: parse_type_str: > Assertion `siv->lm == LM_NONE' failed. > > Any recommendation? I believe all migration parameters so far can be set > via those means, I don't think we can allow only this one to be > QMP-only. > > Or am I just missing something? I guess we could just skip adding property like Steve did here: https://lore.kernel.org/r/1719776434-435013-10-git-send-email-steven.sistare@oracle.com
Fabiano Rosas <farosas@suse.de> writes: > Yichen Wang <yichen.wang@bytedance.com> writes: > >> On Thu, Jul 11, 2024 at 2:53 PM Yichen Wang <yichen.wang@bytedance.com> wrote: >> >>> diff --git a/migration/options.c b/migration/options.c >>> index 645f55003d..f839493016 100644 >>> --- a/migration/options.c >>> +++ b/migration/options.c >>> @@ -29,6 +29,7 @@ >>> #include "ram.h" >>> #include "options.h" >>> #include "sysemu/kvm.h" >>> +#include <cpuid.h> >>> >>> /* Maximum migrate downtime set to 2000 seconds */ >>> #define MAX_MIGRATE_DOWNTIME_SECONDS 2000 >>> @@ -162,6 +163,10 @@ Property migration_properties[] = { >>> DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState, >>> parameters.zero_page_detection, >>> ZERO_PAGE_DETECTION_MULTIFD), >>> + /* DEFINE_PROP_ARRAY("dsa-accel-path", MigrationState, x, */ >>> + /* parameters.dsa_accel_path, qdev_prop_string, char *), */ > > This is mostly correct, I think, you just need to create a field in > MigrationState to keep the length (instead of x). However, I found out > just now that this only works with QMP. Let me ask for other's > opinions... > >>> + /* DEFINE_PROP_STRING("dsa-accel-path", MigrationState, */ >>> + /* parameters.dsa_accel_path), */ >>> >>> /* Migration capabilities */ >>> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), >> >> I changed the dsa-accel-path to be a ['str'], i.e. strList* in C. >> However, I am having a hard time about how to define the proper >> properties here. I don't know what MACRO to use and I can't find good >> examples... Need some guidance about how to proceed. Basically I will >> need this to pass something like '-global >> migration.dsa-accel-path="/dev/dsa/wq0.0"' in cmdline, or >> "migrate_set_parameter dsa-accel-path" in QEMU CLI. Don't know how to >> pass strList there. >> >> Thanks very much! > > @Daniel, @Markus, any idea here? > > If I'm reading this commit[1] right, it seems we decided to disallow > passing of arrays without JSON, which affects -global on the > command-line and HMP. > > 1- b06f8b500d (qdev: Rework array properties based on list visitor, > 2023-11-09) > > QMP shell: > (QEMU) migrate-set-parameters dsa-accel-path=['a','b'] > {"return": {}} > > HMP: > (qemu) migrate_set_parameter dsa-accel-path "['a','b']" > qemu-system-x86_64: ../qapi/string-input-visitor.c:343: parse_type_str: > Assertion `siv->lm == LM_NONE' failed. HMP migrate_set_parameter doesn't support JSON. It uses the string input visitor to parse the value, which can only do lists of integers. The string visitors have been thorns in my side since forever. > Any recommendation? I believe all migration parameters so far can be set > via those means, I don't think we can allow only this one to be > QMP-only. > > Or am I just missing something? I don't think the string input visitor can be compatibly extended to arbitrary lists. We could replace HMP migrate_set_parameter by migrate_set_parameters. The new command parses its single argument into a struct MigrateSetParameters with keyval_parse(), qobject_input_visitor_new_keyval(), and visit_type_MigrateSetParameters().
On Wed, Jul 24, 2024 at 7:50 AM Markus Armbruster <armbru@redhat.com> wrote: > > Fabiano Rosas <farosas@suse.de> writes: > > > Yichen Wang <yichen.wang@bytedance.com> writes: > > > >> On Thu, Jul 11, 2024 at 2:53 PM Yichen Wang <yichen.wang@bytedance.com> wrote: > >> > >>> diff --git a/migration/options.c b/migration/options.c > >>> index 645f55003d..f839493016 100644 > >>> --- a/migration/options.c > >>> +++ b/migration/options.c > >>> @@ -29,6 +29,7 @@ > >>> #include "ram.h" > >>> #include "options.h" > >>> #include "sysemu/kvm.h" > >>> +#include <cpuid.h> > >>> > >>> /* Maximum migrate downtime set to 2000 seconds */ > >>> #define MAX_MIGRATE_DOWNTIME_SECONDS 2000 > >>> @@ -162,6 +163,10 @@ Property migration_properties[] = { > >>> DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState, > >>> parameters.zero_page_detection, > >>> ZERO_PAGE_DETECTION_MULTIFD), > >>> + /* DEFINE_PROP_ARRAY("dsa-accel-path", MigrationState, x, */ > >>> + /* parameters.dsa_accel_path, qdev_prop_string, char *), */ > > > > This is mostly correct, I think, you just need to create a field in > > MigrationState to keep the length (instead of x). However, I found out > > just now that this only works with QMP. Let me ask for other's > > opinions... > > > >>> + /* DEFINE_PROP_STRING("dsa-accel-path", MigrationState, */ > >>> + /* parameters.dsa_accel_path), */ > >>> > >>> /* Migration capabilities */ > >>> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > >> > >> I changed the dsa-accel-path to be a ['str'], i.e. strList* in C. > >> However, I am having a hard time about how to define the proper > >> properties here. I don't know what MACRO to use and I can't find good > >> examples... Need some guidance about how to proceed. Basically I will > >> need this to pass something like '-global > >> migration.dsa-accel-path="/dev/dsa/wq0.0"' in cmdline, or > >> "migrate_set_parameter dsa-accel-path" in QEMU CLI. Don't know how to > >> pass strList there. > >> > >> Thanks very much! > > > > @Daniel, @Markus, any idea here? > > > > If I'm reading this commit[1] right, it seems we decided to disallow > > passing of arrays without JSON, which affects -global on the > > command-line and HMP. > > > > 1- b06f8b500d (qdev: Rework array properties based on list visitor, > > 2023-11-09) > > > > QMP shell: > > (QEMU) migrate-set-parameters dsa-accel-path=['a','b'] > > {"return": {}} > > > > HMP: > > (qemu) migrate_set_parameter dsa-accel-path "['a','b']" > > qemu-system-x86_64: ../qapi/string-input-visitor.c:343: parse_type_str: > > Assertion `siv->lm == LM_NONE' failed. > > HMP migrate_set_parameter doesn't support JSON. It uses the string > input visitor to parse the value, which can only do lists of integers. > > The string visitors have been thorns in my side since forever. > > > Any recommendation? I believe all migration parameters so far can be set > > via those means, I don't think we can allow only this one to be > > QMP-only. > > > > Or am I just missing something? > > I don't think the string input visitor can be compatibly extended to > arbitrary lists. > > We could replace HMP migrate_set_parameter by migrate_set_parameters. > The new command parses its single argument into a struct > MigrateSetParameters with keyval_parse(), > qobject_input_visitor_new_keyval(), and > visit_type_MigrateSetParameters(). > I tried Fabiano's suggestion, and put a unit32_t in MigrateState data structure. I got exactly the same: "qemu-system-x86_64.dsa: ../../../qapi/string-input-visitor.c:343: parse_type_str: Assertion `siv->lm == LM_NONE' failed.". Steve's patch is more to be a read-only field from HMP, so probably I can't do that. Markus's suggestion seems to be too heavy for the patch and I took a quick glance and it doesn't seem to be easy to do. So should we revert to the old "str" format instead of strList? Or how should I proceed here?
Yichen Wang <yichen.wang@bytedance.com> writes: > On Wed, Jul 24, 2024 at 7:50 AM Markus Armbruster <armbru@redhat.com> wrote: >> >> Fabiano Rosas <farosas@suse.de> writes: >> >> > Yichen Wang <yichen.wang@bytedance.com> writes: >> > >> >> On Thu, Jul 11, 2024 at 2:53 PM Yichen Wang <yichen.wang@bytedance.com> wrote: >> >> >> >>> diff --git a/migration/options.c b/migration/options.c >> >>> index 645f55003d..f839493016 100644 >> >>> --- a/migration/options.c >> >>> +++ b/migration/options.c >> >>> @@ -29,6 +29,7 @@ >> >>> #include "ram.h" >> >>> #include "options.h" >> >>> #include "sysemu/kvm.h" >> >>> +#include <cpuid.h> >> >>> >> >>> /* Maximum migrate downtime set to 2000 seconds */ >> >>> #define MAX_MIGRATE_DOWNTIME_SECONDS 2000 >> >>> @@ -162,6 +163,10 @@ Property migration_properties[] = { >> >>> DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState, >> >>> parameters.zero_page_detection, >> >>> ZERO_PAGE_DETECTION_MULTIFD), >> >>> + /* DEFINE_PROP_ARRAY("dsa-accel-path", MigrationState, x, */ >> >>> + /* parameters.dsa_accel_path, qdev_prop_string, char *), */ >> > >> > This is mostly correct, I think, you just need to create a field in >> > MigrationState to keep the length (instead of x). However, I found out >> > just now that this only works with QMP. Let me ask for other's >> > opinions... >> > >> >>> + /* DEFINE_PROP_STRING("dsa-accel-path", MigrationState, */ >> >>> + /* parameters.dsa_accel_path), */ >> >>> >> >>> /* Migration capabilities */ >> >>> DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), >> >> >> >> I changed the dsa-accel-path to be a ['str'], i.e. strList* in C. >> >> However, I am having a hard time about how to define the proper >> >> properties here. I don't know what MACRO to use and I can't find good >> >> examples... Need some guidance about how to proceed. Basically I will >> >> need this to pass something like '-global >> >> migration.dsa-accel-path="/dev/dsa/wq0.0"' in cmdline, or >> >> "migrate_set_parameter dsa-accel-path" in QEMU CLI. Don't know how to >> >> pass strList there. >> >> >> >> Thanks very much! >> > >> > @Daniel, @Markus, any idea here? >> > >> > If I'm reading this commit[1] right, it seems we decided to disallow >> > passing of arrays without JSON, which affects -global on the >> > command-line and HMP. >> > >> > 1- b06f8b500d (qdev: Rework array properties based on list visitor, >> > 2023-11-09) >> > >> > QMP shell: >> > (QEMU) migrate-set-parameters dsa-accel-path=['a','b'] >> > {"return": {}} >> > >> > HMP: >> > (qemu) migrate_set_parameter dsa-accel-path "['a','b']" >> > qemu-system-x86_64: ../qapi/string-input-visitor.c:343: parse_type_str: >> > Assertion `siv->lm == LM_NONE' failed. >> >> HMP migrate_set_parameter doesn't support JSON. It uses the string >> input visitor to parse the value, which can only do lists of integers. >> >> The string visitors have been thorns in my side since forever. >> >> > Any recommendation? I believe all migration parameters so far can be set >> > via those means, I don't think we can allow only this one to be >> > QMP-only. >> > >> > Or am I just missing something? >> >> I don't think the string input visitor can be compatibly extended to >> arbitrary lists. >> >> We could replace HMP migrate_set_parameter by migrate_set_parameters. >> The new command parses its single argument into a struct >> MigrateSetParameters with keyval_parse(), >> qobject_input_visitor_new_keyval(), and >> visit_type_MigrateSetParameters(). >> > > I tried Fabiano's suggestion, and put a unit32_t in MigrateState data > structure. I got exactly the same: "qemu-system-x86_64.dsa: > ../../../qapi/string-input-visitor.c:343: parse_type_str: Assertion > `siv->lm == LM_NONE' failed.". Steve's patch is more to be a read-only > field from HMP, so probably I can't do that. What do you mean by read-only field? I thought his usage was the same as what we want for dsa-accel-path: (qemu) migrate_set_parameter cpr-exec-command abc def (qemu) info migrate_parameters ... cpr-exec-command: abc def (gdb) p valuestr $3 = 0x55555766a8d0 "abc def" (gdb) p *p->cpr_exec_command $6 = {next = 0x55555823d300, value = 0x55555765f690 "abc"} (gdb) p *p->cpr_exec_command.next $7 = {next = 0x55555805be20, value = 0x555557fefc80 "def"}
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index 7d608d26e1..c422db4ecd 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -312,7 +312,16 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) monitor_printf(mon, "%s: '%s'\n", MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ), params->tls_authz); - + if (params->has_dsa_accel_path) { + strList *dsa_accel_path = params->dsa_accel_path; + monitor_printf(mon, "%s:", + MigrationParameter_str(MIGRATION_PARAMETER_DSA_ACCEL_PATH)); + while (dsa_accel_path) { + monitor_printf(mon, " %s", dsa_accel_path->value); + dsa_accel_path = dsa_accel_path->next; + } + monitor_printf(mon, "\n"); + } if (params->has_block_bitmap_mapping) { const BitmapMigrationNodeAliasList *bmnal; @@ -563,6 +572,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) p->has_x_checkpoint_delay = true; visit_type_uint32(v, param, &p->x_checkpoint_delay, &err); break; + case MIGRATION_PARAMETER_DSA_ACCEL_PATH: + p->has_dsa_accel_path = true; + visit_type_strList(v, param, &p->dsa_accel_path, &err); + break; case MIGRATION_PARAMETER_MULTIFD_CHANNELS: p->has_multifd_channels = true; visit_type_uint8(v, param, &p->multifd_channels, &err); diff --git a/migration/options.c b/migration/options.c index 645f55003d..f839493016 100644 --- a/migration/options.c +++ b/migration/options.c @@ -29,6 +29,7 @@ #include "ram.h" #include "options.h" #include "sysemu/kvm.h" +#include <cpuid.h> /* Maximum migrate downtime set to 2000 seconds */ #define MAX_MIGRATE_DOWNTIME_SECONDS 2000 @@ -162,6 +163,10 @@ Property migration_properties[] = { DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState, parameters.zero_page_detection, ZERO_PAGE_DETECTION_MULTIFD), + /* DEFINE_PROP_ARRAY("dsa-accel-path", MigrationState, x, */ + /* parameters.dsa_accel_path, qdev_prop_string, char *), */ + /* DEFINE_PROP_STRING("dsa-accel-path", MigrationState, */ + /* parameters.dsa_accel_path), */ /* Migration capabilities */ DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), @@ -815,6 +820,13 @@ const char *migrate_tls_creds(void) return s->parameters.tls_creds; } +const strList *migrate_dsa_accel_path(void) +{ + MigrationState *s = migrate_get_current(); + + return s->parameters.dsa_accel_path; +} + const char *migrate_tls_hostname(void) { MigrationState *s = migrate_get_current(); @@ -926,6 +938,7 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) params->zero_page_detection = s->parameters.zero_page_detection; params->has_direct_io = true; params->direct_io = s->parameters.direct_io; + params->dsa_accel_path = QAPI_CLONE(strList, s->parameters.dsa_accel_path); return params; } @@ -934,6 +947,7 @@ void migrate_params_init(MigrationParameters *params) { params->tls_hostname = g_strdup(""); params->tls_creds = g_strdup(""); + params->dsa_accel_path = NULL; /* Set has_* up only for parameter checks */ params->has_throttle_trigger_threshold = true; @@ -1137,6 +1151,22 @@ bool migrate_params_check(MigrationParameters *params, Error **errp) return false; } + if (params->has_zero_page_detection && + params->zero_page_detection == ZERO_PAGE_DETECTION_DSA_ACCEL) { +#ifdef CONFIG_DSA_OPT + unsigned int eax, ebx, ecx, edx; + /* ENQCMD is indicated by bit 29 of ecx in CPUID leaf 7, subleaf 0. */ + if (!__get_cpuid_count(7, 0, &eax, &ebx, &ecx, &edx) || + !(ecx & (1 << 29))) { + error_setg(errp, "DSA acceleration is not supported by CPU"); + return false; + } +#else + error_setg(errp, "DSA acceleration is not enabled"); + return false; +#endif + } + return true; } @@ -1247,6 +1277,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params, if (params->has_direct_io) { dest->direct_io = params->direct_io; } + + if (params->has_dsa_accel_path) { + dest->has_dsa_accel_path = true; + dest->dsa_accel_path = params->dsa_accel_path; + } } static void migrate_params_apply(MigrateSetParameters *params, Error **errp) @@ -1376,6 +1411,12 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) if (params->has_direct_io) { s->parameters.direct_io = params->direct_io; } + if (params->has_dsa_accel_path) { + qapi_free_strList(s->parameters.dsa_accel_path); + s->parameters.has_dsa_accel_path = true; + s->parameters.dsa_accel_path = + QAPI_CLONE(strList, params->dsa_accel_path); + } } void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) @@ -1401,6 +1442,12 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) params->tls_authz->type = QTYPE_QSTRING; params->tls_authz->u.s = strdup(""); } + /* if (params->dsa_accel_path */ + /* && params->dsa_accel_path->type == QTYPE_QNULL) { */ + /* qobject_unref(params->dsa_accel_path->u.n); */ + /* params->dsa_accel_path->type = QTYPE_QLIST; */ + /* params->dsa_accel_path->u.s = strdup(""); */ + /* } */ migrate_params_test_apply(params, &tmp); diff --git a/migration/options.h b/migration/options.h index a2397026db..78b9e4080b 100644 --- a/migration/options.h +++ b/migration/options.h @@ -85,6 +85,7 @@ const char *migrate_tls_creds(void); const char *migrate_tls_hostname(void); uint64_t migrate_xbzrle_cache_size(void); ZeroPageDetection migrate_zero_page_detection(void); +const strList *migrate_dsa_accel_path(void); /* parameters helpers */ diff --git a/qapi/migration.json b/qapi/migration.json index 1234bef888..ff41780347 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -619,10 +619,14 @@ # multifd migration is enabled, else in the main migration thread # as for @legacy. # +# @dsa-accel: Perform zero page checking with the DSA accelerator +# offloading in multifd sender thread if multifd migration is +# enabled, else in the main migration thread as for @legacy. +# # Since: 9.0 ## { 'enum': 'ZeroPageDetection', - 'data': [ 'none', 'legacy', 'multifd' ] } + 'data': [ 'none', 'legacy', 'multifd', 'dsa-accel' ] } ## # @BitmapMigrationBitmapAliasTransform: @@ -825,6 +829,12 @@ # See description in @ZeroPageDetection. Default is 'multifd'. # (since 9.0) # +# @dsa-accel-path: If enabled, use DSA accelerator offloading for +# certain memory operations. Enable DSA accelerator for zero +# page detection offloading by setting the @zero-page-detection +# to dsa-accel. This parameter defines the dsa device path, and +# defaults to an empty list. (since 9.2) +# # @direct-io: Open migration files with O_DIRECT when possible. This # only has effect if the @mapped-ram capability is enabled. # (Since 9.1) @@ -843,7 +853,7 @@ 'cpu-throttle-initial', 'cpu-throttle-increment', 'cpu-throttle-tailslow', 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', - 'avail-switchover-bandwidth', 'downtime-limit', + 'avail-switchover-bandwidth', 'downtime-limit', 'dsa-accel-path', { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] }, 'multifd-channels', 'xbzrle-cache-size', 'max-postcopy-bandwidth', @@ -1000,6 +1010,12 @@ # See description in @ZeroPageDetection. Default is 'multifd'. # (since 9.0) # +# @dsa-accel-path: If enabled, use DSA accelerator offloading for +# certain memory operations. Enable DSA accelerator for zero +# page detection offloading by setting the @zero-page-detection +# to dsa-accel. This parameter defines the dsa device path, and +# defaults to an empty list. (since 9.2) +# # @direct-io: Open migration files with O_DIRECT when possible. This # only has effect if the @mapped-ram capability is enabled. # (Since 9.1) @@ -1044,7 +1060,8 @@ '*vcpu-dirty-limit': 'uint64', '*mode': 'MigMode', '*zero-page-detection': 'ZeroPageDetection', - '*direct-io': 'bool' } } + '*direct-io': 'bool', + '*dsa-accel-path': ['str'] } } ## # @migrate-set-parameters: @@ -1204,6 +1221,12 @@ # See description in @ZeroPageDetection. Default is 'multifd'. # (since 9.0) # +# @dsa-accel-path: If enabled, use DSA accelerator offloading for +# certain memory operations. Enable DSA accelerator for zero +# page detection offloading by setting the @zero-page-detection +# to dsa-accel. This parameter defines the dsa device path, and +# defaults to an empty list. (since 9.2) +# # @direct-io: Open migration files with O_DIRECT when possible. This # only has effect if the @mapped-ram capability is enabled. # (Since 9.1) @@ -1245,7 +1268,8 @@ '*vcpu-dirty-limit': 'uint64', '*mode': 'MigMode', '*zero-page-detection': 'ZeroPageDetection', - '*direct-io': 'bool' } } + '*direct-io': 'bool', + '*dsa-accel-path': ['str'] } } ## # @query-migrate-parameters: