[PATCH 0/3] Revert xc cpupool retries and document anomaly

[PATCH 0/3] Revert xc cpupool retries and document anomaly

[Ian Jackson]

This small series is part of some cleanup for the CPUPOOL RMCPU EBUSY
problem.

The first patch in this series, a libxc patch, reverts what I think is
simply a mistake.  The second is trivial adds some annotations for the
benefit of the hypercall API HTML docs generator.  The third patch
provides an attempt at documenting the RMCPU EBUSY problem.


It is disappointing that such a strange and hard-to-use interface
should be committed to the hypervisor.  A contributing factor seem to
be a failure to realise that the behaviour should be documented.  An
proper attempt to document it would probably have resulted in
improvements to the interface.

I think that (unless suppositions not marked with `xxx' are false)
this doc comment is an improvement over leaving the anomaly totally
undocumented.  I therefore think that this patch is appropriate to
commit right away (!)

Followups, or review comments, which provide more accurate or more
precise wording, would of course be welcome.

I also think that there is room for improvement in the presented
hypercall semantics, as I understand them.


I would have liked to submit a fourth patch which made xl print the
unhelpful message about trying to re-add and re-remove the cpu, only
when it was relevant.

Unfortunately because the hypervisor doesn't seem to distinguish the
troublesome cases, it is not possible for libxl to tell the
difference.  So it is not possible for libxl to return a different
error code which xl could use to tell the difference.

I would appreciate suggestions from hypervisor maintainers as to how
this could be improved.


Finally, sorry for not spotting these problems earlier.

Thanks,
Ian.

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

[PATCH 1/3] libxc: Revert "do some retries in xc_cpupool_removecpu() for EBUSY case"

[Ian Jackson]

libxc may be called from within long-running daemons such as libvirt.

In such a system this sleep would enable an uncooperative or buggy
guest to block all toolstack operations for an extended period.

Sadly, therefore, such a retry loop is not feasible without a lot of
engineering which is probably not appropriate.

This reverts commit 1ef6beea187bca8d11152b6c7d987b2b9450f936
"libxc: do some retries in xc_cpupool_removecpu() for EBUSY case"

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Dario Faggioli <dario.faggioli@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/xc_cpupool.c |   20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/tools/libxc/xc_cpupool.c b/tools/libxc/xc_cpupool.c
index 261b9c9..c42273e 100644
--- a/tools/libxc/xc_cpupool.c
+++ b/tools/libxc/xc_cpupool.c
@@ -20,7 +20,6 @@
  */
 
 #include <stdarg.h>
-#include <unistd.h>
 #include "xc_private.h"
 
 static int do_sysctl_save(xc_interface *xch, struct xen_sysctl *sysctl)
@@ -138,34 +137,17 @@ int xc_cpupool_addcpu(xc_interface *xch,
     return do_sysctl_save(xch, &sysctl);
 }
 
-/*
- * The hypervisor might return EBUSY when trying to remove a cpu from a
- * cpupool when a domain running in this cpupool has pinned a vcpu
- * temporarily. Do some retries in this case, perhaps the situation
- * cleans up.
- */
-#define NUM_RMCPU_BUSY_RETRIES 5
-
 int xc_cpupool_removecpu(xc_interface *xch,
                          uint32_t poolid,
                          int cpu)
 {
-    unsigned retries;
-    int err;
     DECLARE_SYSCTL;
 
     sysctl.cmd = XEN_SYSCTL_cpupool_op;
     sysctl.u.cpupool_op.op = XEN_SYSCTL_CPUPOOL_OP_RMCPU;
     sysctl.u.cpupool_op.cpupool_id = poolid;
     sysctl.u.cpupool_op.cpu = (cpu < 0) ? XEN_SYSCTL_CPUPOOL_PAR_ANY : cpu;
-    for ( retries = 0; retries < NUM_RMCPU_BUSY_RETRIES; retries++ ) {
-        err = do_sysctl_save(xch, &sysctl);
-        if ( err < 0 && errno == EBUSY )
-            sleep(1);
-        else
-            break;
-    }
-    return err;
+    return do_sysctl_save(xch, &sysctl);
 }
 
 int xc_cpupool_movedomain(xc_interface *xch,
-- 
1.7.10.4


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

[PATCH 2/3] xen: hypercall docs annotations for xen_sysctl_cpupool_op

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/include/public/sysctl.h |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 4596d20..0849908 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -538,7 +538,7 @@ struct xen_sysctl_numainfo {
 typedef struct xen_sysctl_numainfo xen_sysctl_numainfo_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_numainfo_t);
 
-/* XEN_SYSCTL_cpupool_op */
+/* ` enum XEN_SYSCTL_cpupool_op { */
 #define XEN_SYSCTL_CPUPOOL_OP_CREATE                1  /* C */
 #define XEN_SYSCTL_CPUPOOL_OP_DESTROY               2  /* D */
 #define XEN_SYSCTL_CPUPOOL_OP_INFO                  3  /* I */
@@ -546,9 +546,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_sysctl_numainfo_t);
 #define XEN_SYSCTL_CPUPOOL_OP_RMCPU                 5  /* R */
 #define XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN            6  /* M */
 #define XEN_SYSCTL_CPUPOOL_OP_FREEINFO              7  /* F */
+/* ` } */
 #define XEN_SYSCTL_CPUPOOL_PAR_ANY     0xFFFFFFFF
 struct xen_sysctl_cpupool_op {
-    uint32_t op;          /* IN */
+    uint32_t op;          /* IN ` enum XEN_SYSCTL_cpupool_op ` */
     uint32_t cpupool_id;  /* IN: CDIARM OUT: CI */
     uint32_t sched_id;    /* IN: C      OUT: I  */
     uint32_t domid;       /* IN: M              */
-- 
1.7.10.4


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

[PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Ian Jackson]

This is my attempt at understanding the situation, from reading
descriptions provided on list in the context of toolstack patches
which were attempting to work around the anomaly.

The multiple `xxx' entries reflect 1. my lack of complete understanding
2. API defects which I think I have identified.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
CC: Dario Faggioli <dario.faggioli@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/include/public/sysctl.h |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 0849908..cfccf38 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -560,6 +560,34 @@ struct xen_sysctl_cpupool_op {
 typedef struct xen_sysctl_cpupool_op xen_sysctl_cpupool_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpupool_op_t);
 
+/*
+ * cpupool operations may return EBUSY if the operation cannot be
+ * executed right now because of another cpupool operation which is
+ * still in progress.  In this case, EBUSY means that the failed
+ * operation had no effect.
+ *
+ * Some operations including at least RMCPU (xxx which others?) may
+ * also return EBUSY because a guest has temporarily pinned one of its
+ * vcpus to the pcpu in question.  It is the pious hope (xxx) of the
+ * author of this comment that this can only occur for domains which
+ * have been granted some kind of hardware privilege (eg passthrough).
+ *
+ * In this case the operation may have been partially carried out and
+ * the pcpu is left in an anomalous state.  In this state the pcpu may
+ * be used by some not readily predictable subset of the vcpus
+ * (domains) whose vcpus are in the old cpupool.  (xxx is this true?)
+ *
+ * This can be detected by seeing whether the pcpu can be added to a
+ * different cpupool.  (xxx this is a silly interface; the situation
+ * should be reported by a different errno value, at least.)  If the
+ * pcpu can't be added to a different cpupool for this reason,
+ * attempts to do so will returning (xxx what errno value?)
+ *
+ * The anomalous situation can be recovered by adding the pcpu back to
+ * the cpupool it came from (xxx this permits a buggy or malicious
+ * guest to prevent the cpu ever being removed from its cpupool).
+ */ 
+
 #define ARINC653_MAX_DOMAINS_PER_SCHEDULE   64
 /*
  * This structure is used to pass a new ARINC653 schedule from a
-- 
1.7.10.4


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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Andrew Cooper]

On 14/04/16 18:07, Ian Jackson wrote:
> This is my attempt at understanding the situation, from reading
> descriptions provided on list in the context of toolstack patches
> which were attempting to work around the anomaly.
>
> The multiple `xxx' entries reflect 1. my lack of complete understanding
> 2. API defects which I think I have identified.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> CC: Dario Faggioli <dario.faggioli@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/include/public/sysctl.h |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 0849908..cfccf38 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -560,6 +560,34 @@ struct xen_sysctl_cpupool_op {
>  typedef struct xen_sysctl_cpupool_op xen_sysctl_cpupool_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpupool_op_t);
>  
> +/*
> + * cpupool operations may return EBUSY if the operation cannot be
> + * executed right now because of another cpupool operation which is
> + * still in progress.  In this case, EBUSY means that the failed
> + * operation had no effect.
> + *
> + * Some operations including at least RMCPU (xxx which others?) may
> + * also return EBUSY because a guest has temporarily pinned one of its
> + * vcpus to the pcpu in question.  It is the pious hope (xxx) of the
> + * author of this comment that this can only occur for domains which
> + * have been granted some kind of hardware privilege (eg passthrough).

Any VM can be given any arbitrary pinning in its xl configuration file. 
Any arbitrary pinning can be applied at runtime via `xl vcpu-pin ...`

(To the best of my knowledge) A VM cannot choose pinning of its own
accord.  (i.e. the host admin has to choose the pinning.)

~Andrew

> + *
> + * In this case the operation may have been partially carried out and
> + * the pcpu is left in an anomalous state.  In this state the pcpu may
> + * be used by some not readily predictable subset of the vcpus
> + * (domains) whose vcpus are in the old cpupool.  (xxx is this true?)
> + *
> + * This can be detected by seeing whether the pcpu can be added to a
> + * different cpupool.  (xxx this is a silly interface; the situation
> + * should be reported by a different errno value, at least.)  If the
> + * pcpu can't be added to a different cpupool for this reason,
> + * attempts to do so will returning (xxx what errno value?)
> + *
> + * The anomalous situation can be recovered by adding the pcpu back to
> + * the cpupool it came from (xxx this permits a buggy or malicious
> + * guest to prevent the cpu ever being removed from its cpupool).
> + */ 
> +
>  #define ARINC653_MAX_DOMAINS_PER_SCHEDULE   64
>  /*
>   * This structure is used to pass a new ARINC653 schedule from a


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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result"):
> On 14/04/16 18:07, Ian Jackson wrote:
> > +/*
> > + * cpupool operations may return EBUSY if the operation cannot be
> > + * executed right now because of another cpupool operation which is
> > + * still in progress.  In this case, EBUSY means that the failed
> > + * operation had no effect.
> > + *
> > + * Some operations including at least RMCPU (xxx which others?) may
> > + * also return EBUSY because a guest has temporarily pinned one of its
> > + * vcpus to the pcpu in question.  It is the pious hope (xxx) of the
> > + * author of this comment that this can only occur for domains which
> > + * have been granted some kind of hardware privilege (eg passthrough).
> 
> Any VM can be given any arbitrary pinning in its xl configuration file. 
> Any arbitrary pinning can be applied at runtime via `xl vcpu-pin ...`

Does that produce EBUSY as well ?

The reuse of the same error number for all of

  "the existing configuration (eg toolstack-selected vcpu pinning)
   means that the operation does not make sense"

  "there is some lock contention and trying again may help"

  "a semantically conflicting, or nearly-semantically-conflicting,
   operation is currently in progress"

  "the guest has done a temporary pin which prevents this operation"

is very unfortunate.  How is a toolstack to know what to do ?

> (To the best of my knowledge) A VM cannot choose pinning of its own
> accord.  (i.e. the host admin has to choose the pinning.)

AIUI, that is not (now) true.

Ian.

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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Dario Faggioli]

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

On Thu, 2016-04-14 at 18:56 +0100, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [PATCH 3/3] xen: Document
> XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result"):
> > 
> > On 14/04/16 18:07, Ian Jackson wrote:
> > > 
> > > +/*
> > > + * cpupool operations may return EBUSY if the operation cannot
> > > be
> > > + * executed right now because of another cpupool operation which
> > > is
> > > + * still in progress.  In this case, EBUSY means that the failed
> > > + * operation had no effect.
> > > + *
> > > + * Some operations including at least RMCPU (xxx which others?)
> > > may
> > > + * also return EBUSY because a guest has temporarily pinned one
> > > of its
> > > + * vcpus to the pcpu in question.  It is the pious hope (xxx) of
> > > the
> > > + * author of this comment that this can only occur for domains
> > > which
> > > + * have been granted some kind of hardware privilege (eg
> > > passthrough).
> > Any VM can be given any arbitrary pinning in its xl configuration
> > file. 
> > Any arbitrary pinning can be applied at runtime via `xl vcpu-pin
> > ...`
> Does that produce EBUSY as well ?
> 
It can, after Juergen series, but I think in this case (setting
affinity), the situation is still acceptable. In fact:

> The reuse of the same error number for all of
> 
>   "the existing configuration (eg toolstack-selected vcpu pinning)
>    means that the operation does not make sense"
> 
This return -EINVAL.

>   "there is some lock contention and trying again may help"
> 
This can't happen in this case (and reason is just that setting the
affinity of a vcpu is different and less problematic than removing a
cpu from a cpupool).

>   "a semantically conflicting, or nearly-semantically-conflicting,
>    operation is currently in progress"
> 
I'm not sure what this means exactly, but I think that --depending on
what it exactly means-- it either can't happen or fall into the -EINVAL
case.

>   "the guest has done a temporary pin which prevents this operation"
> 
This (because of the series) returns -EBUSY.

> is very unfortunate.  How is a toolstack to know what to do ?
> 
Yeah, I agree, but again, I think in this case it's possible for
toolstack to tell.

From a quick check, we do not, in libxl, output any specific error
message in case we get -EBUSY... but I can send a patch to that effect
pretty quickly, if that's deemed necessary.

> > (To the best of my knowledge) A VM cannot choose pinning of its own
> > accord.  (i.e. the host admin has to choose the pinning.)
> AIUI, that is not (now) true.
> 
Yes, now a guest can call the new SCHEDOP_pin_override hypercall (and
Juergen is pushing a series to Linux for it to be able to do that... as
that was the purpose of the while thing!).

However, as said in another email, there's already a check like this in
place, in the implementation of such an hypercall:

        ret = -EPERM;
        if ( !is_hardware_domain(current->domain) )
            break;

which I think satisfies Ian's (legitimate) concern?

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: 181 bytes --]

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

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

Re: [PATCH 2/3] xen: hypercall docs annotations for xen_sysctl_cpupool_op

[Dario Faggioli]

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

On Thu, 2016-04-14 at 18:07 +0100, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

One thing, out of curiosity. This syntax, here:

> -/* XEN_SYSCTL_cpupool_op */
> +/* ` enum XEN_SYSCTL_cpupool_op { */
>  #define XEN_SYSCTL_CPUPOOL_OP_CREATE                1  /* C */
>  #define XEN_SYSCTL_CPUPOOL_OP_DESTROY               2  /* D */
>  #define XEN_SYSCTL_CPUPOOL_OP_INFO                  3  /* I */
> @@ -546,9 +546,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_sysctl_numainfo_t);
>  #define XEN_SYSCTL_CPUPOOL_OP_RMCPU                 5  /* R */
>  #define XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN            6  /* M */
>  #define XEN_SYSCTL_CPUPOOL_OP_FREEINFO              7  /* F */
> +/* ` } */
>  #define XEN_SYSCTL_CPUPOOL_PAR_ANY     0xFFFFFFFF
>  struct xen_sysctl_cpupool_op {
> -    uint32_t op;          /* IN */
> +    uint32_t op;          /* IN ` enum XEN_SYSCTL_cpupool_op ` */
>
and here, is I guess useful to the hypercall HTML docs generator, as
mentioned in the cover letter?

It is not something we do in many other places (if at all, at least in
this file)... If it is, I'll happily add to my TODO list to convert
more entries to it.

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: 181 bytes --]

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

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

Re: [PATCH 1/3] libxc: Revert "do some retries in xc_cpupool_removecpu() for EBUSY case"

[Juergen Gross]

On 14/04/16 19:07, Ian Jackson wrote:
> libxc may be called from within long-running daemons such as libvirt.
> 
> In such a system this sleep would enable an uncooperative or buggy
> guest to block all toolstack operations for an extended period.
> 
> Sadly, therefore, such a retry loop is not feasible without a lot of
> engineering which is probably not appropriate.

I understand your concerns. OTOH you should consider that libvirt has
no support for cpupool operations today, so it won't run this code.

> This reverts commit 1ef6beea187bca8d11152b6c7d987b2b9450f936
> "libxc: do some retries in xc_cpupool_removecpu() for EBUSY case"
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Dario Faggioli <dario.faggioli@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxc/xc_cpupool.c |   20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)
> 
> diff --git a/tools/libxc/xc_cpupool.c b/tools/libxc/xc_cpupool.c
> index 261b9c9..c42273e 100644
> --- a/tools/libxc/xc_cpupool.c
> +++ b/tools/libxc/xc_cpupool.c
> @@ -20,7 +20,6 @@
>   */
>  
>  #include <stdarg.h>
> -#include <unistd.h>
>  #include "xc_private.h"
>  
>  static int do_sysctl_save(xc_interface *xch, struct xen_sysctl *sysctl)
> @@ -138,34 +137,17 @@ int xc_cpupool_addcpu(xc_interface *xch,
>      return do_sysctl_save(xch, &sysctl);
>  }
>  
> -/*
> - * The hypervisor might return EBUSY when trying to remove a cpu from a
> - * cpupool when a domain running in this cpupool has pinned a vcpu
> - * temporarily. Do some retries in this case, perhaps the situation
> - * cleans up.
> - */
> -#define NUM_RMCPU_BUSY_RETRIES 5
> -
>  int xc_cpupool_removecpu(xc_interface *xch,
>                           uint32_t poolid,
>                           int cpu)
>  {
> -    unsigned retries;
> -    int err;
>      DECLARE_SYSCTL;
>  
>      sysctl.cmd = XEN_SYSCTL_cpupool_op;
>      sysctl.u.cpupool_op.op = XEN_SYSCTL_CPUPOOL_OP_RMCPU;
>      sysctl.u.cpupool_op.cpupool_id = poolid;
>      sysctl.u.cpupool_op.cpu = (cpu < 0) ? XEN_SYSCTL_CPUPOOL_PAR_ANY : cpu;
> -    for ( retries = 0; retries < NUM_RMCPU_BUSY_RETRIES; retries++ ) {
> -        err = do_sysctl_save(xch, &sysctl);
> -        if ( err < 0 && errno == EBUSY )
> -            sleep(1);

I'd rather just remove this sleep() call than the whole retry logic.
EBUSY cases should be very very very very rare and last for some
msecs or usecs only.


Juergen

> -        else
> -            break;
> -    }
> -    return err;
> +    return do_sysctl_save(xch, &sysctl);
>  }
>  
>  int xc_cpupool_movedomain(xc_interface *xch,
> 


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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Juergen Gross]

On 14/04/16 19:07, Ian Jackson wrote:
> This is my attempt at understanding the situation, from reading
> descriptions provided on list in the context of toolstack patches
> which were attempting to work around the anomaly.
> 
> The multiple `xxx' entries reflect 1. my lack of complete understanding
> 2. API defects which I think I have identified.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> CC: Dario Faggioli <dario.faggioli@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/include/public/sysctl.h |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 0849908..cfccf38 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -560,6 +560,34 @@ struct xen_sysctl_cpupool_op {
>  typedef struct xen_sysctl_cpupool_op xen_sysctl_cpupool_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpupool_op_t);
>  
> +/*
> + * cpupool operations may return EBUSY if the operation cannot be
> + * executed right now because of another cpupool operation which is
> + * still in progress.  In this case, EBUSY means that the failed
> + * operation had no effect.
> + *
> + * Some operations including at least RMCPU (xxx which others?) may

Only this one.

> + * also return EBUSY because a guest has temporarily pinned one of its
> + * vcpus to the pcpu in question.  It is the pious hope (xxx) of the
> + * author of this comment that this can only occur for domains which
> + * have been granted some kind of hardware privilege (eg passthrough).

Only the hardware domain is allowed to do this.

> + * In this case the operation may have been partially carried out and
> + * the pcpu is left in an anomalous state.  In this state the pcpu may
> + * be used by some not readily predictable subset of the vcpus
> + * (domains) whose vcpus are in the old cpupool.  (xxx is this true?)

Yes.

> + *
> + * This can be detected by seeing whether the pcpu can be added to a
> + * different cpupool.  (xxx this is a silly interface; the situation
> + * should be reported by a different errno value, at least.)  If the

I'm open for suggestions. :-)

> + * pcpu can't be added to a different cpupool for this reason,
> + * attempts to do so will returning (xxx what errno value?)

You might have guessed: EBUSY

> + *
> + * The anomalous situation can be recovered by adding the pcpu back to
> + * the cpupool it came from (xxx this permits a buggy or malicious
> + * guest to prevent the cpu ever being removed from its cpupool).

Only the hardware domain, which has other means to inject problems.


Juergen

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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Dario Faggioli]

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

On Fri, 2016-04-15 at 07:35 +0200, Juergen Gross wrote:
> On 14/04/16 19:07, Ian Jackson wrote:
> > 
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -560,6 +560,34 @@ struct xen_sysctl_cpupool_op {
> > 
> > + * In this case the operation may have been partially carried out
> > and
> > + * the pcpu is left in an anomalous state.  In this state the pcpu
> > may
> > + * be used by some not readily predictable subset of the vcpus
> > + * (domains) whose vcpus are in the old cpupool.  (xxx is this
> > true?)
> Yes.
> 
Well, I think it's a little less bad than that. the pcpu is no longer a
valid member of its cpupool. This means the scheduler will not run
vcpus of the domains of the pool (it's domains that are in cpupools)
any longer. And even the vcpus that are temporarily stuck on the pcpu
(i.e., the ones that are actually preventing the operation to succeed)
will rather quickly get away from it.

So, I'd say rather that "In this state the pcpu will, potentially after
a short transitory, just not be used by any vcpu"

> > + *
> > + * This can be detected by seeing whether the pcpu can be added to
> > a
> > + * different cpupool.  (xxx this is a silly interface; the
> > situation
> > + * should be reported by a different errno value, at least.)  If
> > the
> I'm open for suggestions. :-)
> 
Well, changing the system behavior with anything that involves retry
loops is indeed non-trivial (or, at least, I don't out of the top of my
head have an alternative).

However, a different return value for the super special case of
temporary pinning override could maybe be selected. I'm having a look,
and although I don't find one that could be seen as a perfect fit (that
would be EBUSY, which is taken!), what about one of these:

  EEXIST        17      /* File exists */
  ENOTEMPTY     39      /* Directory not empty */
  EROFS         30      /* Read-only file system */
  ENOSPC        28      /* No space left on device */

After all, as ugly as the resulting error message would be:
 - if's a very rare special case
 - we need to document this very rare special case anyway
 - we can log more info about the actual error

> > + * pcpu can't be added to a different cpupool for this reason,
> > + * attempts to do so will returning (xxx what errno value?)
> You might have guessed: EBUSY
> 
And here too, if the pcpu is not in cpupool_free_cpus, maybe we can go
and check whether it is in any of the existing cpupool's cpu_valid. If
it is, then EBUSY is ok. If not, it means it's in the inconsistent
state, and we can pick another (same as above?) error value, use it and
document the situation.

> > + *
> > + * The anomalous situation can be recovered by adding the pcpu
> > back to
> > + * the cpupool it came from (xxx this permits a buggy or malicious
> > + * guest to prevent the cpu ever being removed from its cpupool).
> Only the hardware domain, which has other means to inject problems.
> 
Yep, indeed.

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: 181 bytes --]

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

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

Re: [PATCH 1/3] libxc: Revert "do some retries in xc_cpupool_removecpu() for EBUSY case"

[Dario Faggioli]

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

On Fri, 2016-04-15 at 07:15 +0200, Juergen Gross wrote:
> On 14/04/16 19:07, Ian Jackson wrote:
> > 
> > libxc may be called from within long-running daemons such as
> > libvirt.
> > 
> > In such a system this sleep would enable an uncooperative or buggy
> > guest to block all toolstack operations for an extended period.
> > 
> > Sadly, therefore, such a retry loop is not feasible without a lot
> > of
> > engineering which is probably not appropriate.
> I understand your concerns. OTOH you should consider that libvirt has
> no support for cpupool operations today, so it won't run this code.
> 
True, and it does not even have the concept of pooling (not in a
compatible way with what we provide with cpupools), so it probably
won't be start supporting them anytime soon.

HOWEVER, Ian's concern applies to any daemon based toolstack (i.e., not
necessarily libvirt) which may want to support cpupools, and would then
fall into this.

> > diff --git a/tools/libxc/xc_cpupool.c b/tools/libxc/xc_cpupool.c
> > index 261b9c9..c42273e 100644
> > --- a/tools/libxc/xc_cpupool.c
> > +++ b/tools/libxc/xc_cpupool.c
> > @@ -20,7 +20,6 @@
> > 
> >  int xc_cpupool_removecpu(xc_interface *xch,
> >                           uint32_t poolid,
> >                           int cpu)
> >  {
> > -    unsigned retries;
> > -    int err;
> >      DECLARE_SYSCTL;
> >  
> >      sysctl.cmd = XEN_SYSCTL_cpupool_op;
> >      sysctl.u.cpupool_op.op = XEN_SYSCTL_CPUPOOL_OP_RMCPU;
> >      sysctl.u.cpupool_op.cpupool_id = poolid;
> >      sysctl.u.cpupool_op.cpu = (cpu < 0) ?
> > XEN_SYSCTL_CPUPOOL_PAR_ANY : cpu;
> > -    for ( retries = 0; retries < NUM_RMCPU_BUSY_RETRIES; retries++
> > ) {
> > -        err = do_sysctl_save(xch, &sysctl);
> > -        if ( err < 0 && errno == EBUSY )
> > -            sleep(1);
> I'd rather just remove this sleep() call than the whole retry logic.
> EBUSY cases should be very very very very rare and last for some
> msecs or usecs only.
> 
+1

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: 181 bytes --]

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

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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Ian Jackson]

Dario Faggioli writes ("Re: [Xen-devel] [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result"):
> On Thu, 2016-04-14 at 18:56 +0100, Ian Jackson wrote:
> > is very unfortunate.  How is a toolstack to know what to do ?
> > 
> Yeah, I agree, but again, I think in this case it's possible for
> toolstack to tell.

Thanks to both you and Juergen for the explanations.  I'm glad to see
that things weren't as bad as I feared - I admit that I didn't spend
the time to completely follow what all the relevant bits of code did.

Would either of you care to provide a version of my documentation patch
which answers the questions that my text answers ?  Or shall we commit
my version and you can edit it in-tree :-).

>         ret = -EPERM;
>         if ( !is_hardware_domain(current->domain) )
>             break;
> 
> which I think satisfies Ian's (legitimate) concern?

That addresses the security concern, yes.  (Is a driver domain a
`hardware domain' in this sense?)

All I need now is a recipe for the tools to tell what has happened and
then I can make xl or libxl at least print comprehensible and correct
error messages...

Thanks,
Ian.

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

Re: [PATCH 2/3] xen: hypercall docs annotations for xen_sysctl_cpupool_op

[Ian Jackson]

Dario Faggioli writes ("Re: [PATCH 2/3] xen: hypercall docs annotations for xen_sysctl_cpupool_op"):
> On Thu, 2016-04-14 at 18:07 +0100, Ian Jackson wrote:
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Tim Deegan <tim@xen.org>
> >
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> One thing, out of curiosity. This syntax, here:
> 
> > -/* XEN_SYSCTL_cpupool_op */
> > +/* ` enum XEN_SYSCTL_cpupool_op { */
...
> > +    uint32_t op;          /* IN ` enum XEN_SYSCTL_cpupool_op ` */
> >
> and here, is I guess useful to the hypercall HTML docs generator, as
> mentioned in the cover letter?

Yes.  Observe that here
  http://xenbits.xen.org/docs/unstable/hypercall/x86_64/index.html
does not contain an entry for the cpupool ops in the section
"Enums and sets of #defines".  And it doesn't even formally state that
the `op' in that struct is one of the XEN_SYSCTL_cpupool_op values -
although the reader will probably guess that that's the case.

The extra markup syntax is pretty minimal and is documented at the top
of the script
  docs/xen-headers

> It is not something we do in many other places (if at all, at least in
> this file)... If it is, I'll happily add to my TODO list to convert
> more entries to it.

Ideally I would like to see all hypercalls and all their arguments
documented properly.  That includes annotating them for the html
cross-reference generator.  But it also includes documenting their
semantics and their error conditions.

Unfortunately, we are starting from a rather sketchy baseline.

Thanks for the offer to help!

Ian.

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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Juergen Gross]

On 15/04/16 12:20, Ian Jackson wrote:
> Dario Faggioli writes ("Re: [Xen-devel] [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result"):
>> On Thu, 2016-04-14 at 18:56 +0100, Ian Jackson wrote:
>>> is very unfortunate.  How is a toolstack to know what to do ?
>>>
>> Yeah, I agree, but again, I think in this case it's possible for
>> toolstack to tell.
> 
> Thanks to both you and Juergen for the explanations.  I'm glad to see
> that things weren't as bad as I feared - I admit that I didn't spend
> the time to completely follow what all the relevant bits of code did.
> 
> Would either of you care to provide a version of my documentation patch
> which answers the questions that my text answers ?  Or shall we commit
> my version and you can edit it in-tree :-).

I can provide an updated patch.

>>         ret = -EPERM;
>>         if ( !is_hardware_domain(current->domain) )
>>             break;
>>
>> which I think satisfies Ian's (legitimate) concern?
> 
> That addresses the security concern, yes.  (Is a driver domain a
> `hardware domain' in this sense?)

The hardware domain is the one domain having direct access to the
native hardware. This is dom0 in most cases. There can be only one
hardware domain.

> All I need now is a recipe for the tools to tell what has happened and
> then I can make xl or libxl at least print comprehensible and correct
> error messages...

So this boils down to finding an appropriate ESOMETHING replacement
for the EBUSY case introduced by the temporary pinning.

I think ENOTEMPTY or EADDRINUSE would fit best.

The EBUSY returns of not successful repair attempts (trying to assign a
cpu to another cpupool) should be changed to e.g. EADDRNOTAVAIL?


Juergen

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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Dario Faggioli]

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

On Fri, 2016-04-15 at 12:43 +0200, Juergen Gross wrote:
> On 15/04/16 12:20, Ian Jackson wrote:
> > 
> > Would either of you care to provide a version of my documentation
> > patch
> > which answers the questions that my text answers ?  Or shall we
> > commit
> > my version and you can edit it in-tree :-).
> I can provide an updated patch.
> 
Great.

> > All I need now is a recipe for the tools to tell what has happened
> > and
> > then I can make xl or libxl at least print comprehensible and
> > correct
> > error messages...
> So this boils down to finding an appropriate ESOMETHING replacement
> for the EBUSY case introduced by the temporary pinning.
> 
Exactly.

