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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 ...

Jan