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

[Jan Beulich <jbeulich@xxxxxxxx>]

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(),
- arch/x86/pv/shim.c:pv_shim_setup_dom(),
- arch/x86/pv/shim.c:write_start_info()?

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

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

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

Jan

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