Hi, jan > -----Original Message----- > From: Jan Beulich <[email protected]> > Sent: Monday, April 25, 2022 4:01 PM > To: Penny Zheng <[email protected]> > Cc: Wei Chen <[email protected]>; Stefano Stabellini > <[email protected]>; Julien Grall <[email protected]>; Bertrand Marquis > <[email protected]>; Volodymyr Babchuk > <[email protected]>; Andrew Cooper > <[email protected]>; George Dunlap <[email protected]>; > Wei Liu <[email protected]>; [email protected] > 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 <[email protected]> > >> 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
