Re: [PATCH v16 08/14] numa: Extend CLI to provide memory latency and bandwidth information

[Tao Xu]

On 11/20/2019 8:56 PM, Igor Mammedov wrote:
> On Wed, 20 Nov 2019 15:55:04 +0800
> Tao Xu <address@hidden> wrote:
>
> > On 11/19/2019 7:03 PM, Igor Mammedov wrote:
> > > On Fri, 15 Nov 2019 15:53:46 +0800
> > > Tao Xu <address@hidden> wrote:
> > >    
> > > > From: Liu Jingqi <address@hidden>
> > > >
> > > > 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>
> > > > Signed-off-by: Tao Xu <address@hidden>
> > >
> > > looks good to me, so
> > >
> > > Reviewed-by: Igor Mammedov <address@hidden>
> > >
> > >
> > > PS:
> > > also see question below
> > >    
> > [...]
> > > > +
> > > > +        hmat_lb->range_bitmap |= node->bandwidth;
> > > > +        first_bit = ctz64(hmat_lb->range_bitmap);
> > > > +        hmat_lb->base = UINT64_C(1) << first_bit;
> > > > +        max_entry = node->bandwidth / hmat_lb->base;
> > > > +        last_bit = 64 - clz64(hmat_lb->range_bitmap);
> > > > +
> > > > +        /*
> > > > +         * For bandwidth, first_bit record the base unit of bandwidth bits,
> > > > +         * last_bit record the last bit of the max bandwidth. The max 
> > > > compressed
> > > > +         * bandwidth should be less than 0xFFFF (UINT16_MAX)
> > > > +         */
> > > > +        if ((last_bit - first_bit) > UINT16_BITS || max_entry >= UINT16_MAX) {
> > >                                                          ^^^^^^^^^^^^^^^^^^^
> > > what bandwidth combination is going to trigger above condition?
> > >    
> > Only use (last_bit - first_bit) > UINT16_BITS, we can't trigger error if
> > the max compressed bandwidth is 0xFFFF. Because in that condition,
> > "last_bit - first_bit == UINT16_BITS". So I add "max_entry >=
> > UINT16_MAX" to catch 0xFFFF. For example:
> >
> > Combination 1 (Error):
> > bandwidth1   = ...0000 1111 1111 1111 1110 0000... (max_entry 32767)
> > range_bitmap = ...0000 1111 1111 1111 1110 0000... (range is 15 bits)
> > bandwidth2   = ...0000 1111 1111 1111 1111 0000... (max_entry 65535)
> > range_bitmap = ...0000 1111 1111 1111 1111 0000... (range is 16 bits)
> >
> > Combination 2 (Error):
> > bandwidth1   = ...0000 1111 1111 1111 1110 0000... (max_entry 32767)
> > range_bitmap = ...0000 1111 1111 1111 1110 0000... (range is 15 bits)
> > bandwidth2   = ...0001 1111 1111 1111 1110 0000... (max_entry 65535)
> > range_bitmap = ...0001 1111 1111 1111 1110 0000... (range is 16 bits)
> >
> > Combination 3 (OK, because bandwidth1 will be compressed to 65534):
> > bandwidth1   = ...0000 1111 1111 1111 1110 0000... (max_entry 32767)
> > range_bitmap = ...0000 1111 1111 1111 1110 0000... (range is 15 bits)
> > bandwidth2   = ...0000 0111 1111 1111 1111 0000... (max_entry 32767)
> > range_bitmap = ...0000 1111 1111 1111 1111 0000... (range is 16 bits)
> >
> > Combination 4 (Error):
> > bandwidth1   = ...0000 1111 1111 1111 1111 0000... (max_entry 65535)
> > range_bitmap = ...0000 1111 1111 1111 1111 0000... (range is 16 bits)
>
> ok, I'd use in max/min possible values in bios-tables-test,
> to make sure that we are testing whole range and would be able
> to detect a error in case the valid ranges regressed (shrink)
> and x-fail tests I've asked for in QMP test should detect
> error other way around.
OK I will add these tests.