Re: [PATCH v2 1/2] qdev: add IOThreadVirtQueueMappingList property type

[Markus Armbruster <armbru@redhat.com>]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Mon, Dec 11, 2023 at 04:32:06PM +0100, Markus Armbruster wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 18.09.2023 um 18:16 hat Stefan Hajnoczi geschrieben:
>> >> virtio-blk and virtio-scsi devices will need a way to specify the
>> >> mapping between IOThreads and virtqueues. At the moment all virtqueues
>> >> are assigned to a single IOThread or the main loop. This single thread
>> >> can be a CPU bottleneck, so it is necessary to allow finer-grained
>> >> assignment to spread the load.
>> >> 
>> >> Introduce DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST() so devices can take a
>> >> parameter that maps virtqueues to IOThreads. The command-line syntax for
>> >> this new property is as follows:
>> >> 
>> >>   --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]}'
>> >> 
>> >> IOThreads are specified by name and virtqueues are specified by 0-based
>> >> index.
>> >> 
>> >> It will be common to simply assign virtqueues round-robin across a set
>> >> of IOThreads. A convenient syntax that does not require specifying
>> >> individual virtqueue indices is available:
>> >> 
>> >>   --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]}'
>> >> 
>> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> >
>> > When testing this, Qing Wang noticed that "info qtree" crashes. This is
>> > because the string output visitor doesn't support structs. I suppose
>> > IOThreadVirtQueueMapping is the first struct type that is used in a qdev
>> > property type.
>> >
>> > So we'll probably have to add some kind of struct support to the string
>> > output visitor before we can apply this. Even if it's as stupid as just
>> > printing "<struct IOThreadVirtQueueMapping>" without actually displaying
>> > the value.
>> 
>> The string visitors have been nothing but trouble.
>> 
>> For input, we can now use keyval_parse() and the QObject input visitor
>> instead.  Comes with restrictions, but I'd argue it's a more solid base
>> than the string input visitor.
>> 
>> Perhaps we can do something similar for output: create a suitable
>> formatter for use it with the QObject output visitor, replacing the
>> string output visitor.
>
> I sent an initial patch that just shows "<omitted>" but would like to
> work on a proper solution with your input.
>
> From what I've seen StringOutputVisitor is used in several places in
> QEMU. "info qtree" calls it through object_property_print() to print
> individual qdev properties. I don't understand the requirements of the
> other callers, but object_property_print() wants to return a single
> string without newlines.

string_output_visitor_new():

* hmp_info_migrate(): format a list of integers, then print it like

        monitor_printf(mon, "postcopy vcpu blocktime: %s\n", str);

  One element per vCPU; can produce a long line.

* netfilter_print_info(): format the property values of a
  NetFilterState, then print each like

        monitor_printf(mon, ",%s=%s", prop->name, str);

* object_property_print(): format a property value of an object, return
  the string.

  Function is misnamed.  object_property_format() or
  object_property_to_string() would be better.

  Just one caller: qdev_print_props(), helper for hmp_info_qtree().
  Prints the string like

        qdev_printf("%s = %s\n", props->name,
                    *value ? value : "<null>");

  where qdev_printf() is a macro wrapping monitor_printf().

  This one passes human=true, unlike the others.  More on that below.

* hmp_info_memdev(): format a list of integers, then print it like

        monitor_printf(mon, "  policy: %s\n",
                       HostMemPolicy_str(m->value->policy));

  One element per "host node", whatever that may be; might produce a
  long line.

* Tests; not relevant here.

hmp_info_migrate() and hmp_info_memdev() use the visitor as a (somewhat
cumbersome) helper for printing uint32List and uint16List, respectively.
Could do without.

The other two display all properties in HMP.  Both kind of assume the
string visitor produces no newlines.  I think we could instead use the
QObject output visitor, then format the QObject in human-readable form.
Might be less efficient, because we create a temporary QObject.  Perhaps
factor out a single helper first.

string_input_visitor_new(), for good measure:

* hmp_migrate_set_parameter(): parse an uint8_t, uint32, size_t, bool,
  str, or QAPI enum from a string.

* object_property_parse(): parse a property value from a string, and
  assign it to the property.

  Calling this object_property_set_from_string() would be better.

  Callers:

  - object_apply_global_props(): applying compatibility properties
    (defined in C) and defauls set with -global (given by user).

  - object_set_propv(): helper for convenience functions to set multiple
    properties in C.

  - hmp_qom_set(): set the property value when not JSON (-j is off).

  - object_parse_property_opt(), for accelerator_set_property(), which
    processes the argument of -accel.

  - x86_cpu_apply_props() and x86_cpu_apply_version_props(): apply
    properties defined in C.

  The property settings defined in C could just as well use QObject
  input visitor instead.  Might be less efficient, because we create a
  temporary QObject.

  The property settings that come from the user are ABI.

  Reminder: supports only scalars and lists of small integers.
  Supporting structured values containing strings would involve creating
  syntax that's basically reinventing JSON.

  I think qom-set demonstrates how to support structured values: just
  use JSON.

> QObjectOutputVisitor produces a QObject. That could be formatted as
> JSON using qobject_to_json_pretty()?
>
> The pretty JSON would contain newlines and existing callers such as
> "info qtree" may need to scan the output and insert indentation.

With pretty=false, the JSON formatter produces a string without
newlines.  With pretty=true, it adds newlines and indentation.

Instead of scanning the formatted string to change its indentation, we
could perhaps pass indentation instructions to the JSON formatter.

> The goal would be to keep the output unchanged as far as possible and to
> emit JSON for structs and lists.

That's basically JSON, except we print 'str' values without quotes and
escapes (resulting in ambiguity when they contain {[]}, and even more
fun when they contain control characters), plus a few more largely
gratuitous differences.

With human=true (advertized as "a bit easier for humans to read"), we
get strings with quotes (still no escapes), and a tweaked set of
gratuitous differences.

> What do you think about this approach?

Format as JSON and call it a day?

I figure the only thing of value we'd lose is displaying integers both
decimal and hex.  In some, but not all places.