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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Mon, May 6, 2024 at 1:54 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 02.05.2024 18:55, 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
> >
> > @@ -513,4 +516,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
> > +
> > +config MAX_LLC_COLORS_ORDER
>
> May I ask that you consider dropping MAX_ from here (but keeping "maximum"
> in prompt and text), thus ...
>
> > --- /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.
> > + */
> > +#include <xen/keyhandler.h>
> > +#include <xen/llc-coloring.h>
> > +#include <xen/param.h>
> > +
> > +#define NR_LLC_COLORS          (1 << CONFIG_MAX_LLC_COLORS_ORDER)
>
> ... making this look less strange?

Ok.

> To match up with e.g. max_nr_colors you may also want to use 1U here.

Ok.

> > +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");
>
> Since you won't accept way_size == 0 here, how about it ending up zero in
> the initial if()'s body? Even more, don't you want to demand
> llc_size % llc_nr_ways == 0 there (thus, together with the enclosing
> condition, guaranteeing way_size != 0)?

Not sure why I would need it. way_size == 0 is checked later via max_nr_colors
as you said. llc_size % llc_nr_ways == 0 doesn't add anything imo. What matters
the most is that max_nr_colors must be a power of 2.

Thanks.


> > +    }
> > +
> > +    /*
> > +     * 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;
> > +
> > +    if ( max_nr_colors & (max_nr_colors - 1) )
> > +        panic("Number of LLC colors (%u) isn't a power of 2\n", 
> > max_nr_colors);
> > +
> > +    if ( max_nr_colors > NR_LLC_COLORS )
> > +    {
> > +        printk(XENLOG_WARNING
> > +               "Number of LLC colors (%u) too big. Using configured max 
> > %u\n",
> > +               max_nr_colors, NR_LLC_COLORS);
> > +        max_nr_colors = NR_LLC_COLORS;
> > +    } else if ( max_nr_colors < 2 )
> > +        panic("Number of LLC colors %u < 2\n", max_nr_colors);
>
> Ah, here's a check guaranteeing at least the first of the two things asked
> about above.
>
> Jan