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

[Igor Mammedov <imammedo@redhat.com>]

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.

> >>   @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.

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?

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