Re: [Xen-devel] [PATCH v2 12/16] libxl: get and set soft affinity

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On gio, 2013-11-14 at 15:11 +0000, Ian Jackson wrote:
> > +        if (libxl_cpu_bitmap_alloc(ctx, &ptr->cpumap_soft, 0)) {
> > +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating 
> > cpumap_soft");
> > +            return NULL;
> > +        }
> 
> I went and looked at the error handling.  Normally in libxl allocators
> can't fail but this one wants to do a hypercall to look up the max
> cpus.
> 
Yep.

> But, pulling on the thread, I wonder if it would be better to do this
> error logging in libxl_cpu_bitmap_alloc rather than all the call
> sites.  
>
Indeed. And the main problem I see here is that the logging mostly says
(and mine above is no exception, shame on me! :-P) "failed
allocating ...", which is not true. If something failed, it was the
retrieval of max cpus/nodes!

> (If we did that in principle we should introduce a new #define
> which allows applications to #ifdef out their own logging for this,
> but in practice the double-logging isn't likely to be a problem.)
> 
Do we? Right now we don't have anything like that... Looks to me that
logging inside rather than outside the function is going to result in
the same amount of logging we have already, with the notable difference
that the "new" logging will be much more accurate.

> And looking at libxl_cpu_bitmap_alloc, it calls libxl_get_max_cpus and
> does something very odd with the return value: libxl_get_max_nodes
> ought to return a positive number or a libxl error code.
> 
> So I went to look at libxl_get_max_nodes and it just returns whatever
> it got from libxc, which is definitely wrong!
> 
I think I see what you mean. It's a weird path, since those xc calls
(xc_get_max_cpus() and xc_get_max_nodes()) either return a positive
number (if successful) or zero (if failing).

That's why, I think, libxl ended up checking for zero, as an indication
of failure, rather than "< 0" / libxl error code.

> Would you mind fixing this as part of this series ?
> 
I don't, but I'm not sure I'm 100% getting how you think this should be
fixed. What about the following (compile tested only) patch?

Basically, I tried to modify libxl_get_max_{cpus,nodes} as you suggeste
above (making them return either a positive number or a libxl error
code). At the same time, I made them a bit more "robust" against what
could come from xc_get_max_{cpus,nodes}() (by checking for <= 0).
I also added the proper logging --stating correctly what is it that is
failing-- inside libxl_get_max_{cpus,nodes}() and removed it from the
callsites that had it (wrong).

If you're fine with that, I've got no problem in folding it in the
series.

Thanks and Regards,
Dario

---

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 0de1112..d3ab65e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -616,10 +616,8 @@ static int cpupool_info(libxl__gc *gc,
     info->n_dom = xcinfo->n_dom;
     rc = libxl_cpu_bitmap_alloc(CTX, &info->cpumap, 0);
     if (rc)
-    {
-        LOG(ERROR, "unable to allocate cpumap %d\n", rc);
         goto out;
-    }
+
     memcpy(info->cpumap.map, xcinfo->cpumap, info->cpumap.size);
 
     rc = 0;
@@ -4204,10 +4202,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)) {
-            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpumap");
+        if (libxl_cpu_bitmap_alloc(ctx, &ptr->cpumap, 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;
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 682f874..28e5b91 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -645,6 +645,47 @@ char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const 
libxl_bitmap *bitmap)
     return q;
 }
 
+inline int libxl_cpu_bitmap_alloc(libxl_ctx *ctx,
+                                  libxl_bitmap *cpumap,
+                                  int max_cpus)
+{
+    if (max_cpus < 0)
+        return ERROR_INVAL;
+
+    if (max_cpus == 0)
+        max_cpus = libxl_get_max_cpus(ctx);
+
+    if (max_cpus <= 0) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "failed to retrieve the maximum number of cpus");
+        return ERROR_FAIL;
+    }
+
+
+    /* This can't fail: no need to check and log */
+    return libxl_bitmap_alloc(ctx, cpumap, max_cpus);
+}
+
+int libxl_node_bitmap_alloc(libxl_ctx *ctx,
+                            libxl_bitmap *nodemap,
+                            int max_nodes)
+{
+    if (max_nodes < 0)
+        return ERROR_INVAL;
+
+    if (max_nodes == 0)
+        max_nodes = libxl_get_max_nodes(ctx);
+
+    if (max_nodes <= 0) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "failed to retrieve the maximum number of nodes");
+        return ERROR_FAIL;
+    }
+
+    /* This can't fail: no need to check and log */
+    return libxl_bitmap_alloc(ctx, nodemap, max_nodes);
+}
+
 int libxl_nodemap_to_cpumap(libxl_ctx *ctx,
                             const libxl_bitmap *nodemap,
                             libxl_bitmap *cpumap)
@@ -713,12 +754,16 @@ int libxl_cpumap_to_nodemap(libxl_ctx *ctx,
 
 int libxl_get_max_cpus(libxl_ctx *ctx)
 {
-    return xc_get_max_cpus(ctx->xch);
+    int max_cpus = xc_get_max_cpus(ctx->xch);
+
+    return max_cpus <= 0 ? ERROR_FAIL : max_cpus;
 }
 
 int libxl_get_max_nodes(libxl_ctx *ctx)
 {
-    return xc_get_max_nodes(ctx->xch);
+    int max_nodes = xc_get_max_nodes(ctx->xch);
+
+    return max_nodes <= 0 ? ERROR_FAIL : max_nodes;
 }
 
 int libxl__enum_from_string(const libxl_enum_string_table *t,
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index 7b84e6a..b11cf28 100644
--- a/tools/libxl/libxl_utils.h
+++ b/tools/libxl/libxl_utils.h
@@ -98,32 +98,12 @@ 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))
 
-static inline int libxl_cpu_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *cpumap,
-                                         int max_cpus)
-{
-    if (max_cpus < 0)
-        return ERROR_INVAL;
-    if (max_cpus == 0)
-        max_cpus = libxl_get_max_cpus(ctx);
-    if (max_cpus == 0)
-        return ERROR_FAIL;
-
-    return libxl_bitmap_alloc(ctx, cpumap, max_cpus);
-}
-
-static inline int libxl_node_bitmap_alloc(libxl_ctx *ctx,
-                                          libxl_bitmap *nodemap,
-                                          int max_nodes)
-{
-    if (max_nodes < 0)
-        return ERROR_INVAL;
-    if (max_nodes == 0)
-        max_nodes = libxl_get_max_nodes(ctx);
-    if (max_nodes == 0)
-        return ERROR_FAIL;
-
-    return libxl_bitmap_alloc(ctx, nodemap, max_nodes);
-}
+int libxl_cpu_bitmap_alloc(libxl_ctx *ctx,
+                           libxl_bitmap *cpumap,
+                           int max_cpus);
+int libxl_node_bitmap_alloc(libxl_ctx *ctx,
+                            libxl_bitmap *nodemap,
+                            int max_nodes);
 
 /* Populate cpumap with the cpus spanned by the nodes in nodemap */
 int libxl_nodemap_to_cpumap(libxl_ctx *ctx,


-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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