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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi, jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, April 25, 2022 4:01 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>;
> Wei Liu <wl@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 5/6] xen/arm: unpopulate memory when domain is
> static
> 
> 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.
> 

Oh, oh, thanks!
put parentheses around the braces, then that's what you said about make it 
be an expression

> Jan