> I think ENOTEMPTY or EADDRINUSE would fit best.
>
I like the latter better.
> 
> The EBUSY returns of not successful repair attempts (trying to assign
> a
> cpu to another cpupool) should be changed to e.g. EADDRNOTAVAIL?
> 
I'd go for EADDRINUSE and EADDRNOTAVAIL so the two error values are
similarly *wrong* hinting an addressing issue, which is more consistent
(and would come handy when documenting) than having one pointing at the
filesystem and the other at the address space.

Are you going to do the patch for this yourself as well?

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: 181 bytes --]

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

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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Juergen Gross]

On 15/04/16 12:58, Dario Faggioli wrote:
> On Fri, 2016-04-15 at 12:43 +0200, Juergen Gross wrote:
>> On 15/04/16 12:20, Ian Jackson wrote:
>>>
>>> Would either of you care to provide a version of my documentation
>>> patch
>>> which answers the questions that my text answers ?  Or shall we
>>> commit
>>> my version and you can edit it in-tree :-).
>> I can provide an updated patch.
>>
> Great.
> 
>>> All I need now is a recipe for the tools to tell what has happened
>>> and
>>> then I can make xl or libxl at least print comprehensible and
>>> correct
>>> error messages...
>> So this boils down to finding an appropriate ESOMETHING replacement
>> for the EBUSY case introduced by the temporary pinning.
>>
> Exactly.
> 
>> I think ENOTEMPTY or EADDRINUSE would fit best.
>>
> I like the latter better.
>>
>> The EBUSY returns of not successful repair attempts (trying to assign
>> a
>> cpu to another cpupool) should be changed to e.g. EADDRNOTAVAIL?
>>
> I'd go for EADDRINUSE and EADDRNOTAVAIL so the two error values are
> similarly *wrong* hinting an addressing issue, which is more consistent
> (and would come handy when documenting) than having one pointing at the
> filesystem and the other at the address space.
> 
> Are you going to do the patch for this yourself as well?

