Re: [Qemu-devel] [PATCH v1 5/7] numa: Extend the command-line to provide memory latency and bandwidth information

[Liu, Jingqi]

> -----Original Message-----
> From: Eric Blake [mailto:address@hidden
> Sent: Wednesday, May 9, 2018 9:16 PM
> To: Liu, Jingqi <address@hidden>; address@hidden; address@hidden;
> address@hidden
> Cc: address@hidden; address@hidden; address@hidden
> Subject: Re: [Qemu-devel] [PATCH v1 5/7] numa: Extend the command-line to
> provide memory latency and bandwidth information
> 
> On 05/09/2018 03:36 AM, Liu Jingqi wrote:
> > Add -numa hmat-lb option to provide System Locality Latency and
> > Bandwidth Information. These memory attributes help to build System
> > Locality Latency and Bandwidth Information Structure(s) in ACPI
> > Heterogeneous Memory Attribute Table (HMAT).
> >
> > Signed-off-by: Liu Jingqi <address@hidden>
> > ---
> 
> > +++ b/qapi/misc.json
> > @@ -2705,7 +2705,7 @@
> >   # Since: 2.1
> >   ##
> >   { 'enum': 'NumaOptionsType',
> > -  'data': [ 'node', 'dist', 'cpu' ] }
> > +  'data': [ 'node', 'dist', 'cpu', 'hmat-lb' ] }
> 
> Missing documentation for the new addition, particularly that it is '(since 
> 2.13)'.
> 
> 
> >   ##
> > +# @NumaHmatLBOptions:
> > +#
> > +# Set the system locality latency and bandwidth information between
> Initiator and Target proximity Domains.
> 
> Long line here and elsewhere. Please keep this file wrapped below 80
> columns.
> 
> > +#
> > +# @initiator: the Initiator Proximity Domain.
> > +#
> > +# @target: the Target Proximity Domain.
> > +#
> > +# @hierarchy: the Memory Hierarchy. Indicates the performance of memory
> or side cache.
> > +#
> > +# @level: indicates the level of side cache, if it's the performance of 
> > side
> cache.
> > +#
> > +# @data-type: presents the type of data, access/read/write latency or hit
> latency.
> > +#
> > +# @base-lat: the base unit for latency in nanosecondsbytes.
> 
> Is that supposed to be nanoseconds/bytes?
> 
> > +#
> > +# @base-bw: the base unit for bandwidth in megabytes per second(MB/s).
> > +#
> > +# @latency: the value of latency based on Base Unit from @initiator to
> @target proximity domain.
> > +#
> > +# @bandwidth: the value of bandwidth based on Base Unit between
> @initiator to @target proximity domain.
> > +#
> > +# Since: 2.10
> 
> Not even close.  The soonest this could make it in is 2.13.
> 
> > +##
> > +{ 'struct': 'NumaHmatLBOptions',
> > +  'data': {
> > +   'initiator': 'uint16',
> > +   'target': 'uint16',
> > +   'hierarchy': 'str',
> 
> Is this really a free-form string, or is it better as an enum type?
> 
> > +   '*level': 'uint8',
> > +   'data-type': 'str',
> 
> Ditto.
> 
> > +   '*base-lat': 'uint64',
> > +   '*base-bw': 'uint64',
> > +   '*latency': 'uint16',
> > +   '*bandwidth': 'uint16' }}
> > +
> 
> 
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

Hi Eric,
Thanks for your reviewing.
I will correct them.

Jingqi Liu