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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 17 March 2020 10:45
> To: paul@xxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; 'Andrew Cooper' 
> <andrew.cooper3@xxxxxxxxxx>; 'George Dunlap'
> <george.dunlap@xxxxxxxxxx>; 'Ian Jackson' <ian.jackson@xxxxxxxxxxxxx>; 
> 'Julien Grall'
> <julien@xxxxxxx>; 'Stefano Stabellini' <sstabellini@xxxxxxxxxx>; 'Wei Liu' 
> <wl@xxxxxxx>; 'Roger Pau
> Monné' <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH v6 2/5] mm: keep PGC_extra pages on a separate list
> 
> On 16.03.2020 19:11, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: 16 March 2020 16:53
> >>
> >> On 10.03.2020 18:49, paul@xxxxxxx wrote:
> >>> --- 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.)
> >>
> >
> > I don't think there is any need to adjust as the current code in will
> > drop the allocation ref in vmx_free_vlapic_mapping(), so it doesn't
> > matter that it is missed by relinquish_memory().
> 
> Hmm, yes. It feels like thin ice, but I think you're right. Nevertheless
> the freeing of extra pages should imo ultimately move to the central
> place, i.e. it would seem more clean to me if the put_page_alloc_ref()
> call was dropped from vmx_free_vlapic_mapping().
> 
> >>> --- 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.
> >>
> >
> > Are you saying it should be:
> >
> > #define arch_free_heap_page(d, pg) \
> >     page_list_del2(pg, page_to_list(d, pg), &(d)->arch.relmem_list)
> 
> Yes. But if this and the other cosmetic changes are the only changes to
> make, I'd be fine to do so while committing (if no other reason for a
> v7 arises):
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks,

  Paul


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