Re: [PATCH v12 01/12] xen/common: add cache coloring common code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.12.2024 11:25, Carlo Nonato wrote:
> On Tue, Dec 17, 2024 at 9:57 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 16.12.2024 17:33, Carlo Nonato wrote:
>>> On Mon, Dec 16, 2024 at 11:51 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 13.12.2024 17:28, Carlo Nonato wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/common/llc-coloring.c
>>>>> @@ -0,0 +1,124 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>>> +/*
>>>>> + * Last Level Cache (LLC) coloring common code
>>>>> + *
>>>>> + * Copyright (C) 2024, Advanced Micro Devices, Inc.
>>>>> + * Copyright (C) 2024, Minerva Systems SRL
>>>>> + */
>>>>> +#include <xen/keyhandler.h>
>>>>> +#include <xen/llc-coloring.h>
>>>>> +#include <xen/param.h>
>>>>> +
>>>>> +#define NR_LLC_COLORS          (1U << CONFIG_LLC_COLORS_ORDER)
>>>>> +
>>>>> +/*
>>>>> + * -1: not specified (disabled unless llc-size and llc-nr-ways present)
>>>>> + *  0: explicitly disabled through cmdline
>>>>> + *  1: explicitly enabled through cmdline
>>>>> + */
>>>>> +static int8_t __initdata opt_llc_coloring = -1;
>>>>> +boolean_param("llc-coloring", opt_llc_coloring);
>>>>> +
>>>>> +static bool __ro_after_init llc_coloring_enabled;
>>>>> +
>>>>> +static unsigned int __initdata llc_size;
>>>>> +size_param("llc-size", llc_size);
>>>>> +static unsigned int __initdata llc_nr_ways;
>>>>> +integer_param("llc-nr-ways", llc_nr_ways);
>>>>> +/* Number of colors available in the LLC */
>>>>> +static unsigned int __ro_after_init max_nr_colors;
>>>>> +
>>>>> +static void print_colors(const unsigned int colors[], unsigned int 
>>>>> num_colors)
>>>>> +{
>>>>> +    unsigned int i;
>>>>> +
>>>>> +    printk("{ ");
>>>>> +    for ( i = 0; i < num_colors; i++ )
>>>>> +    {
>>>>> +        unsigned int start = colors[i], end = start;
>>>>> +
>>>>> +        printk("%u", start);
>>>>> +
>>>>> +        for ( ; i < num_colors - 1 && end + 1 == colors[i + 1]; i++, 
>>>>> end++ )
>>>>> +            ;
>>>>> +
>>>>> +        if ( start != end )
>>>>> +            printk("-%u", end);
>>>>> +
>>>>> +        if ( i < num_colors - 1 )
>>>>> +            printk(", ");
>>>>> +    }
>>>>> +    printk(" }\n");
>>>>> +}
>>>>> +
>>>>> +void __init llc_coloring_init(void)
>>>>> +{
>>>>> +    unsigned int way_size;
>>>>> +
>>>>> +    llc_coloring_enabled = (opt_llc_coloring == 1);
>>>>
>>>> Generally I'd suggest to only use > 0, >= 0, < 0, and <= 0 on such
>>>> variables.
>>>>
>>>>> +    if ( (opt_llc_coloring != 0) && llc_size && llc_nr_ways )
>>>>> +    {
>>>>> +        llc_coloring_enabled = true;
>>>>> +        way_size = llc_size / llc_nr_ways;
>>>>> +    }
>>>>
>>>> Hmm, I actually see a difference in someone saying
>>>>
>>>> "llc-coloring=0 llc-size=... llc-nr-ways=..."
>>>>
>>>> vs
>>>>
>>>> "llc-size=... llc-nr-ways=... llc-coloring=0"
>>>>
>>>> I'm not sure about Arm, but on x86 this can be relevant as there may be
>>>> pre-set parts of a command line with appended (human) overrides. Therefore
>>>> it always wants to be "last wins". Yet yes, you may weant to take the
>>>> position that in such a case the former example would require 
>>>> "llc-coloring=1"
>>>> to also be added.
>>>
>>> Yes, I think this should be the way to go.
>>>
>>>> Kind of against the shorthand llc-size+llc-nr-ways only,
>>>> though.
>>>
>>> The shorthand was proposed by you here:
>>> https://patchew.org/Xen/20240315105902.160047-1-carlo.nonato@xxxxxxxxxxxxxxx/20240315105902.160047-2-carlo.nonato@xxxxxxxxxxxxxxx/#05e4d3da-4130-4c57-9855-43b685ce5005@xxxxxxxx
>>>
>>>> Wouldn't it make sense to infer "llc-coloring" when both of the latter 
>>>> options
>>>> were supplied?
>>>
>>> We both agreed that it was something good to have.
>>
>> Right, and I'm not putting that under question. With that, however, I find
>> your reply ambiguous. If the shorthand is useful to have, is the requirement
>> to put a 2nd "llc-coloring=1" on a command line (as per above) really a good
>> idea?
> 
> I don't know an easy way to check for parameters order. We're close to feature
> freeze. Isn't this solution good enough for now?

Maybe it is, but then imo only when also calling out the apparent anomaly in
the command line doc. I.e. amend 'Note that using both options implies
"llc-coloring=on"' by e.g. 'unless an earlier "llc-coloring=off" is there' (in
both places).

Jan