> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: 17 March 2020 10:45
> To: p...@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Andrew Cooper'
> <andrew.coop...@citrix.com>; 'George Dunlap'
> <george.dun...@citrix.com>; 'Ian Jackson' <ian.jack...@eu.citrix.com>;
> 'Julien Grall'
> <jul...@xen.org>; 'Stefano Stabellini' <sstabell...@kernel.org>; 'Wei Liu'
> <w...@xen.org>; 'Roger Pau
> Monné' <roger....@citrix.com>
> 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 <jbeul...@suse.com>
> >> Sent: 16 March 2020 16:53
> >>
> >> On 10.03.2020 18:49, p...@xen.org 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 <jbeul...@suse.com>
Thanks,
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
