Re: [PATCH v11 01/12] xen/common: add cache coloring common code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.12.2024 10:55, Michal Orzel wrote:
> 
> 
> On 02/12/2024 17:59, Carlo Nonato wrote:
>>
>>
>> Last Level Cache (LLC) coloring allows to partition the cache in smaller
>> chunks called cache colors.
>>
>> Since not all architectures can actually implement it, add a HAS_LLC_COLORING
>> Kconfig option.
>> LLC_COLORS_ORDER Kconfig option has a range maximum of 10 (2^10 = 1024)
>> because that's the number of colors that fit in a 4 KiB page when integers
>> are 4 bytes long.
>>
>> LLC colors are a property of the domain, so struct domain has to be extended.
>>
>> Based on original work from: Luca Miccio <lucmiccio@xxxxxxxxx>
>>
>> Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
>> Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
>> Acked-by: Michal Orzel <michal.orzel@xxxxxxx>
> 
> [...]
>>
>> +### llc-coloring (arm64)
>> +> `= <boolean>`
>> +
>> +> Default: `false`
> By default, it is disabled. If CONFIG_ is enabled but ...
> 
> [...]
> 
>> + * -1: not specified (disabled unless llc-size and llc-nr-ways present)
> the user doesn't specify any llc-* options, LLC feature should be disabled.
> In your case llc_coloring_enabled is -1 and due to 'if ( llc_coloring_enabled 
> ... )' checks
> all around the code base, the LLC will be enabled even though it should not.
> 
> You can either set it to 0 if (llc_coloring_enabled < 0) and other llc-* 
> options have not been provided
> (this would require modifying this comment to provide different meaning 
> depending on the context) or
> you could do sth like that:

I agree the below is going to be better in terms of both readability and
consistency. A few minor comments though:

> --- a/xen/common/llc-coloring.c
> +++ b/xen/common/llc-coloring.c
> @@ -18,8 +18,10 @@
>   *  0: explicitly disabled through cmdline
>   *  1: explicitly enabled through cmdline
>   */
> -int8_t __ro_after_init llc_coloring_enabled = -1;
> -boolean_param("llc-coloring", llc_coloring_enabled);
> +int8_t __init opt_llc_coloring = -1;

__initdata

> +boolean_param("llc-coloring", opt_llc_coloring);
> +
> +bool __ro_after_init llc_coloring_enabled = false;
> 
>  static unsigned int __initdata llc_size;
>  size_param("llc-size", llc_size);
> @@ -147,15 +149,17 @@ void __init llc_coloring_init(void)
>  {
>      unsigned int way_size, i;
> 
> -    if ( (llc_coloring_enabled < 0) && (llc_size && llc_nr_ways) )
> +    if ( (opt_llc_coloring < 0) && (llc_size && llc_nr_ways) )

Excess parentheses (&& doesn't need parenthesizing against another &&).

>      {
>          llc_coloring_enabled = true;

This becomes appropriate only with the variable's type changing back
to bool.

Jan