Re: [PATCH v13 06/12] numa: Extend CLI to provide memory latency and bandwidth information

[Markus Armbruster <armbru@redhat.com>]

Igor Mammedov <imammedo@redhat.com> writes:

> On Fri, 25 Oct 2019 14:33:53 +0800
> Tao Xu <tao3.xu@intel.com> wrote:
>
>> On 10/23/2019 11:28 PM, Igor Mammedov wrote:
>> > On Sun, 20 Oct 2019 19:11:19 +0800
>> > Tao Xu <tao3.xu@intel.com> wrote:  
>> [...]
>> >> +#
>> >> +# @access-bandwidth: access bandwidth (MB/s)
>> >> +#
>> >> +# @read-bandwidth: read bandwidth (MB/s)
>> >> +#
>> >> +# @write-bandwidth: write bandwidth (MB/s)  
>> > I think units here are not appropriate, values stored in fields are
>> > minimal base units only and nothing else (i.e. ps and B/s)
>> >   
>> Eric suggest me to drop picoseconds. So here I can use ns. For 
>> bandwidth, if we use B/s here, does it let user or developer to 
>> misunderstand that the smallest unit is B/s ?
>
> It's not nanoseconds or MB/s stored in theses fields, isn't it?
> I'd specify units in which value is stored or drop units altogether.
>
> Maybe Eric and Markus can suggest a better way to describe fields.

This isn't review (yet), just an attempt to advise more quickly on
general QAPI/QMP conventions.

Unit prefixes like Mebi- are nice for humans, because 1MiB is clearer
than 1048576B.

QMP is for machines.  We eschew unit prefixes and unit symbols there.
The unit is implied.  Unit prefixes only complicate things.  Machines
can deal with 1048576 easily.  Also dealing 1024Ki and 1Mi is additional
work.  We therefore use JSON numbers for byte counts, not strings with
units.

The general rule is "always use the plainest implied unit that would
do."  There are exceptions, mostly due to review failure.

Byte rates should be in bytes per second.

For time, we've made a godawful mess.  The plainest unit is clearly the
second.  We commonly need sub-second granularity, though.
Floating-point seconds are unpopular for some reason :)  Instead we use
milli-, micro-, and nanoseconds, and even (seconds, microseconds) pairs.

QAPI schema documentation describes both the generated C and the QMP
wire protocol.  It must be written with the implied unit.  If you send a
byte rate in bytes per second via QMP, that's what you document.  Even
if a human interface lets you specify the byte rate in MiB/s.

Does this make sense?

