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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Julien and Jan,

On Thu, Jan 26, 2023 at 11:16 AM Julien Grall <julien@xxxxxxx> wrote:
>
> 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.

Understood. Thanks to both.

> Cheers,
>
> --
> Julien Grall