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

[Penny Zheng <Penny.Zheng@arm.com>]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, April 19, 2022 5:11 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v2 5/6] xen/arm: unpopulate memory when domain is
> static
> 
> 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()?

> Jan