Re: [Xen-devel] [PATCH v8 2/4] add a domain_tot_pages() helper function

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 31 January 2020 12:53
> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau Monné
> <roger.pau@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxxxxx>; Ian
> Jackson <ian.jackson@xxxxxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Konrad
> Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Tim Deegan <tim@xxxxxxx>
> Subject: Re: [PATCH v8 2/4] add a domain_tot_pages() helper function
> 
> On 30.01.2020 15:57, Paul Durrant wrote:
> > v8:
> >  - New in v8
> > ---
> >  xen/arch/x86/domain.c           |  2 +-
> >  xen/arch/x86/mm.c               |  6 +++---
> >  xen/arch/x86/mm/p2m-pod.c       | 10 +++++-----
> >  xen/arch/x86/mm/shadow/common.c |  2 +-
> >  xen/arch/x86/msi.c              |  2 +-
> >  xen/arch/x86/numa.c             |  2 +-
> >  xen/arch/x86/pv/dom0_build.c    | 25 +++++++++++++------------
> >  xen/arch/x86/pv/domain.c        |  2 +-
> >  xen/common/domctl.c             |  2 +-
> >  xen/common/grant_table.c        |  4 ++--
> >  xen/common/keyhandler.c         |  2 +-
> >  xen/common/memory.c             |  4 ++--
> >  xen/common/page_alloc.c         | 15 ++++++++-------
> >  xen/include/public/memory.h     |  4 ++--
> >  xen/include/xen/sched.h         | 24 ++++++++++++++++++------
> >  15 files changed, 60 insertions(+), 46 deletions(-)
> 
> From this, with the comment you add next to the struct field, and
> with your reply yesterday, what about the uses in
> - arch/arm/arm64/domctl.c:switch_mode(),

TBH I'm not sure with that one. It looks to me like it needs to check whether 
the domain has *any* memory assigned. Perhaps checking page_list would be more 
appropriate. Perhaps Julien can comment?

> - arch/x86/pv/shim.c:pv_shim_setup_dom(),
> - arch/x86/pv/shim.c:write_start_info()?

It looks like both of those should be changed.

> 
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -4194,8 +4194,8 @@ long do_mmu_update(
> >   * - page caching attributes cleaned up
> >   * - removed from the domain's page_list
> >   *
> > - * If MEMF_no_refcount is not set, the domain's tot_pages will be
> > - * adjusted.  If this results in the page count falling to 0,
> > + * If MEMF_no_refcount is not set, the domain_adjust_tot_pages() will
> > + * be called.  If this results in the page count falling to 0,
> >   * put_domain() will be called.
> 
> If you fiddle with this comment, please also drop the "the" ahead
> of the function name. Unless you as a native speaker would confirm
> it's appropriate there (it doesn't seem so to me). Of course I
> also wouldn't mind leaving this untouched altogether.
> 

Ok, I'll drop that hunk.

> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -717,7 +717,7 @@ static long
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
> >
> >                  /*
> >                   * Pages in in_chunk_list is stolen without
> > -                 * decreasing the tot_pages. If the domain is dying
> when
> > +                 * decreasing domain_tot_pages(). If the domain is
> dying when
> 
> I'd leave this comment alone, or at least not use the function
> name. Maybe do as you did in the public header?
> 

OK I'll leave this alone too.

> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -364,12 +364,18 @@ struct domain
> >      spinlock_t       page_alloc_lock; /* protects all the following
> fields  */
> >      struct page_list_head page_list;  /* linked list */
> >      struct page_list_head xenpage_list; /* linked list (size
> xenheap_pages) */
> > -    unsigned int     tot_pages;       /* number of pages currently
> possesed */
> > -    unsigned int     xenheap_pages;   /* # pages allocated from Xen
> heap    */
> > -    unsigned int     outstanding_pages; /* pages claimed but not
> possessed  */
> > -    unsigned int     max_pages;       /* maximum value for tot_pages
> */
> > -    atomic_t         shr_pages;       /* number of shared pages
> */
> > -    atomic_t         paged_pages;     /* number of paged-out pages
> */
> > +
> > +    /*
> > +     * This field should only be directly accessed by
> domain_adjust_tot_pages()
> > +     * and the domain_tot_pages() helper function defined below.
> > +     */
> > +    unsigned int     tot_pages;
> 
> If the three missing ones got taken care of, with there being arguments
> both pro and con your change to dump_pageframe_info(), I'd be okay with
> it getting changed as you do, to not render this comment partially
> wrong.

Ok.

  Paul

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