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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 26/01/2023 08:06, Jan Beulich wrote:
> On 25.01.2023 17:18, Carlo Nonato wrote:
> > 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:
> > > > > > --- 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?
> Likely, yes.
>
> > Anyway right now there is also another user of such fields in common:
> > page_alloc.c.
> Yet hopefully all inside suitable #ifdef.
>
> > > > 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.
> I'm inclined to read this as a dislike for "#ifdef CONFIG_ARM64", not for
> "#ifdef CONFIG_LLC_COLORING" (or whatever the name of the option was). But
> I guess only Julien can clarify this ...
Your interpretation is correct. I would prefer if the fields are 
protected with #ifdef CONFIG_LLC_COLORING.
Cheers,

--
Julien Grall