Series short description

as well as both error handling and logging in libxl_cpu_bitmap_alloc and libxl_node_bitmap_alloc.

Now libxl_get_max_{cpus,nodes} either return a positive number, or a libxl error code. Thanks to that, it is possible to fix loggig for the two bitmap allocation functions, which now happens _inside_ the functions themselves, and report what happens more accurately.

Signed-off-by Dario Faggioli <[email protected]>

Quoted 3 lines · Dario Faggioli

as well as both error handling and logging in libxl_cpu_bitmap_alloc and libxl_node_bitmap_alloc.

Quoted 6 lines

Now libxl_get_max_{cpus,nodes} either return a positive number, or a libxl error code. Thanks to that, it is possible to fix loggig for the two bitmap allocation functions, which now happens _inside_ the functions themselves, and report what happens more accurately.

Signed-off-by Dario Faggioli <[email protected]>

Reviewed-by George Dunlap <[email protected]>

Quoted 8 lines

tools/libxl/libxl_utils.c +0 -0

index 682f874..2a51c9c 100644
@@ -645,6 +645,46 @@ char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *bitmap)
      return q;
  }
 

tools/libxl/libxl_utils.c char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *bitmap) · line 650

Quoted 3 lines

tools/libxl/libxl_utils.c +3 -0

+inline int libxl_cpu_bitmap_alloc(libxl_ctx *ctx,
+                                  libxl_bitmap *cpumap,
+                                  int max_cpus)

Stray 'inline'. :-)

if a domain's NUMA node-affinity (which is what controls memory allocations) is provided by the user/toolstack, it just is not touched. However, if the user does not say anything, leaving it all to Xen, let's compute it in the following way:

1. cpupool's cpus & hard-affinity & soft-affinity
2. if (1) is empty: cpupool's cpus & hard-affinity

This guarantees memory to be allocated from the narrowest possible set of NUMA nodes, ad makes it relatively easy to set up NUMA-aware scheduling on top of soft affinity.

Signed-off-by Dario Faggioli <[email protected]>

---
Changes from v2:
* the loop computing the mask is now only executed when

    it really is useful, as suggested during review;
  * the loop, and all the cpumask handling is optimized,
    in a way similar to what was suggested during review.
---
  xen/common/domain.c |   62 +++++++++++++++++++++++++++++++++------------------
  1 file changed, 40 insertions(+), 22 deletions(-)

xen/common/domain.c +40 -22

