On 25.01.2023 17:18, Carlo Nonato wrote: > On Wed, Jan 25, 2023 at 2:10 PM Jan Beulich <jbeul...@suse.com> wrote: >> On 25.01.2023 12:18, Carlo Nonato wrote: >>> On Tue, Jan 24, 2023 at 5:37 PM Jan Beulich <jbeul...@suse.com> 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
