On 11/26/2019 9:54 PM, Markus Armbruster wrote:
Tao Xu <tao3...@intel.com> writes:
Hi Markus,
Do you have any comments on this patch and 02/14 05/14 06/14.
Thank you!
These provide a new QAPI built-in type 'time'. It's like 'uint64' with
an implied nanoseconds unit, and additional convenience syntax in the
opts visitor and the keyval qobject input visitor. Patterned after
'size'.
The only use of 'time' so far is member @latency of NumaOptions member
@hmap-lb. Uses of that:
* QMP command set-numa-node
The convenience syntax does not apply, as QMP uses the regular qobject
input visitor, not the keyval one.
* CLI option -numa
We first parse the option argument with QemuOpts, then convert it to
NumaOptions with the opts visitor.
The new built-in type 'time' gets used in -numa hmat-lb,...,latency=T
Questions / observations:
* The keyval qobject input visitor's support for 'time' appears to be
unused for now.
* What's the anticipated range of values for -numa
hmat-lb,...,latency=T? I'm asking because I wonder whether we really
need convenience syntax there.
* Sure you want fractions?
Supporting fractions for byte counts (e.g. 1.5G) has been a mixed
blessing, to put it charitably.
Use of fractions that aren't representable as double is not advisable.
For instance, 1.1G is 1181116006 bytes rounded from
1181116006.4000001. Why would anybody want that?
Use of "nice" fractions is unproblematic, but the additional
convenience is rather minor. Is being able to write 1536M as 1.5G
worth the trouble? Meh.
With "metric" rather than "binary" suffixes, fractions provide even
less convenience: 1.5ms vs. 1500us.
The implementation is limited to 53 bits of precision, which has been
a source of confusion. Even that has arguably taken far more patches
than it's worth. We're now talking about more patches to lift the
restriction. Meh.
What exactly are we trying to achieve by supporting fractions?
* What about all the other time-valued things in the QAPI schema?
There are many more, and some of them are also visible in CLI or HMP.
By providing convenience syntax for just -numa hmat-lb,...,latency=T,
we create inconsistency.
To avoid it, we'd have to hunt down all the others. But some of them
aren't in nanoseconds. Your new built-in type 'time' is only
applicable to the ones in nanoseconds. Do we need more built-in
types?
This series is at v17. I really, really want to tell you it's ready for
merging. But as you see, I can't.
Maybe the convenience syntax is a good idea, maybe it's a bad idea. But
it's definitely not a must-have idea.
If you want to pursue the idea, I recommend to split this series in two:
one part without the convenience, and a second part adding it.
Hopefully, we can then merge the first part without too much fuss. The
second part will have to deal with the questions above.
You can also shelve the idea, i.e. do just the first part now. It's
what I'd do.
Thank you for your suggestion and support! Considering ACPI HMAT can
only store unsigned integer data, and for the memory latency nanoseconds
is enough. So we can use integer for latency data. I am wondering if we
can use this solution:
* Still add builtin type time, but use qemu_strtou64() to parse.
* Still refactor do_strtosz() to support suffixes list, but add a extra
parameter to decide use qemu_strtou64() or qemu_strtod_finite(), so time
use qemu_strtou64() and qemu_strtod_finite()
* Second part dealing with the questions.
Then the only influence on HMAT patch is we need add a comments to tell
user to input integer.
