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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

On Wed, Jan 25, 2023 at 2:10 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 25.01.2023 12:18, Carlo Nonato wrote:
> > On Tue, Jan 24, 2023 at 5:37 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> On 23.01.2023 16:47, Carlo Nonato wrote:
> >>> --- /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?
>
> Likely. I didn't look at most of the Arm-only pieces.
>
> >>> --- 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.
>
> If you mean this to be usable by other arch-es as well (which I would
> welcome, as I think I had expressed on an earlier version), then I think
> more pieces want to be in common code. But putting the fields here and all
> users of them in arch-specific code (which I think is the way I saw it)
> doesn't look very logical to me. IOW to me there exist only two possible
> approaches: As much as possible in common code, or common code being
> disturbed as little as possible.

This means having a llc-coloring.c in common where to put the common
implementation, right?
Anyway right now there is also another user of such fields in common:
page_alloc.c.

> > 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).
>
> I went about five levels deep in the replies, without finding any such reply
> from Julien. Can you please be more specific with the link, so readers don't
> need to endlessly dig?

https://marc.info/?l=xen-devel&m=166669617917298

quote (me and then Julien):
>> We can also think of moving the coloring fields from this
>> struct to the common one (xen_domctl_createdomain) protecting them with
>> the proper #ifdef (but we are targeting only arm64...).

> Your code is targeting arm64 but fundamentally this is an arm64 specific
> feature. IOW, this could be used in the future on other arch. So I think
> it would make sense to define it in common without the #ifdef.

> Jan
>
> > 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?
> >
> > Thanks.
> >
> > - Carlo Nonato
>