index d6ac4d1..721678a 100644
@@ -353,17 +353,17 @@ struct domain *domain_create(
 
  void domain_update_node_affinity(struct domain *d)
  {
-    cpumask_var_t cpumask;
-    cpumask_var_t online_affinity;
+    cpumask_var_t dom_cpumask, dom_cpumask_soft;
+    cpumask_t *dom_affinity;
      const cpumask_t *online;
      struct vcpu *v;
-    unsigned int node;
+    unsigned int cpu;
 
-    if ( !zalloc_cpumask_var(&cpumask) )
+    if ( !zalloc_cpumask_var(&dom_cpumask) )
          return;
-    if ( !alloc_cpumask_var(&online_affinity) )
+    if ( !zalloc_cpumask_var(&dom_cpumask_soft) )
      {
-        free_cpumask_var(cpumask);
+        free_cpumask_var(dom_cpumask);
          return;
      }
 

@@ -371,31 +371,49 @@ void domain_update_node_affinity(struct domain *d)
 
      spin_lock(&d->node_affinity_lock);
 
-    for_each_vcpu ( d, v )
-    {
-        cpumask_and(online_affinity, v->cpu_hard_affinity, online);
-        cpumask_or(cpumask, cpumask, online_affinity);
-    }
-
      /*
-     * If d->auto_node_affinity is true, the domain's node-affinity mask
-     * (d->node_affinity) is automaically computed from all the domain's
-     * vcpus' vcpu-affinity masks (the union of which we have just built
-     * above in cpumask). OTOH, if d->auto_node_affinity is false, we
-     * must leave the node-affinity of the domain alone.
+     * If d->auto_node_affinity is true, let's compute the domain's
+     * node-affinity and update d->node_affinity accordingly. if false,
+     * just leave d->auto_node_affinity alone.
       */
      if ( d->auto_node_affinity )
      {
+        /*
+         * We want the narrowest possible set of pcpus (to get the narowest
+         * possible set of nodes). What we need is the cpumask of where the
+         * domain can run (the union of the hard affinity of all its vcpus),
+         * and the full mask of where it would prefer to run (the union of
+         * the soft affinity of all its various vcpus). Let's build them.
+         */
+        cpumask_clear(dom_cpumask);
+        cpumask_clear(dom_cpumask_soft);
+        for_each_vcpu ( d, v )
+        {
+            cpumask_or(dom_cpumask, dom_cpumask, v->cpu_hard_affinity);
+            cpumask_or(dom_cpumask_soft, dom_cpumask_soft,
+                       v->cpu_soft_affinity);
+        }
+        /* Filter out non-online cpus */
+        cpumask_and(dom_cpumask, dom_cpumask, online);
+        /* And compute the intersection between hard, online and soft */
+        cpumask_and(dom_cpumask_soft, dom_cpumask_soft, dom_cpumask);
+
+        /*
+         * If not empty, the intersection of hard, soft and online is the
+         * narrowest set we want. If empty, we fall back to hard&online.
+         */
+        dom_affinity = cpumask_empty(dom_cpumask_soft) ?
+                           dom_cpumask : dom_cpumask_soft;
+
          nodes_clear(d->node_affinity);
-        for_each_online_node ( node )
-            if ( cpumask_intersects(&node_to_cpumask(node), cpumask) )
-                node_set(node, d->node_affinity);
+        for_each_cpu( cpu, dom_affinity )
+            node_set(cpu_to_node(cpu), d->node_affinity);
      }
 
      spin_unlock(&d->node_affinity_lock);
 
-    free_cpumask_var(online_affinity);
-    free_cpumask_var(cpumask);
+    free_cpumask_var(dom_cpumask_soft);
+    free_cpumask_var(dom_cpumask);
  }
 
 

Quoted 1 line

Quoted 1 line · Dario Faggioli

if a domain's NUMA node-affinity (which is what controls memory allocations) is provided by the user/toolstack, it just is not touched. However, if the user does not say anything, leaving it all to Xen, let's compute it in the following way:

1. cpupool's cpus & hard-affinity & soft-affinity
2. if (1) is empty: cpupool's cpus & hard-affinity

Quoted 7 lines

Quoted 1 line

Quoted 1 line · Dario Faggioli

if a domain's NUMA node-affinity (which is what controls memory allocations) is provided by the user/toolstack, it just is not touched. However, if the user does not say anything, leaving it all to Xen, let's compute it in the following way:

Quoted 2 lines

1. cpupool's cpus & hard-affinity & soft-affinity
2. if (1) is empty: cpupool's cpus & hard-affinity

Is this really guaranteed to always be non-empty? At least an ASSERT() to that effect would be nice, as it's not immediately obvious.

xen/common/domain.c struct domain *domain_create( · line 364

Quoted 5 lines

xen/common/domain.c +2 -2

-    if ( !zalloc_cpumask_var(&cpumask) )
+    if ( !zalloc_cpumask_var(&dom_cpumask) )
         return;
-    if ( !alloc_cpumask_var(&online_affinity) )
+    if ( !zalloc_cpumask_var(&dom_cpumask_soft) )

So you use zalloc_cpumask_var() here ...

xen/common/domain.c void domain_update_node_affinity(struct domain *d) · line 389

Quoted 11 lines

     if ( d->auto_node_affinity )
     {

xen/common/domain.c +9 -0

+        /*
+         * We want the narrowest possible set of pcpus (to get the narowest
+         * possible set of nodes). What we need is the cpumask of where the
+         * domain can run (the union of the hard affinity of all its vcpus),
+         * and the full mask of where it would prefer to run (the union of
+         * the soft affinity of all its various vcpus). Let's build them.
+         */
+        cpumask_clear(dom_cpumask);
+        cpumask_clear(dom_cpumask_soft);

... and then clear the masks explicitly here?

by adding a flag for the caller to specify which one he cares about.

Add also another cpumap there. This way, in case of DOMCTL_setvcpuaffinity, Xen can return back to the caller the "effective affinity" of the vcpu. We call the effective affinity the intersection between cpupool's cpus, the (new?) hard affinity and the (new?) soft affinity.

The purpose of this is allowing the toolstack to figure out whether or not the requested change produced sensible results, when combined with the other settings that are already in place.

Signed-off-by Dario Faggioli <[email protected]>

---
Changes from v2:
* in DOMCTL_[sg]etvcpuaffinity, flag is really a flag now, i.e., we accept request for setting and getting: (1) only hard affinity; (2) only soft affinity; (3) both; as suggested during review.

---
  tools/libxc/xc_domain.c     |    4 ++-
  xen/arch/x86/traps.c        |    4 ++-
  xen/common/domctl.c         |   54 ++++++++++++++++++++++++++++++++++++++++---
  xen/common/schedule.c       |   35 +++++++++++++++++++---------
  xen/common/wait.c           |    6 ++---
  xen/include/public/domctl.h |   15 ++++++++++--
  xen/include/xen/sched.h     |    3 ++
  7 files changed, 97 insertions(+), 24 deletions(-)

tools/libxc/xc_domain.c +3 -1

index 1ccafc5..f9ae4bf 100644
@@ -215,7 +215,9 @@ int xc_vcpu_setaffinity(xc_interface *xch,
 
      domctl.cmd = XEN_DOMCTL_setvcpuaffinity;
      domctl.domain = (domid_t)domid;
-    domctl.u.vcpuaffinity.vcpu    = vcpu;
+    domctl.u.vcpuaffinity.vcpu = vcpu;
+    /* Soft affinity is there, but not used anywhere for now, so... */
+    domctl.u.vcpuaffinity.flags = XEN_VCPUAFFINITY_HARD;
 
      memcpy(local, cpumap, cpusize);
 

xen/arch/x86/traps.c +2 -2

index 4279cad..196ff68 100644
@@ -3093,7 +3093,7 @@ static void nmi_mce_softirq(void)
           * Make sure to wakeup the vcpu on the
           * specified processor.
           */
-        vcpu_set_affinity(st->vcpu, cpumask_of(st->processor));
+        vcpu_set_hard_affinity(st->vcpu, cpumask_of(st->processor));
 
          /* Affinity is restored in the iret hypercall. */
      }

@@ -3122,7 +3122,7 @@ void async_exception_cleanup(struct vcpu *curr)
      if ( !cpumask_empty(curr->cpu_hard_affinity_tmp) &&
           !cpumask_equal(curr->cpu_hard_affinity_tmp, curr->cpu_hard_affinity) )
      {
-        vcpu_set_affinity(curr, curr->cpu_hard_affinity_tmp);
+        vcpu_set_hard_affinity(curr, curr->cpu_hard_affinity_tmp);
          cpumask_clear(curr->cpu_hard_affinity_tmp);
      }
 

xen/common/domctl.c +50 -4

index 5e0ac5c..84be0d6 100644
@@ -617,19 +617,65 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
          if ( op->cmd == XEN_DOMCTL_setvcpuaffinity )
          {
              cpumask_var_t new_affinity;
+            cpumask_t *online;
 
              ret = xenctl_bitmap_to_cpumask(
                  &new_affinity, &op->u.vcpuaffinity.cpumap);
-            if ( !ret )
+            if ( ret )
+                break;
+
+            ret = -EINVAL;
+            if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_HARD )
+                ret = vcpu_set_hard_affinity(v, new_affinity);
+            if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_SOFT )
+                ret = vcpu_set_soft_affinity(v, new_affinity);
+
+            if ( ret )
+                goto setvcpuaffinity_out;
+
+            /*
+             * Report back to the caller what the "effective affinity", that
+             * is the intersection of cpupool's pcpus, the (new?) hard
+             * affinity and the (new?) soft-affinity.
+             */
+            if ( !guest_handle_is_null(op->u.vcpuaffinity.eff_cpumap.bitmap) )
              {
-                ret = vcpu_set_affinity(v, new_affinity);
-                free_cpumask_var(new_affinity);
+                online = cpupool_online_cpumask(v->domain->cpupool);
+                cpumask_and(new_affinity, online, v->cpu_hard_affinity);
+                if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_SOFT)
+                    cpumask_and(new_affinity, new_affinity,
+                                v->cpu_soft_affinity);
+
+                ret = cpumask_to_xenctl_bitmap(
+                    &op->u.vcpuaffinity.eff_cpumap, new_affinity);
              }
+
+ setvcpuaffinity_out:
+            free_cpumask_var(new_affinity);
          }
          else
          {
+            cpumask_var_t affinity;
+
+            /*
+             * If the caller asks for both _HARD and _SOFT, what we return
+             * is the intersection of hard and soft affinity for the vcpu.
+             */
+            if ( !alloc_cpumask_var(&affinity) ) {
+                ret = -EFAULT;
+                break;
+            }
+            cpumask_setall(affinity);
+
+            if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_HARD )
+                cpumask_copy(affinity, v->cpu_hard_affinity);
+            if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_SOFT )
+                cpumask_and(affinity, affinity, v->cpu_soft_affinity);
+
              ret = cpumask_to_xenctl_bitmap(
-                &op->u.vcpuaffinity.cpumap, v->cpu_hard_affinity);
+                &op->u.vcpuaffinity.cpumap, affinity);
+
+            free_cpumask_var(affinity);
          }
      }
      break;

xen/common/schedule.c +24 -11

index c9ae521..6c53287 100644
@@ -654,22 +654,14 @@ void sched_set_node_affinity(struct domain *d, nodemask_t *mask)
      SCHED_OP(DOM2OP(d), set_node_affinity, d, mask);
  }
 
-int vcpu_set_affinity(struct vcpu *v, const cpumask_t *affinity)
+static int vcpu_set_affinity(
+    struct vcpu *v, const cpumask_t *affinity, cpumask_t **which)
  {
-    cpumask_t online_affinity;
-    cpumask_t *online;
      spinlock_t *lock;
 
-    if ( v->domain->is_pinned )
-        return -EINVAL;
-    online = VCPU2ONLINE(v);
-    cpumask_and(&online_affinity, affinity, online);
-    if ( cpumask_empty(&online_affinity) )
-        return -EINVAL;
-
      lock = vcpu_schedule_lock_irq(v);
 
-    cpumask_copy(v->cpu_hard_affinity, affinity);
+    cpumask_copy(*which, affinity);
 
      /* Always ask the scheduler to re-evaluate placement
       * when changing the affinity */

@@ -688,6 +680,27 @@ int vcpu_set_affinity(struct vcpu *v, const cpumask_t *affinity)
      return 0;
  }
 
