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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Mon, Jan 8, 2024 at 5:53 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 02.01.2024 10:51, Carlo Nonato wrote:
> > This commit adds the Last Level Cache (LLC) coloring common header, Kconfig
> > options and functions. Since this is an arch specific feature, actual
> > implementation is postponed to later patches and Kconfig options are placed
> > under xen/arch.
>
> As a general remark / nit: "This commit", "this patch", or alike aren't
> well suited for descriptions.
>
> > --- a/xen/arch/Kconfig
> > +++ b/xen/arch/Kconfig
> > @@ -31,3 +31,19 @@ config NR_NUMA_NODES
> >         associated with multiple-nodes management. It is the upper bound of
> >         the number of NUMA nodes that the scheduler, memory allocation and
> >         other NUMA-aware components can handle.
> > +
> > +config LLC_COLORING
> > +     bool "Last Level Cache (LLC) coloring" if EXPERT
> > +     depends on HAS_LLC_COLORING
> > +
> > +config NR_LLC_COLORS
> > +     int "Maximum number of LLC colors"
> > +     default 128
>
> What if I set to value to 0? Or to an unreasonably large one? You don't
> bound the value range at all.

I can reintroduce the range (it was there in previous versions). I just don't
know what numbers to put.
range 2 1024 seems reasonable since having 1 color only actually lowers
performances because of the obvious sharing of resources. 1024 are the colors
that can fit into a standard 4KiB page. It's big enough for currently
supported hardware that normally has 16 or 32 colors.

> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -7,6 +7,7 @@
> >  #include <xen/compat.h>
> >  #include <xen/init.h>
> >  #include <xen/lib.h>
> > +#include <xen/llc-coloring.h>
> >  #include <xen/ctype.h>
> >  #include <xen/err.h>
> >  #include <xen/param.h>
> > @@ -1144,6 +1145,9 @@ static void cf_check complete_domain_destroy(struct 
> > rcu_head *head)
> >      struct vcpu *v;
> >      int i;
> >
> > +    if ( is_domain_llc_colored(d) )
> > +        domain_llc_coloring_free(d);
>
> Would be nice if the freeing function could be called unconditionally,
> being a no-op for non-colored domains.

Ok.

> Further - is it really necessary to do this freeing this late?

No, I can move it in domain_destroy().

> > --- a/xen/common/keyhandler.c
> > +++ b/xen/common/keyhandler.c
> > @@ -6,6 +6,7 @@
> >  #include <xen/debugger.h>
> >  #include <xen/delay.h>
> >  #include <xen/keyhandler.h>
> > +#include <xen/llc-coloring.h>
> >  #include <xen/param.h>
> >  #include <xen/shutdown.h>
> >  #include <xen/event.h>
> > @@ -307,6 +308,9 @@ static void cf_check dump_domains(unsigned char key)
> >
> >          arch_dump_domain_info(d);
> >
> > +        if ( is_domain_llc_colored(d) )
> > +            domain_dump_llc_colors(d);
>
> I'm less concerned of the conditional here, but along the lines of the
> comment above, it could of course again be the function that simply is
> a no-op for non-colored domains.

Ok.

> > --- /dev/null
> > +++ b/xen/include/xen/llc-coloring.h
> > @@ -0,0 +1,46 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Last Level Cache (LLC) coloring common header
> > + *
> > + * Copyright (C) 2022 Xilinx Inc.
> > + *
> > + * Authors:
> > + *    Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> > + */
> > +#ifndef __COLORING_H__
> > +#define __COLORING_H__
> > +
> > +#include <xen/sched.h>
> > +#include <public/domctl.h>
> > +
> > +#ifdef CONFIG_HAS_LLC_COLORING
>
> Why does this matter here? IOW why ...
>
> > +#include <asm/llc-coloring.h>
> > +
> > +#ifdef CONFIG_LLC_COLORING
>
> ... is it not just this which is checked?
>
> > +extern bool llc_coloring_enabled;
> > +#define llc_coloring_enabled (llc_coloring_enabled)
> > +#endif
> > +
> > +#endif
> > +
> > +#ifndef llc_coloring_enabled
> > +#define llc_coloring_enabled (false)
> > +#endif
>
> +1 to the question Julien has raised here.

Yes this whole block can be better structured.

> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -626,6 +626,11 @@ struct domain
> >
> >      /* Holding CDF_* constant. Internal flags for domain creation. */
> >      unsigned int cdf;
> > +
> > +#ifdef CONFIG_LLC_COLORING
> > +    unsigned int *llc_colors;
>
> Can the color values change over the lifetime of a domain? If not,
> it may be prudent to have this be pointer-to-const.

Can I free a pointer-to-const array?

> Jan
>
> > +    unsigned int num_llc_colors;
> > +#endif
> >  };
> >
> >  static inline struct page_list_head *page_to_list(
>

Thanks.