Re: [External] Re: [PATCH v5 08/13] migration/multifd: Add new migration option for multifd DSA offloading.

[Fabiano Rosas <farosas@suse.de>]

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"}