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

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 9 Oct 2019 14:39:46 +0800
Tao Xu <tao3.xu@intel.com> wrote:

> On 10/2/2019 11:16 PM, Igor Mammedov wrote:
> > On Fri, 20 Sep 2019 15:43:44 +0800
> > Tao Xu <tao3.xu@intel.com> wrote:
> >   
> [...]
> >> +struct HMAT_LB_Info {
> >> +    /* Indicates it's memory or the specified level memory side cache. */
> >> +    uint8_t     hierarchy;
> >> +
> >> +    /* Present the type of data, access/read/write latency or bandwidth. */
> >> +    uint8_t     data_type;
> >> +
> >> +    /* Array to store the latencies */  
> > specify units it's stored in
> >   
> >> +    uint64_t    *latency;
> >> +
> >> +    /* Array to store the bandwidthes */  
> > ditto
> >   
> >> +    uint64_t    *bandwidth;  
> > btw:
> > 
> > what was the reason for picking uint64_t for storing above values?
> > 
> > it seems in this patch you dumb down bandwidth to MB/s above but
> > store latency as is.  
> 
> Because I want to store the bandwidth or latency value (minimum unit) 
> that user input. In HMAT, the minimum unit of bandwidth is MB/s, but in 
> QAPI, the minimum unit of size is Byte. So I convert size into MB/s and 
> time unit is "ps", need not convert.
Just be consistent and store (user input) raw values for both fields
(i.e. B/s PS/s) and post-process them later to uint16_t.

> > and then in 9/11 build_hmat_lb you divide that on 'base' units,
> > where are guaranties that value stored here will fit into 2 bytes
> > used in HMAT to store it in the table?
> >   
> For HMAT spec, for a matrix of bandwidth or latency, there is only one 
> base (in order to save ACPI tables space). We need to extract base for a 
> matrix, but user input bandwidth or latency line by line. So after all 
> data input, we can extract the base (as in 9/11).
> 
> There is another benefit. If user input different but similar units, 
> such as "10ns" and "100ps", we can also store them. Only If user input 
> big gap units, such as "1ps" and "1000ms". we can't store them and raise 
> error.
No disagreement here,

but I suggest to move verification and base calculation from 09/11
into a separate patch (right after this one) and doing it at
numa_complete_configuration() time.
To store calculated base you can add a common_base field to
sub-table structure (HMAT_LB_Info) and use it when building ACPI
table without extra calculations.

> > if this structure should store values in terms on HMAT table it should
> > probably use uint16_t and check that user provided value won't overflow
> > at the time of CLI parsing.
> >   
>