+int vcpu_set_hard_affinity(struct vcpu *v, const cpumask_t *affinity)
+{
+    cpumask_t online_affinity;
+    cpumask_t *online;
+
+    if ( v->domain->is_pinned )
+        return -EINVAL;
+
+    online = VCPU2ONLINE(v);
+    cpumask_and(&online_affinity, affinity, online);
+    if ( cpumask_empty(&online_affinity) )
+        return -EINVAL;
+
+    return vcpu_set_affinity(v, affinity, &v->cpu_hard_affinity);
+}
+
+int vcpu_set_soft_affinity(struct vcpu *v, const cpumask_t *affinity)
+{
+    return vcpu_set_affinity(v, affinity, &v->cpu_soft_affinity);
+}
+
  /* Block the currently-executing domain until a pertinent event occurs. */
  void vcpu_block(void)
  {

xen/common/wait.c +3 -3

index 3f6ff41..1f6b597 100644
@@ -135,7 +135,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
      /* Save current VCPU affinity; force wakeup on *this* CPU only. */
      wqv->wakeup_cpu = smp_processor_id();
      cpumask_copy(&wqv->saved_affinity, curr->cpu_hard_affinity);
-    if ( vcpu_set_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
+    if ( vcpu_set_hard_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
      {
          gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
          domain_crash_synchronous();

@@ -166,7 +166,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
  static void __finish_wait(struct waitqueue_vcpu *wqv)
  {
      wqv->esp = NULL;
-    (void)vcpu_set_affinity(current, &wqv->saved_affinity);
+    (void)vcpu_set_hard_affinity(current, &wqv->saved_affinity);
  }
 
  void check_wakeup_from_wait(void)

@@ -184,7 +184,7 @@ void check_wakeup_from_wait(void)
          /* Re-set VCPU affinity and re-enter the scheduler. */
          struct vcpu *curr = current;
          cpumask_copy(&wqv->saved_affinity, curr->cpu_hard_affinity);
-        if ( vcpu_set_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
+        if ( vcpu_set_hard_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
          {
              gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
              domain_crash_synchronous();

xen/include/public/domctl.h +13 -2

index 01a3652..4f71450 100644
@@ -300,8 +300,19 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_nodeaffinity_t);
  /* XEN_DOMCTL_setvcpuaffinity */
  /* XEN_DOMCTL_getvcpuaffinity */
  struct xen_domctl_vcpuaffinity {
-    uint32_t  vcpu;              /* IN */
-    struct xenctl_bitmap cpumap; /* IN/OUT */
+    /* IN variables. */
+    uint32_t  vcpu;
+ /* Set/get the hard affinity for vcpu */
+#define _XEN_VCPUAFFINITY_HARD  0
+#define XEN_VCPUAFFINITY_HARD   (1U<<_XEN_VCPUAFFINITY_HARD)
+ /* Set/get the soft affinity for vcpu */
+#define _XEN_VCPUAFFINITY_SOFT  1
+#define XEN_VCPUAFFINITY_SOFT   (1U<<_XEN_VCPUAFFINITY_SOFT)
+    uint32_t flags;
+    /* IN/OUT variables. */
+    struct xenctl_bitmap cpumap;
+    /* OUT variables. */
+    struct xenctl_bitmap eff_cpumap;
  };
  typedef struct xen_domctl_vcpuaffinity xen_domctl_vcpuaffinity_t;
  DEFINE_XEN_GUEST_HANDLE(xen_domctl_vcpuaffinity_t);

xen/include/xen/sched.h +2 -1

index 3575312..0f728b3 100644
@@ -755,7 +755,8 @@ void scheduler_free(struct scheduler *sched);
  int schedule_cpu_switch(unsigned int cpu, struct cpupool *c);
  void vcpu_force_reschedule(struct vcpu *v);
  int cpu_disable_scheduler(unsigned int cpu);
-int vcpu_set_affinity(struct vcpu *v, const cpumask_t *affinity);
+int vcpu_set_hard_affinity(struct vcpu *v, const cpumask_t *affinity);
+int vcpu_set_soft_affinity(struct vcpu *v, const cpumask_t *affinity);
  void restore_vcpu_affinity(struct domain *d);
 
  void vcpu_runstate_get(struct vcpu *v, struct vcpu_runstate_info *runstate);

Quoted 1 line

Quoted 1 line · Dario Faggioli

xen/common/domctl.c +1 -0

@@ -617,19 +617,65 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
 u_domctl)
         if ( op->cmd == XEN_DOMCTL_setvcpuaffinity )
         {
             cpumask_var_t new_affinity;
+            cpumask_t *online;
 
             ret = xenctl_bitmap_to_cpumask(

xen/common/domctl.c +7 -0

+
+            /*
+             * Report back to the caller what the "effective affinity", that
+             * is the intersection of cpupool's pcpus, the (new?) hard
+             * affinity and the (new?) soft-affinity.
+             */
+            if ( !guest_handle_is_null(op->u.vcpuaffinity.eff_cpumap.bitmap) )
             {

Quoted 10 lines

Quoted 1 line

Quoted 1 line · Dario Faggioli

xen/common/domctl.c +1 -0

@@ -617,19 +617,65 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
 u_domctl)
         if ( op->cmd == XEN_DOMCTL_setvcpuaffinity )
         {
             cpumask_var_t new_affinity;
+            cpumask_t *online;
 

xen/common/domctl.c long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) · line 631

Quoted 11 lines

xen/common/domctl.c +8 -1

             ret = xenctl_bitmap_to_cpumask(
                 &new_affinity, &op->u.vcpuaffinity.cpumap);
-            if ( !ret )
+            if ( ret )
+                break;
+
+            ret = -EINVAL;
+            if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_HARD )
+                ret = vcpu_set_hard_affinity(v, new_affinity);
+            if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_SOFT )
+                ret = vcpu_set_soft_affinity(v, new_affinity);

You're discarding an eventual error indicator from vcpu_set_hard_affinity() here.

xen/common/domctl.c long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) · line 634

Quoted 3 lines

xen/common/domctl.c +3 -0

+
+            if ( ret )
+                goto setvcpuaffinity_out;

Considering that you're going to return an error here, the caller may expect that the call did nothing, even if vcpu_set_hard_affinity() succeeded and vcpu_set_soft_affinity() failed. I know this is ugly to handle...

Quoted 13 lines

xen/common/domctl.c +10 -2

+
+            /*
+             * Report back to the caller what the "effective affinity", that
+             * is the intersection of cpupool's pcpus, the (new?) hard
+             * affinity and the (new?) soft-affinity.
+             */
+            if ( !guest_handle_is_null(op->u.vcpuaffinity.eff_cpumap.bitmap) )
             {
-                ret = vcpu_set_affinity(v, new_affinity);
-                free_cpumask_var(new_affinity);
+                online = cpupool_online_cpumask(v->domain->cpupool);
+                cpumask_and(new_affinity, online, v->cpu_hard_affinity);
+                if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_SOFT)

xen/common/domctl.c long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) · line 650

Quoted 5 lines

xen/common/domctl.c +5 -0

+                    cpumask_and(new_affinity, new_affinity,
+                                v->cpu_soft_affinity);
+
+                ret = cpumask_to_xenctl_bitmap(
+                    &op->u.vcpuaffinity.eff_cpumap, new_affinity);

Considering that you have two bitmaps available from the caller, can't you just return both when both flags are set?

Quoted 1 line

Quoted 1 line · Dario Faggioli

Quoted 10 lines · Jan Beulich

Quoted 9 lines

Quoted 1 line

Quoted 1 line · Dario Faggioli

xen/common/domctl.c +8 -0

+                online = cpupool_online_cpumask(v->domain->cpupool);
+                cpumask_and(new_affinity, online, v->cpu_hard_affinity);
+                if ( op->u.vcpuaffinity.flags & XEN_VCPUAFFINITY_SOFT)
+                    cpumask_and(new_affinity, new_affinity,
+                                v->cpu_soft_affinity);
+
+                ret = cpumask_to_xenctl_bitmap(
+                    &op->u.vcpuaffinity.eff_cpumap, new_affinity);

Quoted 3 lines

Considering that you have two bitmaps available from the caller, can't you just return both when both flags are set?

Well, it's true that there are two cpumaps, but only one is meant to be an output parameter.

Also, I think this is more useful like this, i.e., returning either:
- hard-affinity&online
- hard-affinity&soft-affinity&online,

As it can be seen in the libxl patch (in the new series).

Therefore, I kept this interface as it was here, also considering that:
- it's pretty late to re-re-redesign;
- neither this nor the xc one are stable interfaces, so we can come back and revisit this later, if we want to.

Do you think this could be acceptable?

Quoted 7 lines

Quoted 1 line

Quoted 1 line · Dario Faggioli

Therefore, I kept this interface as it was here, also considering that:
- it's pretty late to re-re-redesign;
- neither this nor the xc one are stable interfaces, so we can come back and revisit this later, if we want to.

Do you think this could be acceptable?

I wouldn't veto it, but I also dislike reduced flexibility when more flexibility is obviously achievable without much effort.

Quoted 1 line

Quoted 1 line · Dario Faggioli

Quoted 11 lines · Jan Beulich

Quoted 7 lines

Quoted 1 line

Quoted 1 line · Dario Faggioli

Therefore, I kept this interface as it was here, also considering that:
- it's pretty late to re-re-redesign;
- neither this nor the xc one are stable interfaces, so we can come back and revisit this later, if we want to.

Do you think this could be acceptable?

I wouldn't veto it, but I also dislike reduced flexibility when more flexibility is obviously achievable without much effort.

Ok, understood. Ok, I'm up for changing this then. So, let m ask a few questions, just to make sure ot get it right this time! ;-P

You are saying the interface should look as follows:

int xc_vcpu_setaffinity(xc_interface *xch,

                         uint32_t domid,
                         int vcpu,
                         xc_cpumap_t cpumap_soft,
                         xc_cpumap_t cpumap_hard,
                         uint32_t flags);

Where both cpumap_soft and cpumap_hard are IN/OUT parameters and, as far as OUT is concerned:
- cpumap_hard will contain hard-affinity&online
- cpumap_soft will contain what?
(a) soft-affinity?
(b) soft-affinity&online
(c) soft-affinity&hard-affinity&online?

I would make it (c), as (a) is what xc_vcpu_setaffinity() retrieves, while the '&online' part is particularly hard to get from other existing calls (either at libxc and libxl level). Among (b) and (c), (c) is what would be most useful for an high level caller (like libxl) to have it ready, but it's also true that, if I give (b) to it, it could in theory reconstruct (c) by himself (with the vice-versa not being true).

Thoughts?

Also, should I return something in either *only* of the maps when the corresponding flag is set (and of course fill both when both flags are)?

Quoted 24 lines

Quoted 1 line

Quoted 1 line · Dario Faggioli

Quoted 10 lines · Jan Beulich

Quoted 7 lines

Quoted 1 line

Quoted 1 line · Dario Faggioli

Therefore, I kept this interface as it was here, also considering that:
- it's pretty late to re-re-redesign;
- neither this nor the xc one are stable interfaces, so we can come back and revisit this later, if we want to.

Do you think this could be acceptable?

I wouldn't veto it, but I also dislike reduced flexibility when more flexibility is obviously achievable without much effort.

Ok, understood. Ok, I'm up for changing this then. So, let m ask a few questions, just to make sure ot get it right this time! ;-P

You are saying the interface should look as follows:

int xc_vcpu_setaffinity(xc_interface *xch,

                          uint32_t domid,
                          int vcpu,
                          xc_cpumap_t cpumap_soft,
                          xc_cpumap_t cpumap_hard,
                          uint32_t flags);

Quoted 7 lines

Where both cpumap_soft and cpumap_hard are IN/OUT parameters and, as far as OUT is concerned:
- cpumap_hard will contain hard-affinity&online
- cpumap_soft will contain what?
(a) soft-affinity?
(b) soft-affinity&online
(c) soft-affinity&hard-affinity&online?

(c) seems the best fit - after all it should represent what the hypervisor will effectively use.

by using the new flag introduced in the parameters of the DOMCTL_{get,set}_vcpuaffinity hypercall.

This happens by adding a new parameter (flags) to xc_vcpu_setaffinity() and xc_vcpu_getaffinity(), so that the caller can decide to set either the soft or hard affinity, or even both.

In case of setting both hard and soft, they are set to the same cpumap. xc_get_setaffinity() also takes another new param, for reporting back to the caller what the actual affinity the scheduler uses will be after a successful call.
In case of asking to get both hard and soft, what the caller gets is the intersection between them.

In-tree callers are also fixed to cope with the new interface.

Signed-off-by Dario Faggioli <[email protected]>

---
Changes from v2:
* better cleanup logic in _vcpu_setaffinity() (regarding xc_hypercall_buffer_{alloc,free}()), as suggested during review;
* make it more evident that DOMCTL_setvcpuaffinity has an out parameter, by calling ecpumap_out, and improving the comment wrt that;
* change the interface and have xc_vcpu_[sg]etaffinity() so that they take the new parameters (flags and ecpumap_out) and fix the in tree callers.

---
  tools/libxc/xc_domain.c             |   47 +++++++++++++++++++++--------------
  tools/libxc/xenctrl.h               |   44 ++++++++++++++++++++++++++++++++-
  tools/libxl/libxl.c                 |    7 ++++-
  tools/ocaml/libs/xc/xenctrl_stubs.c |    8 ++++--
  tools/python/xen/lowlevel/xc/xc.c   |    6 +++-
  5 files changed, 86 insertions(+), 26 deletions(-)

tools/libxc/xc_domain.c +3 -1

index f9ae4bf..bddf4e0 100644
@@ -192,44 +192,52 @@ int xc_domain_node_getaffinity(xc_interface *xch,
  int xc_vcpu_setaffinity(xc_interface *xch,
                          uint32_t domid,
                          int vcpu,
-                        xc_cpumap_t cpumap)
+                        xc_cpumap_t cpumap,
+                        uint32_t flags,
+                        xc_cpumap_t ecpumap_out)
  {

Quoted 15 lines · Dario Faggioli

by using the new flag introduced in the parameters of the DOMCTL_{get,set}_vcpuaffinity hypercall.

This happens by adding a new parameter (flags) to xc_vcpu_setaffinity() and xc_vcpu_getaffinity(), so that the caller can decide to set either the soft or hard affinity, or even both.

In case of setting both hard and soft, they are set to the same cpumap. xc_get_setaffinity() also takes another new param, for reporting back to the caller what the actual affinity the scheduler uses will be after a successful call.
In case of asking to get both hard and soft, what the caller gets is the intersection between them.

Quoted 3 lines

In-tree callers are also fixed to cope with the new interface.

Signed-off-by Dario Faggioli <[email protected]>

Acked-by George Dunlap <[email protected]>

But...

Quoted 39 lines

---
Changes from v2:
* better cleanup logic in _vcpu_setaffinity() (regarding xc_hypercall_buffer_{alloc,free}()), as suggested during review;
* make it more evident that DOMCTL_setvcpuaffinity has an out parameter, by calling ecpumap_out, and improving the comment wrt that;
* change the interface and have xc_vcpu_[sg]etaffinity() so that they take the new parameters (flags and ecpumap_out) and fix the in tree callers.

---
  tools/libxc/xc_domain.c             |   47 +++++++++++++++++++++--------------
  tools/libxc/xenctrl.h               |   44 ++++++++++++++++++++++++++++++++-
  tools/libxl/libxl.c                 |    7 ++++-
  tools/ocaml/libs/xc/xenctrl_stubs.c |    8 ++++--
  tools/python/xen/lowlevel/xc/xc.c   |    6 +++-
  5 files changed, 86 insertions(+), 26 deletions(-)

tools/libxc/xc_domain.c +5 -2

index f9ae4bf..bddf4e0 100644
@@ -192,44 +192,52 @@ int xc_domain_node_getaffinity(xc_interface *xch,
  int xc_vcpu_setaffinity(xc_interface *xch,
                          uint32_t domid,
                          int vcpu,
-                        xc_cpumap_t cpumap)
+                        xc_cpumap_t cpumap,
+                        uint32_t flags,
+                        xc_cpumap_t ecpumap_out)
  {
      DECLARE_DOMCTL;
-    DECLARE_HYPERCALL_BUFFER(uint8_t, local);
+    DECLARE_HYPERCALL_BUFFER(uint8_t, cpumap_local);
+    DECLARE_HYPERCALL_BUFFER(uint8_t, ecpumap_local);
      int ret = -1;
      int cpusize;
 

tools/libxc/xc_domain.c int xc_domain_node_getaffinity(xc_interface *xch, · line 206

Quoted 3 lines

tools/libxc/xc_domain.c +1 -1

      cpusize = xc_get_cpumap_size(xch);
-    if (!cpusize)
+    if ( !cpusize )

I know IanJ will have something to say about non-snuggly braces here and below. :-)

Quoted 63 lines · George Dunlap

Quoted 15 lines · Dario Faggioli

by using the new flag introduced in the parameters of the DOMCTL_{get,set}_vcpuaffinity hypercall.

This happens by adding a new parameter (flags) to xc_vcpu_setaffinity() and xc_vcpu_getaffinity(), so that the caller can decide to set either the soft or hard affinity, or even both.

In case of setting both hard and soft, they are set to the same cpumap. xc_get_setaffinity() also takes another new param, for reporting back to the caller what the actual affinity the scheduler uses will be after a successful call.
In case of asking to get both hard and soft, what the caller gets is the intersection between them.

Quoted 3 lines

In-tree callers are also fixed to cope with the new interface.

Signed-off-by Dario Faggioli <[email protected]>

Acked-by George Dunlap <[email protected]>

But...

Quoted 39 lines

---
Changes from v2:
* better cleanup logic in _vcpu_setaffinity() (regarding xc_hypercall_buffer_{alloc,free}()), as suggested during review;
* make it more evident that DOMCTL_setvcpuaffinity has an out parameter, by calling ecpumap_out, and improving the comment wrt that;
* change the interface and have xc_vcpu_[sg]etaffinity() so that they take the new parameters (flags and ecpumap_out) and fix the in tree callers.

---
   tools/libxc/xc_domain.c             |   47 +++++++++++++++++++++--------------
   tools/libxc/xenctrl.h               |   44 ++++++++++++++++++++++++++++++++-
   tools/libxl/libxl.c                 |    7 ++++-
   tools/ocaml/libs/xc/xenctrl_stubs.c |    8 ++++--
   tools/python/xen/lowlevel/xc/xc.c   |    6 +++-
   5 files changed, 86 insertions(+), 26 deletions(-)

tools/libxc/xc_domain.c +5 -2

index f9ae4bf..bddf4e0 100644
@@ -192,44 +192,52 @@ int xc_domain_node_getaffinity(xc_interface *xch,
   int xc_vcpu_setaffinity(xc_interface *xch,
                           uint32_t domid,
                           int vcpu,
-                        xc_cpumap_t cpumap)
+                        xc_cpumap_t cpumap,
+                        uint32_t flags,
+                        xc_cpumap_t ecpumap_out)
   {
       DECLARE_DOMCTL;
-    DECLARE_HYPERCALL_BUFFER(uint8_t, local);
+    DECLARE_HYPERCALL_BUFFER(uint8_t, cpumap_local);
+    DECLARE_HYPERCALL_BUFFER(uint8_t, ecpumap_local);
       int ret = -1;
       int cpusize;
 

Quoted 6 lines

tools/libxc/xc_domain.c int xc_domain_node_getaffinity(xc_interface *xch, · line 206

Quoted 3 lines

tools/libxc/xc_domain.c +1 -1

       cpusize = xc_get_cpumap_size(xch);
-    if (!cpusize)
+    if ( !cpusize )

I know IanJ will have something to say about non-snuggly braces here and below. :-)

The spaces are legit in libxc I think, it uses Xen coding style. It's libxl which differs...

Signed-off-by Dario Faggioli <[email protected]>

tools/libxc/xc_domain.c +5 -2

index f9ae4bf..bddf4e0 100644
@@ -192,44 +192,52 @@ int xc_domain_node_getaffinity(xc_interface *xch,
 int xc_vcpu_setaffinity(xc_interface *xch,
                         uint32_t domid,
                         int vcpu,
-                        xc_cpumap_t cpumap)
+                        xc_cpumap_t cpumap,
+                        uint32_t flags,
+                        xc_cpumap_t ecpumap_out)
 {
     DECLARE_DOMCTL;
-    DECLARE_HYPERCALL_BUFFER(uint8_t, local);
+    DECLARE_HYPERCALL_BUFFER(uint8_t, cpumap_local);
+    DECLARE_HYPERCALL_BUFFER(uint8_t, ecpumap_local);

tools/libxc/xc_domain.c +4 -3

     }
 
-    local = xc_hypercall_buffer_alloc(xch, local, cpusize);
-    if ( local == NULL )
+    cpumap_local = xc_hypercall_buffer_alloc(xch, cpumap_local, cpusize);
+    ecpumap_local = xc_hypercall_buffer_alloc(xch, ecpumap_local, cpusize);
+    if ( cpumap_local == NULL || cpumap_local == NULL)
     {
-        PERROR("Could not allocate memory for setvcpuaffinity domctl hypercall");
+        PERROR("Could not allocate hcall buffers for DOMCTL_setvcpuaffinity");
         goto out;
     }
 
     domctl.cmd = XEN_DOMCTL_setvcpuaffinity;
     domctl.domain = (domid_t)domid;

tools/libxc/xc_domain.c +2 -0

  out:
+    xc_hypercall_buffer_free(xch, cpumap_local);
+    xc_hypercall_buffer_free(xch, ecpumap_local);
     return ret;
 }
 

Quoted 1 line · Dario Faggioli

Signed-off-by Dario Faggioli <[email protected]>

Acked-by Ian Campbell <[email protected]>

There are a few preexisting issues with the setaffinity function, but this just duplicates them into the new cpumap, so I don't see any point in holding up the series for them. Perhaps you could put them on your todo list?

Quoted 20 lines

tools/libxc/xc_domain.c +5 -2

index f9ae4bf..bddf4e0 100644
@@ -192,44 +192,52 @@ int xc_domain_node_getaffinity(xc_interface *xch,
 int xc_vcpu_setaffinity(xc_interface *xch,
                         uint32_t domid,
                         int vcpu,
-                        xc_cpumap_t cpumap)
+                        xc_cpumap_t cpumap,
+                        uint32_t flags,
+                        xc_cpumap_t ecpumap_out)
 {
     DECLARE_DOMCTL;
-    DECLARE_HYPERCALL_BUFFER(uint8_t, local);
+    DECLARE_HYPERCALL_BUFFER(uint8_t, cpumap_local);
+    DECLARE_HYPERCALL_BUFFER(uint8_t, ecpumap_local);
     int ret = -1;
     int cpusize;
 

tools/libxc/xc_domain.c int xc_domain_node_getaffinity(xc_interface *xch, · line 209

Quoted 7 lines

tools/libxc/xc_domain.c +2 -2

     cpusize = xc_get_cpumap_size(xch);
-    if (!cpusize)
+    if ( !cpusize )
     {
         PERROR("Could not get number of cpus");
-        goto out;
+        return -1;;

Double ";;"?

Quoted 24 lines

tools/libxc/xc_domain.c +5 -9

     }
 
-    local = xc_hypercall_buffer_alloc(xch, local, cpusize);
-    if ( local == NULL )
+    cpumap_local = xc_hypercall_buffer_alloc(xch, cpumap_local, cpusize);
+    ecpumap_local = xc_hypercall_buffer_alloc(xch, ecpumap_local, cpusize);
+    if ( cpumap_local == NULL || cpumap_local == NULL)
     {
-        PERROR("Could not allocate memory for setvcpuaffinity domctl hypercall");
+        PERROR("Could not allocate hcall buffers for DOMCTL_setvcpuaffinity");
         goto out;
     }
 
     domctl.cmd = XEN_DOMCTL_setvcpuaffinity;
     domctl.domain = (domid_t)domid;
     domctl.u.vcpuaffinity.vcpu = vcpu;
-    /* Soft affinity is there, but not used anywhere for now, so... */
-    domctl.u.vcpuaffinity.flags = XEN_VCPUAFFINITY_HARD;
-
-    memcpy(local, cpumap, cpusize);
-
-    set_xen_guest_handle(domctl.u.vcpuaffinity.cpumap.bitmap, local);
+    domctl.u.vcpuaffinity.flags = flags;
 

tools/libxc/xc_domain.c int xc_domain_node_getaffinity(xc_interface *xch, · line 225

Quoted 1 line

tools/libxc/xc_domain.c +1 -0

+    memcpy(cpumap_local, cpumap, cpusize);

This risks running of the end of the supplies cpumap, if it is smaller than cpusize.

But more importantly why is this not using the hypercall buffer bounce mechanism?

Make space for two new cpumap-s, one in vcpu_info (for getting soft affinity) and build_info (for setting it). Provide two new API calls:

* libxl_set_vcpuaffinity2, taking a cpumap and setting either

    hard, soft or both affinity to it, depending on 'flags';

* libxl_set_vcpuaffinity3, taking two cpumap, one for hard and one for soft affinity.

The bheavior of the existing libxl_set_vcpuaffinity is left unchanged, i.e., it only set hard affinity.

Getting soft affinity happens indirectly, via `xl vcpu-list' (as it is already for hard affinity).

The new calls include logic to check whether the affinity which will be used by Xen to schedule the vCPU(s) does actually match with the cpumap provided. In fact, we want to allow every possible combination of hard and soft affinities to be set, but we warn the user upon particularly weird combinations (e.g., hard and soft being disjoint sets of pCPUs).

Also, this is the first change breaking the libxl ABI, so it bumps the MAJOR.

Signed-off-by Dario Faggioli <[email protected]>

tools/libxl/libxl_utils.h +0 -0

index b11cf28..fc3afee 100644
@@ -98,6 +98,21 @@ static inline int libxl_bitmap_cpu_valid(libxl_bitmap *bitmap, int bit)
  #define libxl_for_each_set_bit(v, m) for (v = 0; v < (m).size * 8; v++) \

Quoted 22 lines · Dario Faggioli

Make space for two new cpumap-s, one in vcpu_info (for getting soft affinity) and build_info (for setting it). Provide two new API calls:

* libxl_set_vcpuaffinity2, taking a cpumap and setting either

    hard, soft or both affinity to it, depending on 'flags';

* libxl_set_vcpuaffinity3, taking two cpumap, one for hard and one for soft affinity.

The bheavior of the existing libxl_set_vcpuaffinity is left unchanged, i.e., it only set hard affinity.

Getting soft affinity happens indirectly, via `xl vcpu-list' (as it is already for hard affinity).

The new calls include logic to check whether the affinity which will be used by Xen to schedule the vCPU(s) does actually match with the cpumap provided. In fact, we want to allow every possible combination of hard and soft affinities to be set, but we warn the user upon particularly weird combinations (e.g., hard and soft being disjoint sets of pCPUs).

Quoted 4 lines

Also, this is the first change breaking the libxl ABI, so it bumps the MAJOR.

Signed-off-by Dario Faggioli <[email protected]>

The interface is fine with me (I would probably just have 2 and not 3, but I'm OK with both). Just a few minor comments:

Quoted 4 lines

tools/libxl/libxl.h +0 -0

@@ -973,6 +987,22 @@ int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
                             libxl_bitmap *cpumap);
  int libxl_set_vcpuaffinity_all(libxl_ctx *ctx, uint32_t domid,
                                 unsigned int max_vcpus, libxl_bitmap *cpumap);

tools/libxl/libxl.h int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid, · line 994

Quoted 5 lines

tools/libxl/libxl.h +5 -0

+int libxl_set_vcpuaffinity2(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
+                            const libxl_bitmap *cpumap, int flags);
+int libxl_set_vcpuaffinity_all2(libxl_ctx *ctx, uint32_t domid,
+                                unsigned int max_vcpus,
+                                const libxl_bitmap *cpumap, int flags);

Should we have a bit more documentation about the behavior of "flags" somewhere?

Quoted 8 lines

tools/libxl/libxl_utils.h +0 -0

index b11cf28..fc3afee 100644
@@ -98,6 +98,21 @@ static inline int libxl_bitmap_cpu_valid(libxl_bitmap *bitmap, int bit)
  #define libxl_for_each_set_bit(v, m) for (v = 0; v < (m).size * 8; v++) \
                                               if (libxl_bitmap_test(&(m), v))
 

tools/libxl/libxl_utils.h static inline int libxl_bitmap_cpu_valid(libxl_bitmap *bitmap, int bit) · line 108

Quoted 8 lines

tools/libxl/libxl_utils.h +8 -0

+static inline int libxl_bitmap_equal(const libxl_bitmap *ba,
+                                     const libxl_bitmap *bb,
+                                     int nr_bits)
+{
+    int i;
+
+    /* Only check nr_bits (all bits if <= 0) */
+    nr_bits = nr_bits <=0 ? ba->size * 8 : nr_bits;

The conditional should really have parenthesis around it, I'm pretty sure; and the spacing is inconsistent.

Quoted 9 lines · George Dunlap

Quoted 8 lines · Dario Faggioli

Make space for two new cpumap-s, one in vcpu_info (for getting soft affinity) and build_info (for setting it). Provide two new API calls:

* libxl_set_vcpuaffinity2, taking a cpumap and setting either

    hard, soft or both affinity to it, depending on 'flags';

* libxl_set_vcpuaffinity3, taking two cpumap, one for hard and one for soft affinity.

Quoted 17 lines

Quoted 17 lines

The bheavior of the existing libxl_set_vcpuaffinity is left unchanged, i.e., it only set hard affinity.

Getting soft affinity happens indirectly, via `xl vcpu-list' (as it is already for hard affinity).

The new calls include logic to check whether the affinity which will be used by Xen to schedule the vCPU(s) does actually match with the cpumap provided. In fact, we want to allow every possible combination of hard and soft affinities to be set, but we warn the user upon particularly weird combinations (e.g., hard and soft being disjoint sets of pCPUs).

Also, this is the first change breaking the libxl ABI, so it bumps the MAJOR.

Quoted 5 lines

Quoted 1 line

Signed-off-by Dario Faggioli <[email protected]>

The interface is fine with me (I would probably just have 2 and not 3, but I'm OK with both). Just a few minor comments:

the '3' variant (tries to) accomplish what IanJ explicitly asked: having a way to set both hard and soft affinity at the same time, and each with its own value, and only checking for consistency at the very end.

I also wasn't sure whether that would have been actually useful but, I have to admit, it turned out it is, as it can be seen in the following patches, when the interface is used to (re)implement both the existing and the new xl commands and command variants.

Let's see what IanJ thinks, I guess. :-)

Quoted 6 lines · Dario Faggioli

Quoted 5 lines · George Dunlap

Quoted 4 lines · Dario Faggioli

Make space for two new cpumap-s, one in vcpu_info (for getting soft affinity) and build_info (for setting it). Provide two new API calls:

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

* libxl_set_vcpuaffinity2, taking a cpumap and setting either

    hard, soft or both affinity to it, depending on 'flags';

* libxl_set_vcpuaffinity3, taking two cpumap, one for hard and one for soft affinity.

I must confess that in the end I really dislike these foo, fooN, fooM style functions. Can we not use LIBXL_APIVERSION here to allow us to uprev the API?

Quoted 3 lines

the '3' variant (tries to) accomplish what IanJ explicitly asked: having a way to set both hard and soft affinity at the same time, and each with its own value, and only checking for consistency at the very end.

Can this not be accomplished by a single function which accepts one or zero of the bitmasks being NULL?

Quoted 4 lines

I also wasn't sure whether that would have been actually useful but, I have to admit, it turned out it is, as it can be seen in the following patches, when the interface is used to (re)implement both the existing and the new xl commands and command variants.

So did ...2 turn out not to be useful? Lets not provide both in that case.

As per the mechanism used to amend the current API, I don't have a real preference. Both me and George asked (in a previous thread) what mechanism we should use, but we did not get much feedback at that time (not complain, just summing up what has happened).

Right now, I think I just need for someone authoritative enough to express his preference. You certainly qualify as such, but, if possible, I'd like to see what the other Ian thinks too, as he's been quite deeply involved in reviewing this interface.

Quoted 7 lines

Quoted 3 lines

the '3' variant (tries to) accomplish what IanJ explicitly asked: having a way to set both hard and soft affinity at the same time, and each with its own value, and only checking for consistency at the very end.

Can this not be accomplished by a single function which accepts one or zero of the bitmasks being NULL?

I'll see whether that's true, but yes, I think I can do that.

Quoted 4 lines · Dario Faggioli

As per the mechanism used to amend the current API, I don't have a real preference. Both me and George asked (in a previous thread) what mechanism we should use, but we did not get much feedback at that time (not complain, just summing up what has happened).

Yes, sorry about that.

Quoted 4 lines

Right now, I think I just need for someone authoritative enough to express his preference. You certainly qualify as such, but, if possible, I'd like to see what the other Ian thinks too, as he's been quite deeply involved in reviewing this interface.

I think the major argument in favour of the LIBXL_API_VERSION based approach is that this is how we have dealt with all the previous such changes. Having a mixture of this and fooN interfaces seems to me to be worse that either option by itself.

Quoted 11 lines · Ian Campbell

Quoted 4 lines · Dario Faggioli

As per the mechanism used to amend the current API, I don't have a real preference. Both me and George asked (in a previous thread) what mechanism we should use, but we did not get much feedback at that time (not complain, just summing up what has happened).

Yes, sorry about that.

Quoted 4 lines

Right now, I think I just need for someone authoritative enough to express his preference. You certainly qualify as such, but, if possible, I'd like to see what the other Ian thinks too, as he's been quite deeply involved in reviewing this interface.

Quoted 4 lines

I think the major argument in favour of the LIBXL_API_VERSION based approach is that this is how we have dealt with all the previous such changes. Having a mixture of this and fooN interfaces seems to me to be worse that either option by itself.

Just to be clear -- if we bump the API number, then we have to do some #ifdef-ery to allow programs compiled against the old API to compile, right?

Quoted 2 lines

Just to be clear -- if we bump the API number, then we have to do some #ifdef-ery to allow programs compiled against the old API to compile, right?

Yes.

I expect that the libxl_domain_create_restore machinery is the template which would be desired here.

This does not remove the need for a LIBXL_HAVE_FOO either -- they are somewhat orthogonal. (In practice at least libvirt prefers the LIBXL_HAVE mechanism)

Ian Campbell writes ("Re: [PATCH v3 11/14] libxl: get and set soft affinity"):

Quoted 6 lines

I expect that the libxl_domain_create_restore machinery is the template which would be desired here.

This does not remove the need for a LIBXL_HAVE_FOO either -- they are somewhat orthogonal. (In practice at least libvirt prefers the LIBXL_HAVE mechanism)

For the record, I don't object to this approach, if that's what George and Ian C prefer. I agree that the profusion of variants is ugly.

Quoted 23 lines · George Dunlap

Quoted 22 lines · Dario Faggioli

Make space for two new cpumap-s, one in vcpu_info (for getting soft affinity) and build_info (for setting it). Provide two new API calls:

* libxl_set_vcpuaffinity2, taking a cpumap and setting either

     hard, soft or both affinity to it, depending on 'flags';

* libxl_set_vcpuaffinity3, taking two cpumap, one for hard and one for soft affinity.

The bheavior of the existing libxl_set_vcpuaffinity is left unchanged, i.e., it only set hard affinity.

Getting soft affinity happens indirectly, via `xl vcpu-list' (as it is already for hard affinity).

The new calls include logic to check whether the affinity which will be used by Xen to schedule the vCPU(s) does actually match with the cpumap provided. In fact, we want to allow every possible combination of hard and soft affinities to be set, but we warn the user upon particularly weird combinations (e.g., hard and soft being disjoint sets of pCPUs).

Quoted 7 lines

Quoted 4 lines

Also, this is the first change breaking the libxl ABI, so it bumps the MAJOR.

Signed-off-by Dario Faggioli <[email protected]>

The interface is fine with me (I would probably just have 2 and not 3, but I'm OK with both). Just a few minor comments:

the '3' variant (tries to) accomplish what IanJ explicitly asked: having a way to set both hard and soft affinity at the same time, and each with its own value, and only checking for consistency at the very end.

I also wasn't sure whether that would have been actually useful but, I have to admit, it turned out it is, as it can be seen in the following patches, when the interface is used to (re)implement both the existing and the new xl commands and command variants.

Let's see what IanJ thinks, I guess. :-)

Make space for two new cpumap-s, one in vcpu_info (for getting soft affinity) and build_info (for setting it). Provide two new API calls:

* libxl_set_vcpuaffinity2, taking a cpumap and setting either hard, soft or both affinity to it, depending on 'flags';
* libxl_set_vcpuaffinity3, taking two cpumap, one for hard and one for soft affinity.

The bheavior of the existing libxl_set_vcpuaffinity is left unchanged, i.e., it only set hard affinity.

Getting soft affinity happens indirectly, via `xl vcpu-list' (as it is already for hard affinity).

The new calls include logic to check whether the affinity which will be used by Xen to schedule the vCPU(s) does actually match with the cpumap provided. In fact, we want to allow every possible combination of hard and soft affinities to be set, but we warn the user upon particularly weird combinations (e.g., hard and soft being disjoint sets of pCPUs).

Also, this is the first change breaking the libxl ABI, so it bumps the MAJOR.

Signed-off-by Dario Faggioli <[email protected]>

---
Changes from v2:
* interface completely redesigned, as discussed during review.

---
 tools/libxl/Makefile        |    2 -
 tools/libxl/libxl.c         |  131 +++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl.h         |   30 ++++++++++
 tools/libxl/libxl_create.c  |    6 ++
 tools/libxl/libxl_types.idl |    4 +
 tools/libxl/libxl_utils.h   |   15 +++++
 6 files changed, 186 insertions(+), 2 deletions(-)

tools/libxl/Makefile +1 -1

index cf214bb..cba32d5 100644
@@ -5,7 +5,7 @@
 XEN_ROOT = $(CURDIR)/../..
 include $(XEN_ROOT)/tools/Rules.mk
 
-MAJOR = 4.3
+MAJOR = 4.4
 MINOR = 0
 
 XLUMAJOR = 4.3

tools/libxl/libxl.c +8 -0

index d0db3f0..1122360 100644
@@ -4204,6 +4204,8 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
     for (*nb_vcpu = 0; *nb_vcpu <= domaininfo.max_vcpu_id; ++*nb_vcpu, ++ptr) {
         if (libxl_cpu_bitmap_alloc(ctx, &ptr->cpumap, 0))
             return NULL;
+        if (libxl_cpu_bitmap_alloc(ctx, &ptr->cpumap_soft, 0))
+            return NULL;
         if (xc_vcpu_getinfo(ctx->xch, domid, *nb_vcpu, &vcpuinfo) == -1) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting vcpu info");
             return NULL;

@@ -4214,6 +4216,12 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting vcpu affinity");
             return NULL;
         }
+        if (xc_vcpu_getaffinity(ctx->xch, domid, *nb_vcpu,
+                                XEN_VCPUAFFINITY_SOFT,
+                                ptr->cpumap_soft.map) == -1) {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting vcpu affinity");
+            return NULL;
+        }
         ptr->vcpuid = *nb_vcpu;
         ptr->cpu = vcpuinfo.cpu;
         ptr->online = !!vcpuinfo.online;

tools/libxl/libxl.c +9 -0

+
+    rc = libxl_cpu_bitmap_alloc(ctx, &ecpumap, 0);
+    if (rc)
+        return rc;
+
+    if (xc_vcpu_setaffinity(ctx->xch, domid, vcpuid, cpumap->map,
+                            flags, ecpumap.map)) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "setting vcpu affinity");
+        rc = ERROR_FAIL;

tools/libxl/libxl.h +2 -0

index c7dceda..504c57b 100644
@@ -82,6 +82,20 @@
 #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
 
 /*
+ * LIBXL_HAVE_VCPUINFO_SOFTAFFINITY indicates that a 'cpumap_soft'
+ * field (of libxl_bitmap type) is present in libxl_vcpuinfo,

tools/libxl/libxl.h +7 -0

+
+/*
  * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the
  * libxl_vendor_device field is present in the hvm sections of
  * libxl_domain_build_info. This field tells libxl which

@@ -973,6 +987,22 @@ int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
                            libxl_bitmap *cpumap);
 int libxl_set_vcpuaffinity_all(libxl_ctx *ctx, uint32_t domid,
                                unsigned int max_vcpus, libxl_bitmap *cpumap);
+int libxl_set_vcpuaffinity2(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
+                            const libxl_bitmap *cpumap, int flags);
+int libxl_set_vcpuaffinity_all2(libxl_ctx *ctx, uint32_t domid,
+                                unsigned int max_vcpus,
+                                const libxl_bitmap *cpumap, int flags);

tools/libxl/libxl.h +1 -0

+
 int libxl_domain_set_nodeaffinity(libxl_ctx *ctx, uint32_t domid,
                                   libxl_bitmap *nodemap);
 int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid,

tools/libxl/libxl_create.c +0 -0

Review

Quoted 4 lines

Signed-off-by Dario Faggioli <[email protected]>

(quoted diff) +2 -0

+int libxl_set_vcpuaffinity2(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
+                            const libxl_bitmap *cpumap, int flags)

I think if we are going to duplicate hte API in this way then we should still combine the implementation, either with an internal private helper or by making the old API a wrapper around the new one.

The internals of ...2 and ...3 should be shared as far as possible too.

tools/libxl/libxl.c int libxl_set_vcpuaffinity_all(libxl_ctx *ctx, uint32_t domid, · line 4270

Quoted 8 lines

tools/libxl/libxl.c +8 -0

+{
+    libxl_cputopology *topology;
+    libxl_bitmap ecpumap;
+    int nr_cpus = 0, rc;
+
+    topology = libxl_get_cpu_topology(ctx, &nr_cpus);
+    if (!topology) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "failed to retrieve CPU topology");

It's not consistent within the file but I think for new functions we should use the LOG macro variants.

tools/libxl/libxl.c int libxl_set_vcpuaffinity_all(libxl_ctx *ctx, uint32_t domid, · line 4273

Quoted 3 lines

tools/libxl/libxl.c +3 -0

+        return ERROR_FAIL;
+    }
+    libxl_cputopology_list_free(topology, nr_cpus);

Why are you retrieving this only to immediately throw it away?

tools/libxl/libxl.c int libxl_set_vcpuaffinity_all(libxl_ctx *ctx, uint32_t domid, · line 4320

Quoted 3 lines

tools/libxl/libxl.c +3 -0

+int libxl_set_vcpuaffinity3(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
+                            const libxl_bitmap *cpumap_hard,
+                            const libxl_bitmap *cpumap_soft)

Insert the same comments as ...2, because AFAICT it is mostly the same function.

Quoted 7 lines

tools/libxl/libxl.h +0 -0

index c7dceda..504c57b 100644
@@ -82,6 +82,20 @@
 #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
 

tools/libxl/libxl.h line 96

Quoted 13 lines

tools/libxl/libxl.h +12 -0

 /*
+ * LIBXL_HAVE_VCPUINFO_SOFTAFFINITY indicates that a 'cpumap_soft'
+ * field (of libxl_bitmap type) is present in libxl_vcpuinfo,
+ * containing the soft affinity for the vcpu.
+ */
+#define LIBXL_HAVE_VCPUINFO_SOFTAFFINITY 1
+
+/*
+ * LIBXL_HAVE_BUILDINFO_SOFTAFFINITY indicates that a 'cpumap_soft'
+ * field (of libxl_bitmap type) is present in libxl_domain_build_info,
+ * containing the soft affinity for the vcpu.
+ */
+#define LIBXL_HAVE_BUILDINFO_SOFTAFFINITY 1

Given that they arrive can we just use HAVE_SOFTRAFFINITY?

tools/libxl/libxl.h int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid, · line 1004

Quoted 3 lines

tools/libxl/libxl.h +3 -0

+/* Flags, consistent with domctl.h */
+#define LIBXL_VCPUAFFINITY_HARD 1
+#define LIBXL_VCPUAFFINITY_SOFT 2

Can these be an enum in the idl?

Quoted 12 lines

tools/libxl/libxl.c int libxl_set_vcpuaffinity_all(libxl_ctx *ctx, uint32_t domid, · line 4270

Quoted 8 lines

tools/libxl/libxl.c +8 -0

+{
+    libxl_cputopology *topology;
+    libxl_bitmap ecpumap;
+    int nr_cpus = 0, rc;
+
+    topology = libxl_get_cpu_topology(ctx, &nr_cpus);
+    if (!topology) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "failed to retrieve CPU topology");

It's not consistent within the file but I think for new functions we should use the LOG macro variants.