Re: [PATCH 06/12] xen/common: add cache coloring allocator for domains

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.09.2022 00:42, Stefano Stabellini wrote:
> On Mon, 19 Sep 2022, Jan Beulich wrote:
>> On 16.09.2022 18:05, Carlo Nonato wrote:
>>> On Thu, Sep 15, 2022 at 3:13 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 26.08.2022 14:51, Carlo Nonato wrote:
>>>>> --- a/xen/arch/arm/Kconfig
>>>>> +++ b/xen/arch/arm/Kconfig
>>>>> @@ -147,6 +147,18 @@ config MAX_CACHE_COLORS
>>>>>         colors at boot. Note that if, at any time, a color configuration 
>>>>> with more
>>>>>         colors than the maximum will be employed an error will be 
>>>>> produced.
>>>>>
>>>>> +config BUDDY_ALLOCATOR_SIZE
>>>>> +     string "Buddy allocator reserved memory size" if CACHE_COLORING
>>>>> +     default "64M" if CACHE_COLORING
>>>>> +     default "0M" if !CACHE_COLORING
>>>>
>>>> I don't understand the purpose of this last line, nor the two earlier
>>>> "if". Why not simply
>>>>
>>>> config BUDDY_ALLOCATOR_SIZE
>>>>         string "Buddy allocator reserved memory size"
>>>>         depend on CACHE_COLORING
>>>>         default "64M"
>>>
>>> This was just to have a value for the config option even with cache coloring
>>> disabled. All those ifs emulate the "depends on" keyword, but let the
>>> CONFIG_BUDDY_ALLOCATOR_SIZE takes "0M" when coloring is disabled. With just
>>> the "depends on" the macro isn't defined at all. I know that this can be
>>> handled with some simple #ifdef, but I found this way to be more elegant.
>>> Not an expert here so if you prefer the other way or a whole different one
>>> (more readable/better fitted) please let me know.
>>
>> As far as I saw, the sole use was already inside a suitable #ifdef. Hence
>> yes, I clearly would see "depends on" as the better fit. Please also don't
>> forget that if later cache coloring would be enabled for another
>> architecture, that default of zero (pre-recorded in a .config) would get
>> in the way; one would need to manually change it (and remember to do so).
>>
>>>> Finally - how much of this is really Arm-specific? Shouldn't this be a
>>>> common config option, perhaps merely restricted to Arm by the top level
>>>> option (CACHE_COLORING?) depending on a further HAS_CACHE_COLORING,
>>>> which only Arm would select?
>>>
>>> I'm sorry, but I don't understand your suggestion. BUDDY_ALLOCATOR_SIZE
>>> is Arm specific because CACHE_COLORING is. In fact it depends only on this
>>> last config value and not on Arm config directly. Why should someone limit
>>> the buddy allocator when coloring isn't enabled?
>>
>> My comment wasn't on this on setting alone, but on the coloring ones as a
>> set.
>>
>>> I've lost you on the HAS_CACHE_COLORING. Why should Arm config select this
>>> one? Cache coloring must remain optional. I'm probably missing something.
>>
>> HAS_* settings only express what an arch is capable of; they don't replace
>> dependent options which actually are user-selectable. (That said, we have
>> a number where there's no user selection possible, but that's not of
>> interest here.)
>>
>>>>> --- a/xen/arch/arm/coloring.c
>>>>> +++ b/xen/arch/arm/coloring.c
>>>>> @@ -300,6 +300,16 @@ void prepare_color_domain_config(struct 
>>>>> xen_arch_domainconfig *config,
>>>>>      config->num_colors = (uint16_t)num;
>>>>>  }
>>>>>
>>>>> +unsigned int page_to_color(struct page_info *pg)
>>>>
>>>> The parameter will want to be pointer-to-const and I wonder whether ...
>>>>
>>>>> +{
>>>>> +    return addr_to_color(page_to_maddr(pg));
>>>>> +}
>>>>
>>>> ... the function as a whole wouldn't be a good candidate for being an
>>>> inline one (requiring addr_to_color() to be available in outside of
>>>> this file, of course).
>>>
>>> You mean defining it as static inline in the coloring.h header?
>>
>> That would seem preferable for a simple function like this one.
>>
>>>>> +static void color_heap_insert_page(struct page_info *pg)
>>>>> +{
>>>>> +    struct page_info *pos;
>>>>> +    struct page_list_head *head = colored_pages(page_to_color(pg));
>>>>> +
>>>>> +    pg->count_info |= PGC_colored;
>>>>
>>>> The function isn't marked __init, so runtime correctness as to the
>>>> (non-atomic) update here wants clarifying.
>>>
>>> Yes. I need to check and probably add a spin lock for the color heap.
>>
>> I'm afraid a spin lock won't help. May I suggest you look at some of
>> the staticmem discussions that had happened, including a similar
>> topic. (Sorry, I don't have a link at hand to the exact mail.)
> 
> I searched through the recent staticmem discussions to try to provide a
> helpful link for Carlo, but I don't think I managed to find what you had
> in mind. I found these two lock-related emails:
> 
> https://marc.info/?l=xen-devel&m=165476642832402
> https://marc.info/?l=xen-devel&m=165632461420257
> 
> If they are not relevant, could you please provide a few more details?

Those aren't the ones. The point is that count_info is a somewhat
odd field: It's not consistently updated under (all the same) lock,
and it's also not consistently updated atomically. Hence new
updates that appear in the code need properly justifying that the
way updates are done there doesn't conflict with any other of the
already existing updates.

Jan