Re: [PATCH v9 01/13] xen/common: add cache coloring common code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.11.2024 17:09, Carlo Nonato wrote:
> On Tue, Nov 5, 2024 at 4:46 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 25.10.2024 11:50, Carlo Nonato wrote:
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -71,6 +71,9 @@ config HAS_IOPORTS
>>>  config HAS_KEXEC
>>>       bool
>>>
>>> +config HAS_LLC_COLORING
>>> +     bool
>>> +
>>>  config HAS_PIRQ
>>>       bool
>>>
>>> @@ -516,4 +519,23 @@ config TRACEBUFFER
>>>         to be collected at run time for debugging or performance analysis.
>>>         Memory and execution overhead when not active is minimal.
>>>
>>> +config LLC_COLORING
>>> +     bool "Last Level Cache (LLC) coloring" if EXPERT
>>> +     depends on HAS_LLC_COLORING
>>> +     depends on !NUMA
>>
>> Instead of this dependency, wouldn't it be more natural to suppress the
>> setting of HAS_LLC_COLORING by an arch when NUMA is on?
> 
> So moving the "depends on" in the HAS_LLC_COLORING definition? Yes I believe
> it would be better.

No. Putting it on an option without prompt will, iirc, only cause a warning
when violated, but will otherwise have no real effect. The "select" of
HAS_LLC_COLORING wants to become dependent upon !NUMA, until that combination
was made work.

>>> --- /dev/null
>>> +++ b/xen/common/llc-coloring.c
>>> @@ -0,0 +1,111 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Last Level Cache (LLC) coloring common code
>>> + *
>>> + * Copyright (C) 2022 Xilinx Inc.
>>
>> Does this need updating (if it can't be dropped)?
> 
> I don't remember what's the current policy for these copyright lines.
> Do you still use them? If they are used, should they reflect the history
> of the revisions of the patch series? I mean, in v1 it was "2019 Xilinx Inc."
> 2023-2024 would then be MinervaSys.

I don't know what the policy is either. I think it can be there or it can
be omitted. Yet if it's there, I think it wants to be accurate at least at
the time a new file is being added. (These lines usually aren't updated
when later changes are made to the files.)

>>> +void __init llc_coloring_init(void)
>>> +{
>>> +    unsigned int way_size;
>>> +
>>> +    if ( llc_size && llc_nr_ways )
>>> +    {
>>> +        llc_coloring_enabled = true;
>>> +        way_size = llc_size / llc_nr_ways;
>>> +    }
>>> +    else if ( !llc_coloring_enabled )
>>> +        return;
>>> +    else
>>> +    {
>>> +        way_size = get_llc_way_size();
>>> +        if ( !way_size )
>>> +            panic("LLC probing failed and 'llc-size' or 'llc-nr-ways' 
>>> missing\n");
>>> +    }
>>> +
>>> +    /*
>>> +     * The maximum number of colors must be a power of 2 in order to 
>>> correctly
>>> +     * map them to bits of an address.
>>> +     */
>>> +    max_nr_colors = way_size >> PAGE_SHIFT;
>>
>> This discards low bits of the quotient calculated above, bearing a certain
>> risk that ...
>>
>>> +    if ( max_nr_colors & (max_nr_colors - 1) )
>>> +        panic("Number of LLC colors (%u) isn't a power of 2\n", 
>>> max_nr_colors);
>>
>> ... this panic() wrongly doesn't trigger.
> 
> Yes, but I don't care if way_size isn't a power of 2.

Well, you may not care, but imo the resulting configuration ought to reflect
what was requested on the command line (maybe unless e.g. documentation
explicitly says otherwise). If way_size has low bits set, that wouldn't be
the case.

Jan