Re: [Xen-devel] [PATCH v6 2/5] mm: keep PGC_extra pages on a separate list

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2020 18:49, paul@xxxxxxx wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -257,6 +257,13 @@ void dump_pageframe_info(struct domain *d)
>                 _p(mfn_x(page_to_mfn(page))),
>                 page->count_info, page->u.inuse.type_info);
>      }
> +
> +    page_list_for_each ( page, &d->extra_page_list )
> +    {
> +        printk("    ExtraPage %p: caf=%08lx, taf=%" PRtype_info "\n",
> +               _p(mfn_x(page_to_mfn(page))),
> +               page->count_info, page->u.inuse.type_info);
> +    }
>      spin_unlock(&d->page_alloc_lock);

Another blank line above here would have been nice.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2314,7 +2314,7 @@ int assign_pages(
>          smp_wmb(); /* Domain pointer must be visible before updating refcnt. 
> */
>          pg[i].count_info =
>              (pg[i].count_info & PGC_extra) | PGC_allocated | 1;
> -        page_list_add_tail(&pg[i], &d->page_list);
> +        page_list_add_tail(&pg[i], page_to_list(d, &pg[i]));
>      }

This moves the one extra page we currently have (VMX'es APIC access
page) to a different list. Without adjustment to domain cleanup,
how is this page now going to get freed? (This of course then should
be extended to Arm, even if right now there's no "extra" page there.)

> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -629,10 +629,8 @@ typedef struct mm_rwlock {
>      const char        *locker_function; /* func that took it */
>  } mm_rwlock_t;
>  
> -#define arch_free_heap_page(d, pg)                                      \
> -    page_list_del2(pg, is_xen_heap_page(pg) ?                           \
> -                       &(d)->xenpage_list : &(d)->page_list,            \
> -                   &(d)->arch.relmem_list)
> +#define arch_free_heap_page(d, pg) \
> +    page_list_del2(pg, page_to_list((d), (pg)), &(d)->arch.relmem_list)

Arguments passed on as is (i.e. not as part of an expression) don't
need parentheses.

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -583,9 +583,8 @@ static inline unsigned int get_order_from_pages(unsigned 
> long nr_pages)
>  void scrub_one_page(struct page_info *);
>  
>  #ifndef arch_free_heap_page
> -#define arch_free_heap_page(d, pg)                      \
> -    page_list_del(pg, is_xen_heap_page(pg) ?            \
> -                      &(d)->xenpage_list : &(d)->page_list)
> +#define arch_free_heap_page(d, pg) \
> +    page_list_del(pg, page_to_list((d), (pg)))

Same here then.

> @@ -538,6 +539,17 @@ struct domain
>  #endif
>  };
>  
> +static inline struct page_list_head *page_to_list(
> +    struct domain *d, const struct page_info *pg)
> +{
> +    if ( is_xen_heap_page(pg) )
> +        return &d->xenpage_list;
> +    else if ( pg->count_info & PGC_extra )

Unnecessary "else".

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel