Re: [PATCH v4 01/11] xen/common: add cache coloring common code

From: Jan Beulich <jbeulich@suse.com>
To: Carlo Nonato <carlo.nonato@minervasys.tech>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Marco Solieri <marco.solieri@minervasys.tech>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v4 01/11] xen/common: add cache coloring common code
Date: Tue, 24 Jan 2023 17:37:08 +0100	[thread overview]
Message-ID: <a470be46-ab6e-3970-2b04-6f4035adf1cb@suse.com> (raw)
In-Reply-To: <20230123154735.74832-2-carlo.nonato@minervasys.tech>

On 23.01.2023 16:47, Carlo Nonato wrote:
> @@ -769,6 +776,13 @@ struct domain *domain_create(domid_t domid,
>      return ERR_PTR(err);
>  }
>  
> +struct domain *domain_create(domid_t domid,
> +                             struct xen_domctl_createdomain *config,
> +                             unsigned int flags)
> +{
> +    return domain_create_llc_colored(domid, config, flags, 0, 0);

Please can you use NULL when you mean a null pointer?

> --- /dev/null
> +++ b/xen/include/xen/llc_coloring.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Last Level Cache (LLC) coloring common header
> + *
> + * Copyright (C) 2022 Xilinx Inc.
> + *
> + * Authors:
> + *    Carlo Nonato <carlo.nonato@minervasys.tech>
> + */
> +#ifndef __COLORING_H__
> +#define __COLORING_H__
> +
> +#include <xen/sched.h>
> +#include <public/domctl.h>
> +
> +#ifdef CONFIG_HAS_LLC_COLORING
> +
> +#include <asm/llc_coloring.h>
> +
> +extern bool llc_coloring_enabled;
> +
> +int domain_llc_coloring_init(struct domain *d, unsigned int *colors,
> +                             unsigned int num_colors);
> +void domain_llc_coloring_free(struct domain *d);
> +void domain_dump_llc_colors(struct domain *d);
> +
> +#else
> +
> +#define llc_coloring_enabled (false)

While I agree this is needed, ...

> +static inline int domain_llc_coloring_init(struct domain *d,
> +                                           unsigned int *colors,
> +                                           unsigned int num_colors)
> +{
> +    return 0;
> +}
> +static inline void domain_llc_coloring_free(struct domain *d) {}
> +static inline void domain_dump_llc_colors(struct domain *d) {}

... I don't think you need any of these. Instead the declarations above
simply need to be visible unconditionally (to be visible to the compiler
when processing consuming code). We rely on DCE to remove such references
in many other places.

> +#endif /* CONFIG_HAS_LLC_COLORING */
> +
> +#define is_domain_llc_colored(d) (llc_coloring_enabled)
> +
> +#endif /* __COLORING_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> \ No newline at end of file

This wants taking care of.

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -602,6 +602,9 @@ struct domain
>  
>      /* Holding CDF_* constant. Internal flags for domain creation. */
>      unsigned int cdf;
> +
> +    unsigned int *llc_colors;
> +    unsigned int num_llc_colors;
>  };

Why outside of any #ifdef, and why not in struct arch_domain?

Jan