Sure.


Juergen


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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Juergen Gross]

On 15/04/16 12:58, Dario Faggioli wrote:
> On Fri, 2016-04-15 at 12:43 +0200, Juergen Gross wrote:
>> On 15/04/16 12:20, Ian Jackson wrote:
>>>
>>> Would either of you care to provide a version of my documentation
>>> patch
>>> which answers the questions that my text answers ?  Or shall we
>>> commit
>>> my version and you can edit it in-tree :-).
>> I can provide an updated patch.
>>
> Great.
> 
>>> All I need now is a recipe for the tools to tell what has happened
>>> and
>>> then I can make xl or libxl at least print comprehensible and
>>> correct
>>> error messages...
>> So this boils down to finding an appropriate ESOMETHING replacement
>> for the EBUSY case introduced by the temporary pinning.
>>
> Exactly.
> 
>> I think ENOTEMPTY or EADDRINUSE would fit best.
>>
> I like the latter better.
>>
>> The EBUSY returns of not successful repair attempts (trying to assign
>> a
>> cpu to another cpupool) should be changed to e.g. EADDRNOTAVAIL?
>>
> I'd go for EADDRINUSE and EADDRNOTAVAIL so the two error values are
> similarly *wrong* hinting an addressing issue, which is more consistent
> (and would come handy when documenting) than having one pointing at the
> filesystem and the other at the address space.

