Re: [PATCH v2 5/6] xen/arm: unpopulate memory when domain is static

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.04.2022 08:34, Penny Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Tuesday, April 19, 2022 5:11 PM
>>
>> On 18.04.2022 14:22, Penny Zheng wrote:
>>> --- a/xen/arch/arm/include/asm/mm.h
>>> +++ b/xen/arch/arm/include/asm/mm.h
>>> @@ -358,6 +358,23 @@ void clear_and_clean_page(struct page_info
>>> *page);
>>>
>>>  unsigned int arch_get_dma_bitsize(void);
>>>
>>> +/*
>>> + * Put free pages on the resv page list after having taken them
>>> + * off the "normal" page list, when pages from static memory  */
>>> +#ifdef CONFIG_STATIC_MEMORY
>>> +#define arch_free_heap_page(d, pg) {                    \
>>> +    if ( (pg)->count_info & PGC_reserved )              \
>>> +    {                                                   \
>>> +        page_list_del(pg, page_to_list(d, pg));         \
>>> +        page_list_add_tail(pg, &(d)->resv_page_list);   \
>>> +        (d)->resv_pages++;                              \
>>
>> There's no consumer of this counter, so I'd like to ask that it be introduced
>> once a consumer appears.
>>
>>> +    }                                                   \
>>> +    else                                                \
>>> +        page_list_del(pg, page_to_list(d, pg));         \
>>
>> Is there a particular reason to have this page_list_del() twice, instead of 
>> just
>> once ahead of the if()?
>>
>>> +}
>>
>> Also this entire construct want to be an expression, not a
>> (compound) statement. And it probably would better evaluate its parameters
>> just once.
>>
> 
> #define arch_free_heap_page(d, pg) {                    \
>         page_list_del(pg, page_to_list(d, pg));             \
>         if ( (pg)->count_info & PGC_reserved )              \
>              page_list_add_tail(pg, &(d)->resv_page_list);   \
> }
> 
> I'm trying to refine the arch_free_heap_page() here, but I'm a bit confused
> about to let it be an expression, not a compound statement.  Do you mean that
> you prefer to let the if-clause out of the arch_free_heap_page()?

No. You want to put parentheses around the braces, using a gcc extension
we make extensive use of throughout the code base.

Jan