Re: [PATCH 10/11] tools/xl: Expose a "claim_on_node" setting in xl.cfg

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Mar 14, 2025 at 05:25:01PM +0000, Alejandro Vallejo wrote:
> Expose a setting to explicitly select a NUMA node for created domains
> 
> If the hypervisor can't reserve enough memory in the relevant NUMA node
> it fails the claim early. Also, disable automatic NUMA placement when
> this new option is enabled.

Overall it would be more helpful for users if soft-affinity was taken
into consideration when doing the claims, as that's the node where the
memory will ultimately be allocated?

Otherwise it's not very helpful IMO, as it's unlikely for the user to
know which NUMA the domain will get assigned to ahead of having
created it.

> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  tools/golang/xenlight/helpers.gen.go |  2 ++
>  tools/golang/xenlight/types.gen.go   |  1 +
>  tools/include/xenguest.h             |  7 +++++++
>  tools/libs/guest/xg_dom_core.c       |  1 +
>  tools/libs/guest/xg_dom_x86.c        | 18 +++++++++---------
>  tools/libs/light/libxl_dom.c         |  2 ++
>  tools/libs/light/libxl_types.idl     |  3 ++-
>  tools/xl/xl_parse.c                  | 11 +++++++++++
>  8 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
> index 90846ea8e882..9204020224d5 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -1108,6 +1108,7 @@ x.LlcColors[i] = uint32(v)
>  if err := x.ClaimMode.fromC(&xc.claim_mode);err != nil {
>  return fmt.Errorf("converting field ClaimMode: %v", err)
>  }
> +x.ClaimOnNode = uint32(xc.claim_on_node)
>  x.EventChannels = uint32(xc.event_channels)
>  x.Kernel = C.GoString(xc.kernel)
>  x.Cmdline = C.GoString(xc.cmdline)
> @@ -1472,6 +1473,7 @@ cLlcColors[i] = C.uint32_t(v)
>  if err := x.ClaimMode.toC(&xc.claim_mode); err != nil {
>  return fmt.Errorf("converting field ClaimMode: %v", err)
>  }
> +xc.claim_on_node = C.uint32_t(x.ClaimOnNode)
>  xc.event_channels = C.uint32_t(x.EventChannels)
>  if x.Kernel != "" {
>  xc.kernel = C.CString(x.Kernel)}
> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
> index e7667f1ce3a3..1c6319200411 100644
> --- a/tools/golang/xenlight/types.gen.go
> +++ b/tools/golang/xenlight/types.gen.go
> @@ -577,6 +577,7 @@ Irqs []uint32
>  Iomem []IomemRange
>  LlcColors []uint32
>  ClaimMode Defbool
> +ClaimOnNode uint32
>  EventChannels uint32
>  Kernel string
>  Cmdline string
> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
> index e01f494b772a..08aae1e24e4c 100644
> --- a/tools/include/xenguest.h
> +++ b/tools/include/xenguest.h
> @@ -185,6 +185,13 @@ struct xc_dom_image {
>      uint32_t guest_domid;
>      int claim_enabled; /* 0 by default, 1 enables it */
>  
> +    /*
> +     * Exact NUMA node on which to allocate memory from.
> +     *
> +     * XC_NUMA_NO_NODE by default.
> +     */
> +    unsigned int claim_on_node;
> +
>      int xen_version;
>      xen_capabilities_info_t xen_caps;
>  
> diff --git a/tools/libs/guest/xg_dom_core.c b/tools/libs/guest/xg_dom_core.c
> index 595b0a667c03..152ec8ea6591 100644
> --- a/tools/libs/guest/xg_dom_core.c
> +++ b/tools/libs/guest/xg_dom_core.c
> @@ -775,6 +775,7 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
>      dom->parms->p2m_base = UNSET_ADDR;
>  
>      dom->flags = SIF_VIRT_P2M_4TOOLS;
> +    dom->claim_on_node = XC_NUMA_NO_NODE;
>  
>      dom->alloc_malloc += sizeof(*dom);
>      return dom;
> diff --git a/tools/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c
> index ac05106a8c1c..4fe816b9bcb4 100644
> --- a/tools/libs/guest/xg_dom_x86.c
> +++ b/tools/libs/guest/xg_dom_x86.c
> @@ -1199,7 +1199,7 @@ static int meminit_pv(struct xc_dom_image *dom)
>      if ( dom->claim_enabled )
>      {
>          rc = xc_domain_claim_pages(dom->xch, dom->guest_domid,
> -                                   XC_NUMA_NO_NODE,
> +                                   dom->claim_on_node,
>                                     dom->total_pages);
>          if ( rc )
>              return rc;
> @@ -1209,9 +1209,10 @@ static int meminit_pv(struct xc_dom_image *dom)
>       * that this is a valid state if libxl doesn't provide any
>       * vNUMA information.
>       *
> -     * The dummy values make libxc allocate all pages from
> -     * arbitrary physical nodes. This is the expected behaviour if
> -     * no vNUMA configuration is provided to libxc.
> +     * If there's an outstanding claim on a node, memory is allocated from that
> +     * node. Otherwise the dummy values make libxc allocate all pages from
> +     * arbitrary physical nodes. This is the expected behaviour if no vNUMA
> +     * configuration is provided to libxc and.

Stray 'and' or truncated sentence?

>       *
>       * Note that the following hunk is just for the convenience of
>       * allocation code. No defaulting happens in libxc.
> @@ -1227,7 +1228,7 @@ static int meminit_pv(struct xc_dom_image *dom)
>  
>          nr_vnodes = 1;
>          vnode_to_pnode = dummy_vnode_to_pnode;
> -        vnode_to_pnode[0] = XC_NUMA_NO_NODE;
> +        vnode_to_pnode[0] = dom->claim_on_node;
>      }
>      else
>      {
> @@ -1357,7 +1358,6 @@ static int meminit_hvm(struct xc_dom_image *dom)
>      unsigned long stat_normal_pages = 0, stat_2mb_pages = 0,
>          stat_1gb_pages = 0;
>      unsigned int memflags = 0;
> -    int claim_enabled = dom->claim_enabled;
>      uint64_t total_pages;
>      xen_vmemrange_t dummy_vmemrange[2];
>      unsigned int dummy_vnode_to_pnode[1];
> @@ -1397,7 +1397,7 @@ static int meminit_hvm(struct xc_dom_image *dom)
>              nr_vmemranges++;
>          }
>  
> -        dummy_vnode_to_pnode[0] = XC_NUMA_NO_NODE;
> +        dummy_vnode_to_pnode[0] = dom->claim_on_node;
>          nr_vnodes = 1;
>          vmemranges = dummy_vmemrange;
>          vnode_to_pnode = dummy_vnode_to_pnode;
> @@ -1443,8 +1443,8 @@ static int meminit_hvm(struct xc_dom_image *dom)
>       * actually allocates memory for the guest. Claiming after memory has been
>       * allocated is pointless.
>       */
> -    if ( claim_enabled ) {
> -        rc = xc_domain_claim_pages(xch, domid, XC_NUMA_NO_NODE,
> +    if ( dom->claim_enabled ) {
> +        rc = xc_domain_claim_pages(xch, domid, dom->claim_on_node,
>                                     target_pages - dom->vga_hole_size);
>          if ( rc != 0 )
>          {
> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index 94fef374014e..1a5500702239 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -663,6 +663,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>      dom->xenstore_evtchn = state->store_port;
>      dom->xenstore_domid = state->store_domid;
>      dom->claim_enabled = libxl_defbool_val(info->claim_mode);
> +    dom->claim_on_node = info->claim_on_node;
>      dom->max_vcpus = info->max_vcpus;
>  
>      if (info->num_vnuma_nodes != 0) {
> @@ -1090,6 +1091,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
>      mem_size = (uint64_t)(info->max_memkb - info->video_memkb) << 10;
>      dom->target_pages = (uint64_t)(info->target_memkb - info->video_memkb) >> 2;
>      dom->claim_enabled = libxl_defbool_val(info->claim_mode);
> +    dom->claim_on_node = info->claim_on_node;
>      if (info->u.hvm.mmio_hole_memkb) {
>          uint64_t max_ram_below_4g = (1ULL << 32) -
>              (info->u.hvm.mmio_hole_memkb << 10);
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index bd4b8721ff19..a252b36c2b5d 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -617,7 +617,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("irqs",             Array(uint32, "num_irqs")),
>      ("iomem",            Array(libxl_iomem_range, "num_iomem")),
>      ("llc_colors",       Array(uint32, "num_llc_colors")),
> -    ("claim_mode",	     libxl_defbool),
> +    ("claim_mode",       libxl_defbool),
> +    ("claim_on_node",    uint32),
>      ("event_channels",   uint32),
>      ("kernel",           string),
>      ("cmdline",          string),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 089a88935aff..5880a032e6d6 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1558,6 +1558,17 @@ void parse_config_data(const char *config_source,
>          exit(1);
>  
>      libxl_defbool_set(&b_info->claim_mode, claim_mode);
> +    e = xlu_cfg_get_bounded_long (config, "claim_on_node", 0,

You seem to introduce a new xl.cfg option, but there are no man page
changes as part of the patch?

> +                                  254, &l, 1);
> +    if (e == ESRCH) /* not specified */
> +        b_info->claim_on_node = ~0U;
> +    else if (!e) {
> +        libxl_defbool_set(&b_info->numa_placement, false);
> +        libxl_defbool_set(&b_info->claim_mode, true);
> +        b_info->claim_on_node = l;
> +    }

This setting of defaults should better be done in
libxl__domain_build_info_setdefault() IMO.

Thanks, Roger.