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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

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?

> Jan