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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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.

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

> --- 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.

> --- /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.

> --- 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.

Jan

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