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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

On Tue, Nov 5, 2024 at 4:46 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 25.10.2024 11:50, Carlo Nonato wrote:
> > Last Level Cache (LLC) coloring allows to partition the cache in smaller
> > chunks called cache colors.
> >
> > Since not all architectures can actually implement it, add a 
> > HAS_LLC_COLORING
> > Kconfig option.
> > MAX_LLC_COLORS_ORDER Kconfig option has a range maximum of 10 (2^10 = 1024)
>
> Is the MAX_ here stale ...
>
> > because that's the number of colors that fit in a 4 KiB page when integers
> > are 4 bytes long.
> >
> > LLC colors are a property of the domain, so struct domain has to be 
> > extended.
> >
> > Based on original work from: Luca Miccio <lucmiccio@xxxxxxxxx>
> >
> > Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> > Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> > ---
> > v9:
> > - dropped _MAX_ from CONFIG_MAX_LLC_COLORS_ORDER
>
> ... with this change?

Yes.

> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -1708,6 +1708,43 @@ This option is intended for debugging purposes only. 
> >  Enable MSR_DEBUGCTL.LBR
> >  in hypervisor context to be able to dump the Last Interrupt/Exception 
> > To/From
> >  record with other registers.
> >
> > +### llc-coloring (arm64)
> > +> `= <boolean>`
> > +
> > +> Default: `false`
> > +
> > +Flag to enable or disable LLC coloring support at runtime. This option is
> > +available only when `CONFIG_LLC_COLORING` is enabled. See the general
> > +cache coloring documentation for more info.
> > +
> > +### llc-nr-ways (arm64)
> > +> `= <integer>`
> > +
> > +> Default: `Obtained from hardware`
> > +
> > +Specify the number of ways of the Last Level Cache. This option is 
> > available
> > +only when `CONFIG_LLC_COLORING` is enabled. LLC size and number of ways 
> > are used
> > +to find the number of supported cache colors. By default the value is
> > +automatically computed by probing the hardware, but in case of specific 
> > needs,
> > +it can be manually set. Those include failing probing and debugging/testing
> > +purposes so that it's possible to emulate platforms with different number 
> > of
> > +supported colors. If set, also "llc-size" must be set, otherwise the 
> > default
> > +will be used. Note that using these two options implies "llc-coloring=on".
>
> Nit: Both here and ...
>
> > +### llc-size (arm64)
> > +> `= <size>`
> > +
> > +> Default: `Obtained from hardware`
> > +
> > +Specify the size of the Last Level Cache. This option is available only 
> > when
> > +`CONFIG_LLC_COLORING` is enabled. LLC size and number of ways are used to 
> > find
> > +the number of supported cache colors. By default the value is automatically
> > +computed by probing the hardware, but in case of specific needs, it can be
> > +manually set. Those include failing probing and debugging/testing purposes 
> > so
> > +that it's possible to emulate platforms with different number of supported
> > +colors. If set, also "llc-nr-ways" must be set, otherwise the default will 
> > be
> > +used. Note that using these two options implies "llc-coloring=on".
>
> ... here, maybe better s/these two/both/?

Ok.

> > --- 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
> >
> > @@ -516,4 +519,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
>
> Instead of this dependency, wouldn't it be more natural to suppress the
> setting of HAS_LLC_COLORING by an arch when NUMA is on?

So moving the "depends on" in the HAS_LLC_COLORING definition? Yes I believe
it would be better.

> > --- /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.
>
> Does this need updating (if it can't be dropped)?

I don't remember what's the current policy for these copyright lines.
Do you still use them? If they are used, should they reflect the history
of the revisions of the patch series? I mean, in v1 it was "2019 Xilinx Inc."
2023-2024 would then be MinervaSys.

> > + */
> > +#include <xen/keyhandler.h>
> > +#include <xen/llc-coloring.h>
> > +#include <xen/param.h>
> > +
> > +#define NR_LLC_COLORS          (1U << CONFIG_LLC_COLORS_ORDER)
> > +
> > +static bool __ro_after_init llc_coloring_enabled;
> > +boolean_param("llc-coloring", 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)
>
> Just to mention it here as well (I mentioned it elsewhere in the past):
> Personally I think that when function parameters denote array, array
> notation would also better be used. I.e. "const unsigned int colors[]"
> here. That'll then probably also bring us closer to using the upcoming
> (in gcc) counted_by attribute.

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");
> > +    }
> > +
> > +    /*
> > +     * 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;
>
> This discards low bits of the quotient calculated above, bearing a certain
> risk that ...
>
> > +    if ( max_nr_colors & (max_nr_colors - 1) )
> > +        panic("Number of LLC colors (%u) isn't a power of 2\n", 
> > max_nr_colors);
>
> ... this panic() wrongly doesn't trigger.

Yes, but I don't care if way_size isn't a power of 2.

> Jan

- Carlo