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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan, Julien

On Tue, Jan 24, 2023 at 5:37 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxx>
> > + */
> > +#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.

So this is true for any other stub function that I used in the series, right?
Since all of them are guarded by the same kind of if statement: checking for
llc_coloring_enabled value which, in case of coloring disabled from Kconfig,
is always false and then DCE comes in. Sorry for being so verbose, but I just
want to be sure I understood.

> > +#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?

Moving this in sched.h seemed like the natural continuation of the common +
arch specific split. Notice that this split is also because Julien pointed
out (as you did in some earlier revision) that cache coloring can be used
by other arch in the future (even if x86 is excluded). Having two maintainers
saying the same thing sounded like a good reason to do that.

The missing #ifdef comes from a discussion I had with Julien in v2 about
domctl interface where he suggested removing it
(https://marc.info/?l=xen-devel&m=166151802002263). We were talking about
a different struct, but I thought the principle was the same. Anyway I would
like the #ifdef too.

So @Jan, @Julien, can you help me fix this once for all?

> Jan

Thanks.

- Carlo Nonato