Re: [PATCH v17 01/14] util/cutils: Add Add qemu_strtold and qemu_strtold_finite

[Tao Xu <tao3.xu@intel.com>]

On 11/27/2019 2:44 PM, Markus Armbruster wrote:
> Tao Xu <tao3.xu@intel.com> writes:
> 
>> On 11/26/2019 9:54 PM, Markus Armbruster wrote:
>>> Tao Xu <tao3.xu@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()
> 
> We'd still have to grapple with "What about all the other time-valued
> things in the QAPI schema?"
> 
>> * 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.
> 
> Considerung we're already at v17, I recommend to keep the first part as
> focused as possible.  Since plain integer types are good enough for time
> values elsewhere in our external interfaces, they'll do for HMAT
> latency, too.
> 
> Once the first part is accepted, you're free to tackle the wider problem
> of providing convenience syntax for time values.  Fair warning: I
> consider this a swamp.
> 

Thank you! I will use integer for HMAT latency and submit the first part 
in next version.