Re: [PATCH v10 09/12] xen: add cache coloring allocator for domains

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.11.2024 10:37, Carlo Nonato wrote:
> On Thu, Nov 28, 2024 at 12:43 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 19.11.2024 15:13, Carlo Nonato wrote:
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -537,4 +537,12 @@ config LLC_COLORS_ORDER
>>>         The default value corresponds to an 8 MiB 16-ways LLC, which should 
>>> be
>>>         more than what's needed in the general case.
>>>
>>> +config BUDDY_ALLOCATOR_SIZE
>>> +     int "Buddy allocator reserved memory size (MiB)" if LLC_COLORING
>>> +     default "0" if !LLC_COLORING
>>> +     default "64"
>>> +     help
>>> +       Amount of memory reserved for the buddy allocator to serve Xen heap,
>>> +       working alongside the colored one.
>>
>> As the description has nothing in this regard: Why again is it that this
>> can't simply have "depends on LLC_COLORING"? Even if it ends up with a
>> value of 0, in an LLC_COLORING=n (or LLC_COLORING entirely absent) .config
>> I'd find it at least irritating to see this setting to be there.
> 
> Quoting you from v8 (6 months ago, a lot of time ago I know, link here
> https://patchew.org/Xen/20240502165533.319988-1-carlo.nonato@xxxxxxxxxxxxxxx/20240502165533.319988-10-carlo.nonato@xxxxxxxxxxxxxxx/#5c16f53f-3bb0-49d6-b174-b0e8c6ceff4c@xxxxxxxx):
>>> +/* Memory required for buddy allocator to work with colored one */
>>> +#ifdef CONFIG_LLC_COLORING
>>> +static unsigned long __initdata buddy_alloc_size =
>>> +    MB(CONFIG_BUDDY_ALLOCATOR_SIZE);
>>
>> I think it would be quite nice if this and ...
>>
>>> +size_param("buddy-alloc-size", buddy_alloc_size);
>>> +
>>> +#define domain_num_llc_colors(d) (d)->num_llc_colors
>>> +#define domain_llc_color(d, i)   (d)->llc_colors[i]
>>> +#else
>>> +static unsigned long __initdata buddy_alloc_size;
>>
>> ... this were folded. Which I think would be possible if the Kconfig
>> addition went like this:
>>
>> config BUDDY_ALLOCATOR_SIZE
>> int "Buddy allocator reserved memory size (MiB)" if LLC_COLORING
>> default "0" if !LLC_COLORING
>> default "64"

Hmm, I see. I didn't consider the negative effect back at the time, it seems.
The main goal was anyway to get rid of the multiple instances of
buddy_alloc_size. I'm sure that can be easily achieved also when going back
to the original form of the Kconfig option (at least partly by following what
I said in the initial reply to this v10 patch, I think).

Jan