>> >>   @item -numa node[,mem=@var{size}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@var{node}][,initiator=@var{initiator}]
>> >>   @itemx -numa node[,memdev=@var{id}][,cpus=@var{firstcpu}[-@var{lastcpu}]][,nodeid=@var{node}][,initiator=@var{initiator}]
>> >>   @itemx -numa dist,src=@var{source},dst=@var{destination},val=@var{distance}
>> >>   @itemx -numa cpu,node-id=@var{node}[,socket-id=@var{x}][,core-id=@var{y}][,thread-id=@var{z}]
>> >> +@itemx -numa hmat-lb,initiator=@var{node},target=@var{node},hierarchy=@var{str},data-type=@var{str}[,latency=@var{lat}][,bandwidth=@var{bw}]  
>> >                                                                                ^^^                 ^^^
>> > Using the same 'str' for 2 different enums is confusing.
>> > Suggest for 1st use 'level' and for the second just 'type'
>> >   
>> Ok
>> 
>> >>   @findex -numa
>> >>   Define a NUMA node and assign RAM and VCPUs to it.
>> >>   Set the NUMA distance from a source node to a destination node.
>> >> +Set the ACPI Heterogeneous Memory Attributes for the given nodes.
>> >>   
>> >>   Legacy VCPU assignment uses @samp{cpus} option where
>> >>   @var{firstcpu} and @var{lastcpu} are CPU indexes. Each
>> >> @@ -256,6 +259,50 @@ specified resources, it just assigns existing resources to NUMA
>> >>   nodes. This means that one still has to use the @option{-m},
>> >>   @option{-smp} options to allocate RAM and VCPUs respectively.
>> >>   
>> >> +Use @samp{hmat-lb} to set System Locality Latency and Bandwidth Information
>> >> +between initiator and target NUMA nodes in ACPI Heterogeneous Attribute Memory Table (HMAT).
>> >> +Initiator NUMA node can create memory requests, usually including one or more processors.  
>> > s/including/it has/
>> >   
>> >> +Target NUMA node contains addressable memory.
>> >> +
>> >> +In @samp{hmat-lb} option, @var{node} are NUMA node IDs. @var{str} of 'hierarchy'
>> >> +is the memory hierarchy of the target NUMA node: if @var{str} is 'memory', the structure
>> >> +represents the memory performance; if @var{str} is 'first-level|second-level|third-level',
>> >> +this structure represents aggregated performance of memory side caches for each domain.
>> >> +@var{str} of 'data-type' is type of data represented by this structure instance:
>> >> +if 'hierarchy' is 'memory', 'data-type' is 'access|read|write' latency(nanoseconds)  
>> > is nanoseconds is right here? Looking at previous patches default value of suffix-less
>> > should be picoseconds. I'd just drop '(nanoseconds)'. User will use appropriate suffix.
>> >   
>> OK, I will drop it.
>> >> +or 'access|read|write' bandwidth(MB/s) of the target memory; if 'hierarchy' is  
>> > ditto (MB/s), probably should be Bytes/s for default suffix-less value
>> > (well, I'm not sure how to express it better)
>> >   
>> 
>> But last version, we let !QEMU_IS_ALIGNED(node->bandwidth, MiB) as error.
> it's alignment requirement and it doesn't mean that value could not be
> represented in bytes/s.
> And it is bytes/s if suffix isn't used.
>
> As for alignment and precision of values you probably need to document
> expectations here as well.
>
>> >> +'first-level|second-level|third-level', 'data-type' is 'access|read|write' hit latency
>> >> +or 'access|read|write' hit bandwidth of the target memory side cache.
>> >> +
>> >> +@var{lat} of 'latency' is latency value, the possible value and units are
>> >> +NUM[ps|ns|us] (picosecond|nanosecond|microsecond), the recommended unit is 'ns'. @var{bw}
>> >> +is bandwidth value, the possible value and units are NUM[M|G|T], mean that  
>> >   
>> >> +the bandwidth value are NUM MB/s, GB/s or TB/s. Note that max NUM is 65534,
>> >> +if NUM is 0, means the corresponding latency or bandwidth information is not provided.
>> >> +And if input numbers without any unit, the latency unit will be 'ps' and the bandwidth
>> >> +will be MB/s.  
>> >   1st: above is applicable to both bw and lat values and should be documented as such
>> >   2nd: 'max NUM is 65534' when different suffixes is fleeting target,
>> >        spec says that entry with 0xFFFF is unreachable, so how about documenting
>> >        unreachable value as 0xFFFFFFFFFFFFFFFF (then CLI parsing code will
>> >        exclude it from range detection and acpi table building code translate it
>> >        to internal 0xFFFF it could fit into the tables)
>> >   
>> 
>> If we input 0xFFFFFFFFFFFFFFFF, qemu will raise error that parameter 
>> expect a size value.
>
> opts_type_size() can't handle values >= 0xfffffffffffffc00
>
> commit f46bfdbfc8f (util/cutils: Change qemu_strtosz*() from int64_t to uint64_t)
>
> so behavior would change depending on if the value is parsed by CLI (size) or QMP (unit64) parsers.

Do these values matter?  Is there a use case for passing
18446744073709550593 via CLI?

> we can cannibalize 0x0 as the unreachable value and an absent bandwidth/lat option
> for not specified case.
> It would be conflicting with matrix [1] values in spec, but CLI/QMP deals with
> absolute values which are later processed into HMAT substructure.
>
> Markus,
> Can we make opts_type_size() handle full range of uint64_t?

Maybe.

> 1)
> ACPI 6.3 spec:
> 5.2.27.4 System Locality Latency and Bandwidth Information Structure