Re: [PATCH v6 01/15] xen/common: add cache coloring common code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.02.2024 11:57, Carlo Nonato wrote:
> On Thu, Feb 1, 2024 at 1:59 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 29.01.2024 18:17, Carlo Nonato wrote:
>>> --- a/xen/arch/Kconfig
>>> +++ b/xen/arch/Kconfig
>>> @@ -31,3 +31,20 @@ config NR_NUMA_NODES
>>>         associated with multiple-nodes management. It is the upper bound of
>>>         the number of NUMA nodes that the scheduler, memory allocation and
>>>         other NUMA-aware components can handle.
>>> +
>>> +config LLC_COLORING
>>> +     bool "Last Level Cache (LLC) coloring" if EXPERT
>>> +     depends on HAS_LLC_COLORING
>>> +
>>> +config NR_LLC_COLORS
>>> +     int "Maximum number of LLC colors"
>>> +     range 2 1024
>>
>> What's the reasoning behind this upper bound? IOW - can something to this
>> effect be said in the description, please?
> 
> The only reason is that this is the number of colors that fit in a 4 KiB page.
> I don't have any other good way of picking a number here. 1024 is already big
> and probably nobody would use such a configuration. But 512 or 256 would be
> equally arbitrary.

And because of this I'm asking that you say in the description how you
arrived at this value. As to fitting in 4k-page: That makes two
assumptions (both true for all ports right now, but liable to be missed if
either changed down the road): PAGE_SIZE == 0x1000 && sizeof(int) == 4.

>>> --- /dev/null
>>> +++ b/xen/common/llc-coloring.c
>>> @@ -0,0 +1,87 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Last Level Cache (LLC) coloring common code
>>> + *
>>> + * Copyright (C) 2022 Xilinx Inc.
>>> + */
>>> +#include <xen/keyhandler.h>
>>> +#include <xen/llc-coloring.h>
>>> +#include <xen/param.h>
>>> +
>>> +bool __ro_after_init llc_coloring_enabled;
>>> +boolean_param("llc-coloring", llc_coloring_enabled);
>>
>> The variable has no use right now afaics, so it's unclear whether (a) it
>> is legitimately non-static and (b) placed in an appropriate section.
> 
> My bad here. The variable should be tested for in llc_coloring_init() and in
> domain_dump_llc_colors() (in domain_llc_coloring_free() as well, in later
> patches). That change was lost in the rebase of the series.
> 
> Anyway per this patch, the global is only accessed from this file while it's
> going to be accessed from outside in later patches. In this case what should
> I do? Declare it static and then make it non-static afterwards?

That would be preferred, considering that there may be an extended time
period between the 1st and 2nd patches going in. Explaining why a
variable is non-static despite not needing to be just yet would be an
alternative, but then you'd also need to justify why transiently
violating the respective Misra guideline is acceptable.

Jan