[v6,2/5] mm: keep PGC_extra pages on a separate list

Commit Message

Paul Durrant March 10, 2020, 5:49 p.m. UTC

From: Paul Durrant <[e-mail]>

This patch adds a new page_list_head into struct domain to hold PGC_extra
pages. This avoids them getting confused with 'normal' domheap pages where
the domain's page_list is walked.

A new dump loop is also added to dump_pageframe_info() to unconditionally
dump the 'extra page list'.

Signed-off-by: Paul Durrant <[e-mail]>
---
Cc: Andrew Cooper <[e-mail]>
Cc: George Dunlap <[e-mail]>
Cc: Ian Jackson <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Julien Grall <[e-mail]>
Cc: Stefano Stabellini <[e-mail]>
Cc: Wei Liu <[e-mail]>
Cc: "Roger Pau Monné" <[e-mail]>

v6:
 - New in v6
---
 xen/arch/x86/domain.c    |  7 +++++++
 xen/common/domain.c      |  1 +
 xen/common/page_alloc.c  |  2 +-
 xen/include/asm-x86/mm.h |  6 ++----
 xen/include/xen/mm.h     |  5 ++---
 xen/include/xen/sched.h  | 12 ++++++++++++
 6 files changed, 25 insertions(+), 8 deletions(-)

Comments

Jan Beulich March 16, 2020, 4:53 p.m. UTC | #1

On 10.03.2020 18:49, [e-mail] 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

Paul Durrant March 16, 2020, 6:11 p.m. UTC | #2

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 16 March 2020 16:53
> To: [e-mail]
> Cc: [e-mail]; Andrew Cooper <[e-mail]>; George Dunlap
> <[e-mail]>; Ian Jackson <[e-mail]>; Julien Grall <[e-mail]>;
> Stefano Stabellini <[e-mail]>; Wei Liu <[e-mail]>; Roger Pau Monné
> <[e-mail]>
> Subject: Re: [PATCH v6 2/5] mm: keep PGC_extra pages on a separate list
> 
> On 10.03.2020 18:49, [e-mail] 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.
> 

Ok.

> > --- 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().

> > --- 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)

?

> > --- 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".
>

Oh yes.

  Paul

 
> Jan

Jan Beulich March 17, 2020, 10:45 a.m. UTC | #3

On 16.03.2020 19:11, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: 16 March 2020 16:53
>>
>> On 10.03.2020 18:49, [e-mail] 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 <[e-mail]>

Jan

Paul Durrant March 17, 2020, 10:51 a.m. UTC | #4

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 17 March 2020 10:45
> To: [e-mail]
> Cc: [e-mail]; 'Andrew Cooper' <[e-mail]>; 'George Dunlap'
> <[e-mail]>; 'Ian Jackson' <[e-mail]>; 'Julien Grall'
> <[e-mail]>; 'Stefano Stabellini' <[e-mail]>; 'Wei Liu' <[e-mail]>; 'Roger Pau
> Monné' <[e-mail]>
> 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 <[e-mail]>
> >> Sent: 16 March 2020 16:53
> >>
> >> On 10.03.2020 18:49, [e-mail] 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 <[e-mail]>

Thanks,

  Paul

Patch

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index bdcc0d972a..2dda2dbca1 100644
--- 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);
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index ba7a905258..4ef0d3b21e 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -406,6 +406,7 @@  struct domain *domain_create(domid_t domid,
     spin_lock_init_prof(d, page_alloc_lock);
     spin_lock_init(&d->hypercall_deadlock_mutex);
     INIT_PAGE_LIST_HEAD(&d->page_list);
+    INIT_PAGE_LIST_HEAD(&d->extra_page_list);
     INIT_PAGE_LIST_HEAD(&d->xenpage_list);
 
     spin_lock_init(&d->node_affinity_lock);
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 76d37226df..10b7aeca48 100644
--- 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]));
     }
 
  out:
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index a06b2fb81f..81beb359e1 100644
--- 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)
 
 extern const char zero_page[];
 
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index d0d095d9c7..0769e376d2 100644
--- 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)))
 #endif
 
 int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index f41d0ad2a0..85433e0bb1 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -355,6 +355,7 @@  struct domain
 
     spinlock_t       page_alloc_lock; /* protects all the following fields  */
     struct page_list_head page_list;  /* linked list */
+    struct page_list_head extra_page_list; /* linked list (size extra_pages) */
     struct page_list_head xenpage_list; /* linked list (size xenheap_pages) */
 
     /*
@@ -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 )
+        return &d->extra_page_list;
+
+    return &d->page_list;
+}
+
 /* Return number of pages currently posessed by the domain */
 static inline unsigned int domain_tot_pages(const struct domain *d)
 {