[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 1/3] xen/tools: Remove the XENMEM_get_oustanding_pages and provide the data via xc_phys_info



On Wed, 2013-05-08 at 19:35 +0100, Konrad Rzeszutek Wilk wrote:
> During the review of the patches it was noticed that there exists
> a race wherein the 'free_memory' value consists of information from
> two hypercalls. That is the XEN_SYSCTL_physinfo and 
> XENMEM_get_outstanding_pages.
> 
> The free memory the host has available for guest is the difference between
> the 'free_pages' (from XEN_SYSCTL_physinfo) and 'outstanding_pages'. As they
> are two hypercalls many things can happen in between the execution of them.
> 
> This patch resolves this by eliminating the XENMEM_get_outstanding_pages
> hypercall and providing the free_pages and outstanding_pages information
> via the xc_phys_info structure.
> 
> It also removes the XSM hooks and adds locking as needed.
> 
> CC: dgdera@xxxxxxxxxxxxx
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

For the tools side:
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

Needs a hypervisor ack though, since contrary to the subject line this
isn't just a tools change. Adding Keir, Tim & Jan (not sure which of
them is the right one here).

I've left the quotes untrimmed for the new CCs.

> ---
>  tools/libxc/xc_domain.c     |    9 ---------
>  tools/libxc/xenctrl.h       |    2 --
>  tools/libxl/libxl.c         |   15 +--------------
>  tools/libxl/libxl.h         |    1 -
>  tools/libxl/libxl_types.idl |    1 +
>  tools/libxl/xl_cmdimpl.c    |   16 +++-------------
>  xen/common/memory.c         |    8 --------
>  xen/common/page_alloc.c     |    8 ++++++--
>  xen/common/sysctl.c         |    3 ++-
>  xen/include/public/memory.h |    7 -------
>  xen/include/public/sysctl.h |    3 ++-
>  xen/include/xen/mm.h        |    2 +-
>  xen/include/xsm/dummy.h     |    6 ------
>  xen/include/xsm/xsm.h       |    1 -
>  xen/xsm/dummy.c             |    1 -
>  xen/xsm/flask/hooks.c       |    6 ------
>  16 files changed, 16 insertions(+), 73 deletions(-)
> 
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index bb71cca..3257e2a 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -873,15 +873,6 @@ int xc_domain_claim_pages(xc_interface *xch,
>          err = errno = 0;
>      return err;
>  }
> -unsigned long xc_domain_get_outstanding_pages(xc_interface *xch)
> -{
> -    long ret = do_memory_op(xch, XENMEM_get_outstanding_pages, NULL, 0);
> -
> -    /* Ignore it if the hypervisor does not support the call. */
> -    if (ret == -1 && errno == ENOSYS)
> -        ret = errno = 0;
> -    return ret;
> -}
> 
>  int xc_domain_populate_physmap(xc_interface *xch,
>                                 uint32_t domid,
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index c024af4..40ee8fc 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1182,8 +1182,6 @@ int xc_domain_claim_pages(xc_interface *xch,
>                                 uint32_t domid,
>                                 unsigned long nr_pages);
> 
> -unsigned long xc_domain_get_outstanding_pages(xc_interface *xch);
> -
>  int xc_domain_memory_exchange_pages(xc_interface *xch,
>                                      int domid,
>                                      unsigned long nr_in_extents,
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index bc91fd5..ee1fa9c 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3942,6 +3942,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo 
> *physinfo)
>      physinfo->total_pages = xcphysinfo.total_pages;
>      physinfo->free_pages = xcphysinfo.free_pages;
>      physinfo->scrub_pages = xcphysinfo.scrub_pages;
> +    physinfo->outstanding_pages = xcphysinfo.outstanding_pages;
>      l = xc_sharing_freed_pages(ctx->xch);
>      if (l == -ENOSYS) {
>          l = 0;
> @@ -4105,20 +4106,6 @@ libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int 
> *nr)
>      return ret;
>  }
> 
> -uint64_t libxl_get_claiminfo(libxl_ctx *ctx)
> -{
> -    long l;
> -
> -    l = xc_domain_get_outstanding_pages(ctx->xch);
> -    if (l < 0) {
> -        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_WARNING, l,
> -                            "xc_domain_get_outstanding_pages failed.");
> -        return ERROR_FAIL;
> -    }
> -    /* In pages */
> -    return l;
> -}
> -
>  const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
>  {
>      union {
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index ef96bce..0bc005e 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -624,7 +624,6 @@ int libxl_wait_for_free_memory(libxl_ctx *ctx, uint32_t 
> domid, uint32_t memory_k
>  /* wait for the memory target of a domain to be reached */
>  int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int 
> wait_secs);
> 
> -uint64_t libxl_get_claiminfo(libxl_ctx *ctx);
>  int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass);
>  int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, 
> libxl_console_type type);
>  /* libxl_primary_console_exec finds the domid and console number
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index ecf1f0b..8262cba 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -487,6 +487,7 @@ libxl_physinfo = Struct("physinfo", [
>      ("total_pages", uint64),
>      ("free_pages", uint64),
>      ("scrub_pages", uint64),
> +    ("outstanding_pages", uint64),
>      ("sharing_freed_pages", uint64),
>      ("sharing_used_frames", uint64),
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index ef7f81b..5259b2e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4576,21 +4576,11 @@ static void output_physinfo(void)
>      unsigned int i;
>      libxl_bitmap cpumap;
>      int n = 0;
> -    long claims = 0;
> 
>      if (libxl_get_physinfo(ctx, &info) != 0) {
>          fprintf(stderr, "libxl_physinfo failed.\n");
>          return;
>      }
> -    /*
> -     * Don't bother checking "claim_mode" as user might have turned it off
> -     * and we have outstanding claims.
> -     */
> -    if ((claims = libxl_get_claiminfo(ctx)) < 0){
> -        fprintf(stderr, "libxl_get_claiminfo failed. errno: %d (%s)\n",
> -                errno, strerror(errno));
> -        return;
> -    }
>      printf("nr_cpus                : %d\n", info.nr_cpus);
>      printf("max_cpu_id             : %d\n", info.max_cpu_id);
>      printf("nr_nodes               : %d\n", info.nr_nodes);
> @@ -4610,15 +4600,15 @@ static void output_physinfo(void)
>      if (vinfo) {
>          i = (1 << 20) / vinfo->pagesize;
>          printf("total_memory           : %"PRIu64"\n", info.total_pages / i);
> -        printf("free_memory            : %"PRIu64"\n", (info.free_pages - 
> claims) / i);
> +        printf("free_memory            : %"PRIu64"\n", (info.free_pages - 
> info.outstanding_pages) / i);
>          printf("sharing_freed_memory   : %"PRIu64"\n", 
> info.sharing_freed_pages / i);
>          printf("sharing_used_memory    : %"PRIu64"\n", 
> info.sharing_used_frames / i);
>      }
>      /*
>       * Only if enabled (claim_mode=1) or there are outstanding claims.
>       */
> -    if (libxl_defbool_val(claim_mode) || claims)
> -        printf("outstanding_claims     : %ld\n", claims / i);
> +    if (libxl_defbool_val(claim_mode) || info.outstanding_pages)
> +        printf("outstanding_claims     : %ld\n", info.outstanding_pages / i);
> 
>      if (!libxl_get_freecpus(ctx, &cpumap)) {
>          libxl_for_each_bit(i, cpumap)
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 3239d53..06a0d0a 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -737,14 +737,6 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
> 
>          break;
> 
> -    case XENMEM_get_outstanding_pages:
> -        rc = xsm_xenmem_get_outstanding_pages(XSM_PRIV);
> -
> -        if ( !rc )
> -            rc = get_outstanding_claims();
> -
> -        break;
> -
>      default:
>          rc = arch_memory_op(op, arg);
>          break;
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 203f77a..ede896c 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -380,9 +380,13 @@ out:
>      return ret;
>  }
> 
> -long get_outstanding_claims(void)
> +int get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages)
>  {
> -    return outstanding_claims;
> +    spin_lock(&heap_lock);
> +    *outstanding_pages = outstanding_claims;
> +    *free_pages =  avail_domheap_pages();
> +    spin_unlock(&heap_lock);
> +    return 0;
>  }
> 
>  static unsigned long init_node_heap(int node, unsigned long mfn,
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 31f9650..117e095 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -264,7 +264,8 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>          pi->max_node_id = MAX_NUMNODES-1;
>          pi->max_cpu_id = nr_cpu_ids - 1;
>          pi->total_pages = total_pages;
> -        pi->free_pages = avail_domheap_pages();
> +        /* Protected by lock */
> +        get_outstanding_claims(&pi->free_pages, &pi->outstanding_pages);
>          pi->scrub_pages = 0;
>          pi->cpu_khz = cpu_khz;
>          arch_do_physinfo(pi);
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 51d5254..7a26dee 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -459,13 +459,6 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>   * The zero value is appropiate.
>   */
> 
> -/*
> - * Get the number of pages currently claimed (but not yet "possessed")
> - * across all domains.  The caller must be privileged but otherwise
> - * the call never fails.
> - */
> -#define XENMEM_get_outstanding_pages        25
> -
>  #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
> 
>  #endif /* __XEN_PUBLIC_MEMORY_H__ */
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 03710d8..8437d31 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -34,7 +34,7 @@
>  #include "xen.h"
>  #include "domctl.h"
> 
> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000009
> +#define XEN_SYSCTL_INTERFACE_VERSION 0x0000000A
> 
>  /*
>   * Read console content from Xen buffer ring.
> @@ -101,6 +101,7 @@ struct xen_sysctl_physinfo {
>      uint64_aligned_t total_pages;
>      uint64_aligned_t free_pages;
>      uint64_aligned_t scrub_pages;
> +    uint64_aligned_t outstanding_pages;
>      uint32_t hw_cap[8];
> 
>      /* XEN_SYSCTL_PHYSCAP_??? */
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index efc45c7..e7c88a7 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -59,7 +59,7 @@ void destroy_xen_mappings(unsigned long v, unsigned long e);
>  /* Claim handling */
>  unsigned long domain_adjust_tot_pages(struct domain *d, long pages);
>  int domain_set_outstanding_pages(struct domain *d, unsigned long pages);
> -long get_outstanding_claims(void);
> +int get_outstanding_claims(uint64_t *free_pages, uint64_t 
> *outstanding_pages);
> 
>  /* Domain suballocator. These functions are *not* interrupt-safe.*/
>  void init_domheap_pages(paddr_t ps, paddr_t pe);
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index a872056..cc0a5a8 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -259,12 +259,6 @@ static XSM_INLINE int xsm_claim_pages(XSM_DEFAULT_ARG 
> struct domain *d)
>      return xsm_default_action(action, current->domain, d);
>  }
> 
> -static XSM_INLINE int xsm_xenmem_get_outstanding_pages(XSM_DEFAULT_VOID)
> -{
> -    XSM_ASSERT_ACTION(XSM_PRIV);
> -    return xsm_default_action(action, current->domain, NULL);
> -}
> -
>  static XSM_INLINE int xsm_evtchn_unbound(XSM_DEFAULT_ARG struct domain *d, 
> struct evtchn *chn,
>                                           domid_t id2)
>  {
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 58a4fbb..65150a3 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -93,7 +93,6 @@ struct xsm_operations {
>      int (*add_to_physmap) (struct domain *d1, struct domain *d2);
>      int (*remove_from_physmap) (struct domain *d1, struct domain *d2);
>      int (*claim_pages) (struct domain *d);
> -    int (*xenmem_get_outstanding_pages) (void);
> 
>      int (*console_io) (struct domain *d, int cmd);
> 
> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
> index 937761f..31e4f73 100644
> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c
> @@ -67,7 +67,6 @@ void xsm_fixup_ops (struct xsm_operations *ops)
>      set_to_dummy_if_null(ops, memory_stat_reservation);
>      set_to_dummy_if_null(ops, memory_pin_page);
>      set_to_dummy_if_null(ops, claim_pages);
> -    set_to_dummy_if_null(ops, xenmem_get_outstanding_pages);
> 
>      set_to_dummy_if_null(ops, console_io);
> 
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index bb10de3..8e93cdb 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -422,12 +422,6 @@ static int flask_claim_pages(struct domain *d)
>      return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SETCLAIM);
>  }
> 
> -static int flask_xenmem_get_outstanding_pages(void)
> -{
> -    return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN,
> -                                XEN__HEAP, NULL);
> -}
> -
>  static int flask_console_io(struct domain *d, int cmd)
>  {
>      u32 perm;
> --
> 1.7.7.6
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.