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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

On Tue, Dec 17, 2024 at 12:12 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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).
>

Ok.

> Jan