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 <lucmic...@gmail.com>
>>
>> Signed-off-by: Carlo Nonato <carlo.non...@minervasys.tech>
>> Signed-off-by: Marco Solieri <marco.soli...@minervasys.tech>
>> Acked-by: Michal Orzel <michal.or...@amd.com>
>
> [...]
>>
>> +### 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
