[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/2] xen/mm: Introduce PGC_state_uninitialised



On 19.03.2020 22:21, David Woodhouse wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -491,7 +491,8 @@ void share_xen_page_with_guest(struct page_info *page, 
> struct domain *d,
>  
>      page_set_owner(page, d);
>      smp_wmb(); /* install valid domain ptr before updating refcnt. */
> -    ASSERT((page->count_info & ~PGC_xen_heap) == 0);
> +    ASSERT((page->count_info & ~PGC_xen_heap) == PGC_state_inuse ||
> +           (page->count_info & ~PGC_xen_heap) == PGC_state_uninitialised);

Like for patch 1, there's a risk of the page transitioning from
uninitialised to inuse between these two comparisons, making the
ASSERT() trigger when it shouldn't. As you've got two more
similar constructs further down in the patch, maybe this also
warrants a helper function/macro?

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -252,6 +252,8 @@ struct bootmem_region {
>  static struct bootmem_region __initdata
>      bootmem_region_list[PAGE_SIZE / sizeof(struct bootmem_region)];
>  static unsigned int __initdata nr_bootmem_regions;
> +static void init_heap_pages(struct page_info *pg, unsigned long nr_pages,
> +                            bool scrub);
>  
>  struct scrub_region {
>      unsigned long offset;
> @@ -1390,6 +1392,17 @@ static void free_heap_pages(
>      ASSERT(order <= MAX_ORDER);
>      ASSERT(node >= 0);
>  
> +    if ( page_state_is(pg, uninitialised) )
> +    {
> +        init_heap_pages(pg, 1 << order, need_scrub);

While, at least for now, it shouldn't matter in practice, and
while code overall is very inconsistent in this regard, with
the respective parameter type being "unsigned long" may I
suggest to use 1UL here?

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.