Just saw there are still two cases left where EBUSY will be returned:

- when trying to remove the last cpu from a cpupool with active domains
  (EBUSY seems appropriate)
- when trying to move a cpu which is not available in the source cpupool
  or free cpus (I'd suggest ENODEV in this case)


Juergen

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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Dario Faggioli]

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

On Fri, 2016-04-15 at 15:20 +0200, Juergen Gross wrote:
> On 15/04/16 12:58, Dario Faggioli wrote:
> > 
> > On Fri, 2016-04-15 at 12:43 +0200, Juergen Gross wrote:
> > > 
> > > The EBUSY returns of not successful repair attempts (trying to
> > > assign
> > > a
> > > cpu to another cpupool) should be changed to e.g. EADDRNOTAVAIL?
> > > 
> > I'd go for EADDRINUSE and EADDRNOTAVAIL so the two error values are
> > similarly *wrong* hinting an addressing issue, which is more
> > consistent
> > (and would come handy when documenting) than having one pointing at
> > the
> > filesystem and the other at the address space.
> Just saw there are still two cases left where EBUSY will be returned:
> 
I know...

> - when trying to remove the last cpu from a cpupool with active
> domains
>   (EBUSY seems appropriate)
>
Indeed.

> - when trying to move a cpu which is not available in the source
> cpupool
>   or free cpus (I'd suggest ENODEV in this case)
> 
Well, in practice, I don't actually think this is too big of a deal. In
fact, I don't think that tools can or need to differentiate their
behavior too much between this and, for instance, the above mentioned
failure.

_However_, if while you're there you're up for making this case
returning ENODEV, it would indeed look like it's more descriptive of
the actual problem, and hence better.

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: 181 bytes --]

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

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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Ian Jackson]

