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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Thu, Nov 7, 2024 at 10:05 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 06.11.2024 17:09, Carlo Nonato wrote:
> > On Tue, Nov 5, 2024 at 4:46 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> On 25.10.2024 11:50, 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
> >>>
> >>> @@ -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.
>
> No. Putting it on an option without prompt will, iirc, only cause a warning
> when violated, but will otherwise have no real effect. The "select" of
> HAS_LLC_COLORING wants to become dependent upon !NUMA, until that combination
> was made work.

Ok, got it.

> >>> --- /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.
>
> I don't know what the policy is either. I think it can be there or it can
> be omitted. Yet if it's there, I think it wants to be accurate at least at
> the time a new file is being added. (These lines usually aren't updated
> when later changes are made to the files.)

Ok, makes sense.

> >>> +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.
>
> Well, you may not care, but imo the resulting configuration ought to reflect
> what was requested on the command line (maybe unless e.g. documentation
> explicitly says otherwise). If way_size has low bits set, that wouldn't be
> the case.

Ok.

> Jan

- Carlo