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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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