[PATCH 1/2] tools/libxc: Introduce XC_CPUPOOL_POOLID_ANY

[PATCH 1/2] tools/libxc: Introduce XC_CPUPOOL_POOLID_ANY

[George Dunlap]

Callers to xc_cpupool_create() can either request a specific pool id,
or request that Xen do it for them.  But at the moment, the
"automatic" selection is indicated by using a magic value, 0.  This is
undesirable both because it doesn't obviously have meaning, but also
because '0' is a valid cpupool (albeit one which at the moment can't
be changed).

Introduce a constant, XC_CPUPOOL_POOLID_ANY, to indicate this instead.
Have it be the default for the python bindings.

Manually translate it, even though it's the same underlying value,
because we don't yet have a relaible way of enforcing that these
values are the same.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---

I realize this is somewhat of a bike shed, but I want to avoid
propagating this "magic number" interface into the xenlight bindings
if I can.

Also, at some point we might use the IDL to enforce that the libxl
values are identical to the Xen values, at which point we can just
pass the value in directly.

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: Dario Faggioli <dario.faggioli@citrix.com>
CC: Ronald Rojas <ronladred@gmail.com>
---
 tools/libxc/include/xenctrl.h     | 2 ++
 tools/libxc/xc_cpupool.c          | 2 +-
 tools/libxl/libxl.c               | 9 +++++++--
 tools/python/xen/lowlevel/xc/xc.c | 2 +-
 4 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 85d7fe5..927e373 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1052,6 +1052,8 @@ typedef struct xc_cpupoolinfo {
     xc_cpumap_t cpumap;
 } xc_cpupoolinfo_t;
 
+#define XC_CPUPOOL_POOLID_ANY 0xFFFFFFFF
+
 /**
  * Create a new cpupool.
  *
diff --git a/tools/libxc/xc_cpupool.c b/tools/libxc/xc_cpupool.c
index 70011d1..fbd8cc9 100644
--- a/tools/libxc/xc_cpupool.c
+++ b/tools/libxc/xc_cpupool.c
@@ -43,7 +43,7 @@ int xc_cpupool_create(xc_interface *xch,
 
     sysctl.cmd = XEN_SYSCTL_cpupool_op;
     sysctl.u.cpupool_op.op = XEN_SYSCTL_CPUPOOL_OP_CREATE;
-    sysctl.u.cpupool_op.cpupool_id = (*ppoolid == 0) ?
+    sysctl.u.cpupool_op.cpupool_id = (*ppoolid == XC_CPUPOOL_POOLID_ANY) ?
         XEN_SYSCTL_CPUPOOL_PAR_ANY : *ppoolid;
     sysctl.u.cpupool_op.sched_id = sched_id;
     if ( (err = do_sysctl_save(xch, &sysctl)) != 0 )
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d400fa2..51325d9 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -6285,19 +6285,24 @@ int libxl_cpupool_create(libxl_ctx *ctx, const char *name,
     int i;
     xs_transaction_t t;
     char *uuid_string;
+    uint32_t xcpoolid;
+
+    /* Zero means "choose a poolid for me" */
+    xcpoolid = (*poolid) ? (*poolid) : XC_CPUPOOL_POOLID_ANY;
 
     uuid_string = libxl__uuid2string(gc, *uuid);
     if (!uuid_string) {
         GC_FREE;
         return ERROR_NOMEM;
     }
-
-    rc = xc_cpupool_create(ctx->xch, poolid, sched);
+    
+    rc = xc_cpupool_create(ctx->xch, &xcpoolid, sched);
     if (rc) {
         LOGEV(ERROR, rc, "Could not create cpupool");
         GC_FREE;
         return ERROR_FAIL;
     }
