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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

Jan