Juergen Gross writes ("Re: [Xen-devel] [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result"):
> Just saw there are still two cases left where EBUSY will be returned:
> 
> - when trying to remove the last cpu from a cpupool with active domains
>   (EBUSY seems appropriate)

The problem is that EBUSY here might mean "this operation can't be
done because it amounts to a request for a configuration which
violates an invariant" or "this operation can't be done for temporary
reasons and you should retry it".

Ian.

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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Ian Jackson]

Dario Faggioli writes ("Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result"):
> However, a different return value for the super special case of
> temporary pinning override could maybe be selected. I'm having a look,
> and although I don't find one that could be seen as a perfect fit (that
> would be EBUSY, which is taken!), what about one of these:
> 
>   EEXIST        17      /* File exists */
>   ENOTEMPTY     39      /* Directory not empty */
>   EROFS         30      /* Read-only file system */
>   ENOSPC        28      /* No space left on device */

Does the hypervisor currently use EAGAIN for anything ?

Ian.

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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Konrad Rzeszutek Wilk]

On Fri, Apr 15, 2016 at 03:12:56PM +0100, Ian Jackson wrote:
> Dario Faggioli writes ("Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result"):
> > However, a different return value for the super special case of
> > temporary pinning override could maybe be selected. I'm having a look,
> > and although I don't find one that could be seen as a perfect fit (that
> > would be EBUSY, which is taken!), what about one of these:
> > 
> >   EEXIST        17      /* File exists */
> >   ENOTEMPTY     39      /* Directory not empty */
> >   EROFS         30      /* Read-only file system */
> >   ENOSPC        28      /* No space left on device */
> 
> Does the hypervisor currently use EAGAIN for anything ?

Yes. XEN_SYSCTL_pm_op, XSM, HVMOP_get_mem_type, MAP_PIRQ_TYPE_MULTI_MSI (ah
scratch that, it makes it -EINVAL).
> 
> Ian.

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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Juergen Gross]

On 15/04/16 16:12, Ian Jackson wrote:
> Dario Faggioli writes ("Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result"):
>> However, a different return value for the super special case of
>> temporary pinning override could maybe be selected. I'm having a look,
>> and although I don't find one that could be seen as a perfect fit (that
>> would be EBUSY, which is taken!), what about one of these:
>>
>>   EEXIST        17      /* File exists */
>>   ENOTEMPTY     39      /* Directory not empty */
>>   EROFS         30      /* Read-only file system */
>>   ENOSPC        28      /* No space left on device */
> 
> Does the hypervisor currently use EAGAIN for anything ?

Yes. Especially XEN_SYSCTL_CPUPOOL_OP_RMCPU can return it already.
Before you are asking: Jan didn't want it to return EAGAIN in the
temporary pinning case as the hypervisor couldn't guarantee that
the situation will be resolved (opposed to the other usage of
EAGAIN).


Juergen


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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Juergen Gross]

On 15/04/16 16:11, Ian Jackson wrote:
> Juergen Gross writes ("Re: [Xen-devel] [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result"):
>> Just saw there are still two cases left where EBUSY will be returned:
>>
>> - when trying to remove the last cpu from a cpupool with active domains
>>   (EBUSY seems appropriate)
> 
> The problem is that EBUSY here might mean "this operation can't be
> done because it amounts to a request for a configuration which
> violates an invariant" or "this operation can't be done for temporary
> reasons and you should retry it".

I'm not sure I understand your comment correctly: do you see a problem
with EBUSY being returned in the two cases I mentioned or just in the
one case you cited?


Juergen


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

Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_RMCPU anomalous EBUSY result"):
> On 15/04/16 16:12, Ian Jackson wrote:
> > Does the hypervisor currently use EAGAIN for anything ?
> 
> Yes. Especially XEN_SYSCTL_CPUPOOL_OP_RMCPU can return it already.

That is a good reason not to use it.

> Before you are asking: Jan didn't want it to return EAGAIN in the
> temporary pinning case as the hypervisor couldn't guarantee that
> the situation will be resolved (opposed to the other usage of
> EAGAIN).

Hrm.  I note that ENOLCK is in the list in errno.h but not used
anywhere.  EISCONN is another possibility...

Ian.

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