+    *poolid = xcpoolid;
 
     libxl_for_each_bit(i, cpumap)
         if (libxl_bitmap_test(&cpumap, i)) {
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index 39be1d5..9e93d49 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -1715,7 +1715,7 @@ static PyObject *pyxc_cpupool_create(XcObject *self,
                                      PyObject *args,
                                      PyObject *kwds)
 {
-    uint32_t cpupool = 0, sched = XEN_SCHEDULER_CREDIT;
+    uint32_t cpupool = XC_CPUPOOL_POOLID_ANY, sched = XEN_SCHEDULER_CREDIT;
 
     static char *kwd_list[] = { "pool", "sched", NULL };
 
-- 
2.1.4


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

[PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[George Dunlap]

Callers to libxl_cpupool_create() can either request a specific pool
id, or request that Xen do it for them.  But at the moment, the
"automatic" selection is indicated by using a magic value, 0.  This is
undesirable both because it doesn't obviously have meaning, but also
because '0' is a valid cpupool (albeit one which at the moment can't
be changed).

Introduce a constant, LIBXL_CPUPOOL_POOLID_ANY, to indicate this
instead.  Still accept '0' as meaning "ANY" for backwards
compatibility.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: Dario Faggioli <dario.faggioli@citrix.com>
CC: Ronald Rojas <ronladred@gmail.com>
---
 tools/libxl/libxl.c      | 8 ++++++--
 tools/libxl/libxl.h      | 6 ++++++
 tools/libxl/xl_cmdimpl.c | 2 +-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 51325d9..32e537a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -6287,8 +6287,12 @@ int libxl_cpupool_create(libxl_ctx *ctx, const char *name,
     char *uuid_string;
     uint32_t xcpoolid;
 
-    /* Zero means "choose a poolid for me" */
-    xcpoolid = (*poolid) ? (*poolid) : XC_CPUPOOL_POOLID_ANY;
+    /* Accept '0' as 'any poolid' for backwards compatibility */
+    if ( *poolid == LIBXL_CPUPOOL_POOLID_ANY
+         || *poolid == 0 ) 
+        xcpoolid = XC_CPUPOOL_POOLID_ANY;
+    else
+        xcpoolid = *poolid;
 
     uuid_string = libxl__uuid2string(gc, *uuid);
     if (!uuid_string) {
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 3924464..d5559c0 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -2086,6 +2086,12 @@ int libxl_tmem_shared_auth(libxl_ctx *ctx, uint32_t domid, char* uuid,
 int libxl_tmem_freeable(libxl_ctx *ctx);
 
 int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap *cpumap);
+
+/* 
+ * Set poolid to LIBXL_CPUOOL_POOLID_ANY to have Xen choose a
+ * free poolid for you.
+ */
+#define LIBXL_CPUPOOL_POOLID_ANY 0xFFFFFFFF
 int libxl_cpupool_create(libxl_ctx *ctx, const char *name,
                          libxl_scheduler sched,
                          libxl_bitmap cpumap, libxl_uuid *uuid,
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 358757f..80ddeac 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -8366,7 +8366,7 @@ int main_cpupoolcreate(int argc, char **argv)
     printf("number of cpus: %d\n", n_cpus);
 
     if (!dryrun_only) {
-        poolid = 0;
+        poolid = LIBXL_CPUPOOL_POOLID_ANY;
         if (libxl_cpupool_create(ctx, name, sched, cpumap, &uuid, &poolid)) {
             fprintf(stderr, "error on creating cpupool\n");
             goto out_cfg;
-- 
2.1.4


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

Re: [PATCH 1/2] tools/libxc: Introduce XC_CPUPOOL_POOLID_ANY

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1132 bytes --]

On Wed, 2017-02-08 at 14:51 +0000, George Dunlap wrote:
> Callers to xc_cpupool_create() can either request a specific pool id,
> or request that Xen do it for them.  But at the moment, the
> "automatic" selection is indicated by using a magic value, 0.  This
> is
> undesirable both because it doesn't obviously have meaning, but also
> because '0' is a valid cpupool (albeit one which at the moment can't
> be changed).
> 
> Introduce a constant, XC_CPUPOOL_POOLID_ANY, to indicate this
> instead.
> Have it be the default for the python bindings.
> 
> Manually translate it, even though it's the same underlying value,
> because we don't yet have a relaible way of enforcing that these
> values are the same.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1898 bytes --]

On Wed, 2017-02-08 at 14:51 +0000, George Dunlap wrote:
> Callers to libxl_cpupool_create() can either request a specific pool
> id, or request that Xen do it for them.  But at the moment, the
> "automatic" selection is indicated by using a magic value, 0.  This
> is
> undesirable both because it doesn't obviously have meaning, but also
> because '0' is a valid cpupool (albeit one which at the moment can't
> be changed).
> 
> Introduce a constant, LIBXL_CPUPOOL_POOLID_ANY, to indicate this
> instead.  Still accept '0' as meaning "ANY" for backwards
> compatibility.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

With one remark.

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -2086,6 +2086,12 @@ int libxl_tmem_shared_auth(libxl_ctx *ctx,
> uint32_t domid, char* uuid,
>  int libxl_tmem_freeable(libxl_ctx *ctx);
>  
>  int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap *cpumap);
> +
> +/* 
> + * Set poolid to LIBXL_CPUOOL_POOLID_ANY to have Xen choose a
> + * free poolid for you.
> + */
> +#define LIBXL_CPUPOOL_POOLID_ANY 0xFFFFFFFF
>
Do we want this to be here, or in libxl_types.idl.

Asking because, AFAICT, it's the only one LIBXL_FOO_BAR defined like
this. I appreciate that there's few point in making this an enum, as it
is only one value, and will most likely remain so, but still, I thought
I'd at least bring this up.

FWIW, my Reviewed-by stands both if it is kept as is, and if it is
moved to IDL.

Regards,
Dario
-- 
<<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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[George Dunlap]

On 08/02/17 16:11, Dario Faggioli wrote:
> On Wed, 2017-02-08 at 14:51 +0000, George Dunlap wrote:
>> Callers to libxl_cpupool_create() can either request a specific pool
>> id, or request that Xen do it for them.  But at the moment, the
>> "automatic" selection is indicated by using a magic value, 0.  This
>> is
>> undesirable both because it doesn't obviously have meaning, but also
>> because '0' is a valid cpupool (albeit one which at the moment can't
>> be changed).
>>
>> Introduce a constant, LIBXL_CPUPOOL_POOLID_ANY, to indicate this
>> instead.  Still accept '0' as meaning "ANY" for backwards
>> compatibility.
>>
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> With one remark.
> 
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -2086,6 +2086,12 @@ int libxl_tmem_shared_auth(libxl_ctx *ctx,
>> uint32_t domid, char* uuid,
>>  int libxl_tmem_freeable(libxl_ctx *ctx);
>>  
>>  int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap *cpumap);
>> +
>> +/* 
>> + * Set poolid to LIBXL_CPUOOL_POOLID_ANY to have Xen choose a
>> + * free poolid for you.
>> + */
>> +#define LIBXL_CPUPOOL_POOLID_ANY 0xFFFFFFFF
>>
> Do we want this to be here, or in libxl_types.idl.
> 
> Asking because, AFAICT, it's the only one LIBXL_FOO_BAR defined like
> this. I appreciate that there's few point in making this an enum, as it
> is only one value, and will most likely remain so, but still, I thought
> I'd at least bring this up.
> 
> FWIW, my Reviewed-by stands both if it is kept as is, and if it is
> moved to IDL.

Well there's things like:

#define LIBXL_PCI_FUNC_ALL (~0U)

#define LIBXL_TIMER_MODE_DEFAULT -1
#define LIBXL_MEMKB_DEFAULT ~0ULL

#define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024)

#define LIBXL_MS_VM_GENID_LEN 16

#define LIBXL_SUSPEND_DEBUG 1
#define LIBXL_SUSPEND_LIVE 2

Many of which seem similar in some ways.  Enums I think are meant to be
exhaustive (as in, contain all possible options), not be special cases.

But I'm happy to defer to the tools maintainers.

 -George

> 
> Regards,
> Dario
> 


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

Re: [PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[Wei Liu]

On Wed, Feb 08, 2017 at 04:17:58PM +0000, George Dunlap wrote:
> On 08/02/17 16:11, Dario Faggioli wrote:
> > On Wed, 2017-02-08 at 14:51 +0000, George Dunlap wrote:
> >> Callers to libxl_cpupool_create() can either request a specific pool
> >> id, or request that Xen do it for them.  But at the moment, the
> >> "automatic" selection is indicated by using a magic value, 0.  This
> >> is
> >> undesirable both because it doesn't obviously have meaning, but also
> >> because '0' is a valid cpupool (albeit one which at the moment can't
> >> be changed).
> >>
> >> Introduce a constant, LIBXL_CPUPOOL_POOLID_ANY, to indicate this
> >> instead.  Still accept '0' as meaning "ANY" for backwards
> >> compatibility.
> >>
> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> >>
> > Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> > 
> > With one remark.
> > 
> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> >> --- a/tools/libxl/libxl.h
> >> +++ b/tools/libxl/libxl.h
> >> @@ -2086,6 +2086,12 @@ int libxl_tmem_shared_auth(libxl_ctx *ctx,
> >> uint32_t domid, char* uuid,
> >>  int libxl_tmem_freeable(libxl_ctx *ctx);
> >>  
> >>  int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap *cpumap);
> >> +
> >> +/* 
> >> + * Set poolid to LIBXL_CPUOOL_POOLID_ANY to have Xen choose a
> >> + * free poolid for you.
> >> + */
> >> +#define LIBXL_CPUPOOL_POOLID_ANY 0xFFFFFFFF
> >>
> > Do we want this to be here, or in libxl_types.idl.
> > 
> > Asking because, AFAICT, it's the only one LIBXL_FOO_BAR defined like
> > this. I appreciate that there's few point in making this an enum, as it
> > is only one value, and will most likely remain so, but still, I thought
> > I'd at least bring this up.
> > 
> > FWIW, my Reviewed-by stands both if it is kept as is, and if it is
> > moved to IDL.
> 
> Well there's things like:
> 
> #define LIBXL_PCI_FUNC_ALL (~0U)
> 
> #define LIBXL_TIMER_MODE_DEFAULT -1
> #define LIBXL_MEMKB_DEFAULT ~0ULL
> 
> #define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024)
> 
> #define LIBXL_MS_VM_GENID_LEN 16
> 
> #define LIBXL_SUSPEND_DEBUG 1
> #define LIBXL_SUSPEND_LIVE 2
> 
> Many of which seem similar in some ways.  Enums I think are meant to be
> exhaustive (as in, contain all possible options), not be special cases.
> 
> But I'm happy to defer to the tools maintainers.
> 

I don't really care if it is an enum or a macro.

There is an issue that is  more subtle than where it lives or what form
it is in.

You need to modify all the poolid fields in various structure to make it
as default.  Otherwise the whole json infrastructure would still use 0
as the default value.

And maybe a LIBXL_HAVE macro is needed, too.

Wei.

>  -George
> 
> > 
> > Regards,
> > Dario
> > 
> 

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

Re: [PATCH 1/2] tools/libxc: Introduce XC_CPUPOOL_POOLID_ANY

[Wei Liu]

On Wed, Feb 08, 2017 at 02:51:45PM +0000, George Dunlap wrote:
> Callers to xc_cpupool_create() can either request a specific pool id,
> or request that Xen do it for them.  But at the moment, the
> "automatic" selection is indicated by using a magic value, 0.  This is
> undesirable both because it doesn't obviously have meaning, but also
> because '0' is a valid cpupool (albeit one which at the moment can't
> be changed).
> 
> Introduce a constant, XC_CPUPOOL_POOLID_ANY, to indicate this instead.
> Have it be the default for the python bindings.
> 
> Manually translate it, even though it's the same underlying value,
> because we don't yet have a relaible way of enforcing that these
> values are the same.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> 
> I realize this is somewhat of a bike shed, but I want to avoid
> propagating this "magic number" interface into the xenlight bindings
> if I can.
> 
> Also, at some point we might use the IDL to enforce that the libxl
> values are identical to the Xen values, at which point we can just
> pass the value in directly.
> 

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[George Dunlap]

On 09/02/17 10:35, Wei Liu wrote:
> On Wed, Feb 08, 2017 at 04:17:58PM +0000, George Dunlap wrote:
>> On 08/02/17 16:11, Dario Faggioli wrote:
>>> On Wed, 2017-02-08 at 14:51 +0000, George Dunlap wrote:
>>>> Callers to libxl_cpupool_create() can either request a specific pool
>>>> id, or request that Xen do it for them.  But at the moment, the
>>>> "automatic" selection is indicated by using a magic value, 0.  This
>>>> is
>>>> undesirable both because it doesn't obviously have meaning, but also
>>>> because '0' is a valid cpupool (albeit one which at the moment can't
>>>> be changed).
>>>>
>>>> Introduce a constant, LIBXL_CPUPOOL_POOLID_ANY, to indicate this
>>>> instead.  Still accept '0' as meaning "ANY" for backwards
>>>> compatibility.
>>>>
>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>>>>
>>> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
>>>
>>> With one remark.
>>>
>>>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>>>> --- a/tools/libxl/libxl.h
>>>> +++ b/tools/libxl/libxl.h
>>>> @@ -2086,6 +2086,12 @@ int libxl_tmem_shared_auth(libxl_ctx *ctx,
>>>> uint32_t domid, char* uuid,
>>>>  int libxl_tmem_freeable(libxl_ctx *ctx);
>>>>  
>>>>  int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap *cpumap);
>>>> +
>>>> +/* 
>>>> + * Set poolid to LIBXL_CPUOOL_POOLID_ANY to have Xen choose a
>>>> + * free poolid for you.
>>>> + */
>>>> +#define LIBXL_CPUPOOL_POOLID_ANY 0xFFFFFFFF
>>>>
>>> Do we want this to be here, or in libxl_types.idl.
>>>
>>> Asking because, AFAICT, it's the only one LIBXL_FOO_BAR defined like
>>> this. I appreciate that there's few point in making this an enum, as it
>>> is only one value, and will most likely remain so, but still, I thought
>>> I'd at least bring this up.
>>>
>>> FWIW, my Reviewed-by stands both if it is kept as is, and if it is
>>> moved to IDL.
>>
>> Well there's things like:
>>
>> #define LIBXL_PCI_FUNC_ALL (~0U)
>>
>> #define LIBXL_TIMER_MODE_DEFAULT -1
>> #define LIBXL_MEMKB_DEFAULT ~0ULL
>>
>> #define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024)
>>
>> #define LIBXL_MS_VM_GENID_LEN 16
>>
>> #define LIBXL_SUSPEND_DEBUG 1
>> #define LIBXL_SUSPEND_LIVE 2
>>
>> Many of which seem similar in some ways.  Enums I think are meant to be
>> exhaustive (as in, contain all possible options), not be special cases.
>>
>> But I'm happy to defer to the tools maintainers.
>>
> 
> I don't really care if it is an enum or a macro.
> 
> There is an issue that is  more subtle than where it lives or what form
> it is in.
> 
> You need to modify all the poolid fields in various structure to make it
> as default.  Otherwise the whole json infrastructure would still use 0
> as the default value.

Hmm, at the moment there are only two structs that include poolid:
cpupoolinfo, which is OUT-only (so the field should always be
initialized) and domain_create_info, for which 0 is a much better
default logically than "ANY".

> 
> And maybe a LIBXL_HAVE macro is needed, too.

I thought about that, but figured people could just do #ifdef
LIBXL_CPUPOOL_POOLID_ANY.  It seemed a bit strange to define a whole new
macro just to see if another macro existed.

Thoughts?

 -George


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

Re: [PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[Wei Liu]

On Thu, Feb 09, 2017 at 11:17:46AM +0000, George Dunlap wrote:
> On 09/02/17 10:35, Wei Liu wrote:
> > On Wed, Feb 08, 2017 at 04:17:58PM +0000, George Dunlap wrote:
> >> On 08/02/17 16:11, Dario Faggioli wrote:
> >>> On Wed, 2017-02-08 at 14:51 +0000, George Dunlap wrote:
> >>>> Callers to libxl_cpupool_create() can either request a specific pool
> >>>> id, or request that Xen do it for them.  But at the moment, the
> >>>> "automatic" selection is indicated by using a magic value, 0.  This
> >>>> is
> >>>> undesirable both because it doesn't obviously have meaning, but also
> >>>> because '0' is a valid cpupool (albeit one which at the moment can't
> >>>> be changed).
> >>>>
> >>>> Introduce a constant, LIBXL_CPUPOOL_POOLID_ANY, to indicate this
> >>>> instead.  Still accept '0' as meaning "ANY" for backwards
> >>>> compatibility.
> >>>>
> >>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> >>>>
> >>> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> >>>
> >>> With one remark.
> >>>
> >>>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> >>>> --- a/tools/libxl/libxl.h
> >>>> +++ b/tools/libxl/libxl.h
> >>>> @@ -2086,6 +2086,12 @@ int libxl_tmem_shared_auth(libxl_ctx *ctx,
> >>>> uint32_t domid, char* uuid,
> >>>>  int libxl_tmem_freeable(libxl_ctx *ctx);
> >>>>  
> >>>>  int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap *cpumap);
> >>>> +
> >>>> +/* 
> >>>> + * Set poolid to LIBXL_CPUOOL_POOLID_ANY to have Xen choose a
> >>>> + * free poolid for you.
> >>>> + */
> >>>> +#define LIBXL_CPUPOOL_POOLID_ANY 0xFFFFFFFF
> >>>>
> >>> Do we want this to be here, or in libxl_types.idl.
> >>>
> >>> Asking because, AFAICT, it's the only one LIBXL_FOO_BAR defined like
> >>> this. I appreciate that there's few point in making this an enum, as it
> >>> is only one value, and will most likely remain so, but still, I thought
> >>> I'd at least bring this up.
> >>>
> >>> FWIW, my Reviewed-by stands both if it is kept as is, and if it is
> >>> moved to IDL.
> >>
> >> Well there's things like:
> >>
> >> #define LIBXL_PCI_FUNC_ALL (~0U)
> >>
> >> #define LIBXL_TIMER_MODE_DEFAULT -1
> >> #define LIBXL_MEMKB_DEFAULT ~0ULL
> >>
> >> #define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024)
> >>
> >> #define LIBXL_MS_VM_GENID_LEN 16
> >>
> >> #define LIBXL_SUSPEND_DEBUG 1
> >> #define LIBXL_SUSPEND_LIVE 2
> >>
> >> Many of which seem similar in some ways.  Enums I think are meant to be
> >> exhaustive (as in, contain all possible options), not be special cases.
> >>
> >> But I'm happy to defer to the tools maintainers.
> >>
> > 
> > I don't really care if it is an enum or a macro.
> > 
> > There is an issue that is  more subtle than where it lives or what form
> > it is in.
> > 
> > You need to modify all the poolid fields in various structure to make it
> > as default.  Otherwise the whole json infrastructure would still use 0
> > as the default value.
> 
> Hmm, at the moment there are only two structs that include poolid:
> cpupoolinfo, which is OUT-only (so the field should always be
> initialized) and domain_create_info, for which 0 is a much better
> default logically than "ANY".
> 

I don't follow here. Isn't 0 already "ANY"?

> > 
> > And maybe a LIBXL_HAVE macro is needed, too.
> 
> I thought about that, but figured people could just do #ifdef
> LIBXL_CPUPOOL_POOLID_ANY.  It seemed a bit strange to define a whole new
> macro just to see if another macro existed.
> 

I want to reserve the possibility to change that into an enum in the
future.

Wei.

> Thoughts?
> 
>  -George
> 

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

Re: [PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[George Dunlap]

On 09/02/17 11:24, Wei Liu wrote:
> On Thu, Feb 09, 2017 at 11:17:46AM +0000, George Dunlap wrote:
>> On 09/02/17 10:35, Wei Liu wrote:
>>> On Wed, Feb 08, 2017 at 04:17:58PM +0000, George Dunlap wrote:
>>>> On 08/02/17 16:11, Dario Faggioli wrote:
>>>>> On Wed, 2017-02-08 at 14:51 +0000, George Dunlap wrote:
>>>>>> Callers to libxl_cpupool_create() can either request a specific pool
>>>>>> id, or request that Xen do it for them.  But at the moment, the
>>>>>> "automatic" selection is indicated by using a magic value, 0.  This
>>>>>> is
>>>>>> undesirable both because it doesn't obviously have meaning, but also
>>>>>> because '0' is a valid cpupool (albeit one which at the moment can't
>>>>>> be changed).
>>>>>>
>>>>>> Introduce a constant, LIBXL_CPUPOOL_POOLID_ANY, to indicate this
>>>>>> instead.  Still accept '0' as meaning "ANY" for backwards
>>>>>> compatibility.
>>>>>>
>>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>>>>>>
>>>>> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
>>>>>
>>>>> With one remark.
>>>>>
>>>>>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>>>>>> --- a/tools/libxl/libxl.h
>>>>>> +++ b/tools/libxl/libxl.h
>>>>>> @@ -2086,6 +2086,12 @@ int libxl_tmem_shared_auth(libxl_ctx *ctx,
>>>>>> uint32_t domid, char* uuid,
>>>>>>  int libxl_tmem_freeable(libxl_ctx *ctx);
>>>>>>  
>>>>>>  int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap *cpumap);
>>>>>> +
>>>>>> +/* 
>>>>>> + * Set poolid to LIBXL_CPUOOL_POOLID_ANY to have Xen choose a
>>>>>> + * free poolid for you.
>>>>>> + */
>>>>>> +#define LIBXL_CPUPOOL_POOLID_ANY 0xFFFFFFFF
>>>>>>
>>>>> Do we want this to be here, or in libxl_types.idl.
>>>>>
>>>>> Asking because, AFAICT, it's the only one LIBXL_FOO_BAR defined like
>>>>> this. I appreciate that there's few point in making this an enum, as it
>>>>> is only one value, and will most likely remain so, but still, I thought
>>>>> I'd at least bring this up.
>>>>>
>>>>> FWIW, my Reviewed-by stands both if it is kept as is, and if it is
>>>>> moved to IDL.
>>>>
>>>> Well there's things like:
>>>>
>>>> #define LIBXL_PCI_FUNC_ALL (~0U)
>>>>
>>>> #define LIBXL_TIMER_MODE_DEFAULT -1
>>>> #define LIBXL_MEMKB_DEFAULT ~0ULL
>>>>
>>>> #define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024)
>>>>
>>>> #define LIBXL_MS_VM_GENID_LEN 16
>>>>
>>>> #define LIBXL_SUSPEND_DEBUG 1
>>>> #define LIBXL_SUSPEND_LIVE 2
>>>>
>>>> Many of which seem similar in some ways.  Enums I think are meant to be
>>>> exhaustive (as in, contain all possible options), not be special cases.
>>>>
>>>> But I'm happy to defer to the tools maintainers.
>>>>
>>>
>>> I don't really care if it is an enum or a macro.
>>>
>>> There is an issue that is  more subtle than where it lives or what form
>>> it is in.
>>>
>>> You need to modify all the poolid fields in various structure to make it
>>> as default.  Otherwise the whole json infrastructure would still use 0
>>> as the default value.
>>
>> Hmm, at the moment there are only two structs that include poolid:
>> cpupoolinfo, which is OUT-only (so the field should always be
>> initialized) and domain_create_info, for which 0 is a much better
>> default logically than "ANY".
>>
> 
> I don't follow here. Isn't 0 already "ANY"?

No.  This is why I'm bothering to paint this bikeshed: In every context
*except* "cpupool create", 0 means cpupool0 -- the one that was created
at boot, which always contains dom0, and which cannot be destroyed.
(You can only remove all but one of the cpus.)  If you remove a cpupool
from poolid 0, you remove it from cpupool0, not "any" pool.  If you
create a domain and ask to put it in poolid 0, it will be put in
cpupool0, not "any" pool.  The only context in which "0" currently means
"any" is when you're creating a cpupool.

>>> And maybe a LIBXL_HAVE macro is needed, too.
>>
>> I thought about that, but figured people could just do #ifdef
>> LIBXL_CPUPOOL_POOLID_ANY.  It seemed a bit strange to define a whole new
>> macro just to see if another macro existed.
>>
> 
> I want to reserve the possibility to change that into an enum in the
> future.

Yes, I had thought of that -- but like I said, I thought enums were
meant mostly for things for which there was an exhaustive list.  In this
case it's a "magic" value for a parameter which normally has a plain
numerical meaning.

But I can add the #define if you wish.

 -George

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

Re: [PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[Wei Liu]

On Thu, Feb 09, 2017 at 11:35:16AM +0000, George Dunlap wrote:
> On 09/02/17 11:24, Wei Liu wrote:
> > On Thu, Feb 09, 2017 at 11:17:46AM +0000, George Dunlap wrote:
> >> On 09/02/17 10:35, Wei Liu wrote:
> >>> On Wed, Feb 08, 2017 at 04:17:58PM +0000, George Dunlap wrote:
> >>>> On 08/02/17 16:11, Dario Faggioli wrote:
> >>>>> On Wed, 2017-02-08 at 14:51 +0000, George Dunlap wrote:
> >>>>>> Callers to libxl_cpupool_create() can either request a specific pool
> >>>>>> id, or request that Xen do it for them.  But at the moment, the
> >>>>>> "automatic" selection is indicated by using a magic value, 0.  This
> >>>>>> is
> >>>>>> undesirable both because it doesn't obviously have meaning, but also
> >>>>>> because '0' is a valid cpupool (albeit one which at the moment can't
> >>>>>> be changed).
> >>>>>>
> >>>>>> Introduce a constant, LIBXL_CPUPOOL_POOLID_ANY, to indicate this
> >>>>>> instead.  Still accept '0' as meaning "ANY" for backwards
> >>>>>> compatibility.
> >>>>>>
> >>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> >>>>>>
> >>>>> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> >>>>>
> >>>>> With one remark.
> >>>>>
> >>>>>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> >>>>>> --- a/tools/libxl/libxl.h
> >>>>>> +++ b/tools/libxl/libxl.h
> >>>>>> @@ -2086,6 +2086,12 @@ int libxl_tmem_shared_auth(libxl_ctx *ctx,
> >>>>>> uint32_t domid, char* uuid,
> >>>>>>  int libxl_tmem_freeable(libxl_ctx *ctx);
> >>>>>>  
> >>>>>>  int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap *cpumap);
> >>>>>> +
> >>>>>> +/* 
> >>>>>> + * Set poolid to LIBXL_CPUOOL_POOLID_ANY to have Xen choose a
> >>>>>> + * free poolid for you.
> >>>>>> + */
> >>>>>> +#define LIBXL_CPUPOOL_POOLID_ANY 0xFFFFFFFF
> >>>>>>
> >>>>> Do we want this to be here, or in libxl_types.idl.
> >>>>>
> >>>>> Asking because, AFAICT, it's the only one LIBXL_FOO_BAR defined like
> >>>>> this. I appreciate that there's few point in making this an enum, as it
> >>>>> is only one value, and will most likely remain so, but still, I thought
> >>>>> I'd at least bring this up.
> >>>>>
> >>>>> FWIW, my Reviewed-by stands both if it is kept as is, and if it is
> >>>>> moved to IDL.
> >>>>
> >>>> Well there's things like:
> >>>>
> >>>> #define LIBXL_PCI_FUNC_ALL (~0U)
> >>>>
> >>>> #define LIBXL_TIMER_MODE_DEFAULT -1
> >>>> #define LIBXL_MEMKB_DEFAULT ~0ULL
> >>>>
> >>>> #define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024)
> >>>>
> >>>> #define LIBXL_MS_VM_GENID_LEN 16
> >>>>
> >>>> #define LIBXL_SUSPEND_DEBUG 1
> >>>> #define LIBXL_SUSPEND_LIVE 2
> >>>>
> >>>> Many of which seem similar in some ways.  Enums I think are meant to be
> >>>> exhaustive (as in, contain all possible options), not be special cases.
> >>>>
> >>>> But I'm happy to defer to the tools maintainers.
> >>>>
> >>>
> >>> I don't really care if it is an enum or a macro.
> >>>
> >>> There is an issue that is  more subtle than where it lives or what form
> >>> it is in.
> >>>
> >>> You need to modify all the poolid fields in various structure to make it
> >>> as default.  Otherwise the whole json infrastructure would still use 0
> >>> as the default value.
> >>
> >> Hmm, at the moment there are only two structs that include poolid:
> >> cpupoolinfo, which is OUT-only (so the field should always be
> >> initialized) and domain_create_info, for which 0 is a much better
> >> default logically than "ANY".
> >>
> > 
> > I don't follow here. Isn't 0 already "ANY"?
> 
> No.  This is why I'm bothering to paint this bikeshed: In every context
> *except* "cpupool create", 0 means cpupool0 -- the one that was created
> at boot, which always contains dom0, and which cannot be destroyed.
> (You can only remove all but one of the cpus.)  If you remove a cpupool
> from poolid 0, you remove it from cpupool0, not "any" pool.  If you
> create a domain and ask to put it in poolid 0, it will be put in
> cpupool0, not "any" pool.  The only context in which "0" currently means
> "any" is when you're creating a cpupool.
> 

OK. This makes sense.

> >>> And maybe a LIBXL_HAVE macro is needed, too.
> >>
> >> I thought about that, but figured people could just do #ifdef
> >> LIBXL_CPUPOOL_POOLID_ANY.  It seemed a bit strange to define a whole new
> >> macro just to see if another macro existed.
> >>
> > 
> > I want to reserve the possibility to change that into an enum in the
> > future.
> 
> Yes, I had thought of that -- but like I said, I thought enums were
> meant mostly for things for which there was an exhaustive list.  In this
> case it's a "magic" value for a parameter which normally has a plain
> numerical meaning.
> 
> But I can add the #define if you wish.
> 

You don't need to do that.

Wei.

>  -George

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

Re: [PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[George Dunlap]

On Tue, Feb 14, 2017 at 10:29 AM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Thu, Feb 09, 2017 at 11:35:16AM +0000, George Dunlap wrote:
>> No.  This is why I'm bothering to paint this bikeshed: In every context
>> *except* "cpupool create", 0 means cpupool0 -- the one that was created
>> at boot, which always contains dom0, and which cannot be destroyed.
>> (You can only remove all but one of the cpus.)  If you remove a cpupool
>> from poolid 0, you remove it from cpupool0, not "any" pool.  If you
>> create a domain and ask to put it in poolid 0, it will be put in
>> cpupool0, not "any" pool.  The only context in which "0" currently means
>> "any" is when you're creating a cpupool.
>>
>
> OK. This makes sense.
>
[snip]
>> > I want to reserve the possibility to change that into an enum in the
>> > future.
>>
>> Yes, I had thought of that -- but like I said, I thought enums were
>> meant mostly for things for which there was an exhaustive list.  In this
>> case it's a "magic" value for a parameter which normally has a plain
>> numerical meaning.
>>
>> But I can add the #define if you wish.
>>
>
> You don't need to do that.

Is that an Ack? :-)

 -George

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

Re: [PATCH 1/2] tools/libxc: Introduce XC_CPUPOOL_POOLID_ANY

[Wei Liu]

These two patches don't apply anymore. Please rebase.

Wei.

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

[PATCH 2/2] tools/libxl: Introduce LIBXL_CPUPOOL_POOLID_ANY

[George Dunlap]

Callers to libxl_cpupool_create() can either request a specific pool
id, or request that Xen do it for them.  But at the moment, the
"automatic" selection is indicated by using a magic value, 0.  This is
undesirable both because it doesn't obviously have meaning, but also
because '0' is a valid cpupool (albeit one which at the moment can't
be changed).

Introduce a constant, LIBXL_CPUPOOL_POOLID_ANY, to indicate this
instead.  Still accept '0' as meaning "ANY" for backwards
compatibility.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
---
v2: Rebase over libxl.c-split series

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: Dario Faggioli <dario.faggioli@citrix.com>
CC: Ronald Rojas <ronladred@gmail.com>
---
 tools/libxl/libxl.h         | 6 ++++++
 tools/libxl/libxl_cpupool.c | 8 ++++++--
 tools/libxl/xl_cmdimpl.c    | 2 +-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 3924464..d5559c0 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -2086,6 +2086,12 @@ int libxl_tmem_shared_auth(libxl_ctx *ctx, uint32_t domid, char* uuid,
 int libxl_tmem_freeable(libxl_ctx *ctx);
 
 int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap *cpumap);
+
+/* 
+ * Set poolid to LIBXL_CPUOOL_POOLID_ANY to have Xen choose a
+ * free poolid for you.
+ */
+#define LIBXL_CPUPOOL_POOLID_ANY 0xFFFFFFFF
 int libxl_cpupool_create(libxl_ctx *ctx, const char *name,
                          libxl_scheduler sched,
                          libxl_bitmap cpumap, libxl_uuid *uuid,
diff --git a/tools/libxl/libxl_cpupool.c b/tools/libxl/libxl_cpupool.c
index 0ff8724..ef9ff84 100644
--- a/tools/libxl/libxl_cpupool.c
+++ b/tools/libxl/libxl_cpupool.c
@@ -139,8 +139,12 @@ int libxl_cpupool_create(libxl_ctx *ctx, const char *name,
     char *uuid_string;
     uint32_t xcpoolid;
 
-    /* Zero means "choose a poolid for me" */
-    xcpoolid = (*poolid) ? (*poolid) : XC_CPUPOOL_POOLID_ANY;
+    /* Accept '0' as 'any poolid' for backwards compatibility */
+    if ( *poolid == LIBXL_CPUPOOL_POOLID_ANY
+         || *poolid == 0 ) 
+        xcpoolid = XC_CPUPOOL_POOLID_ANY;
+    else
+        xcpoolid = *poolid;
 
     uuid_string = libxl__uuid2string(gc, *uuid);
     if (!uuid_string) {
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 37ebdce..0add5dc 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -8368,7 +8368,7 @@ int main_cpupoolcreate(int argc, char **argv)
     printf("number of cpus: %d\n", n_cpus);
 
     if (!dryrun_only) {
-        poolid = 0;
+        poolid = LIBXL_CPUPOOL_POOLID_ANY;
         if (libxl_cpupool_create(ctx, name, sched, cpumap, &uuid, &poolid)) {
             fprintf(stderr, "error on creating cpupool\n");
             goto out_cfg;
-- 
2.1.4


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