[PATCH 0/3] adjust error return values for cpupool operations

[PATCH 0/3] adjust error return values for cpupool operations

[Juergen Gross]

Some cpupool operations return the same error value for multiple
situations. This makes it hard for the tools to react accordingly e.g.
by issuing a suitable error message.

Return appropriate error values and document them.

Juergen Gross (3):
  xen: return different error values for cpupool operations
  libxc: adjust retry loop in xc_cpupool_removecpu()
  xen: Document XEN_SYSCTL_CPUPOOL_OP_* error codes

 tools/libxc/xc_cpupool.c    |  6 ++----
 xen/common/cpupool.c        | 10 +++++-----
 xen/common/schedule.c       |  2 +-
 xen/include/public/sysctl.h | 36 ++++++++++++++++++++++++++++++++++++
 4 files changed, 44 insertions(+), 10 deletions(-)

-- 
2.6.6


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

[PATCH 1/3] xen: return different error values for cpupool operations

[Juergen Gross]

Today there are several different situations in which moving a cpu
from or to a cpupool will return -EBUSY. This makes it hard for the
user to know what he did wrong, as the Xen tools are not capable to
print a detailed error message.

Depending on the situation return different error codes in order to
enable the tools to print useful messages.

Requested-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/cpupool.c  | 10 +++++-----
 xen/common/schedule.c |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index d0189f8..5dacc61 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -264,7 +264,7 @@ static int cpupool_assign_cpu_locked(struct cpupool *c, unsigned int cpu)
     struct domain *d;
 
     if ( (cpupool_moving_cpu == cpu) && (c != cpupool_cpu_moving) )
-        return -EBUSY;
+        return -EADDRNOTAVAIL;
     ret = schedule_cpu_switch(cpu, c);
     if ( ret )
         return ret;
@@ -301,7 +301,7 @@ static long cpupool_unassign_cpu_helper(void *info)
     spin_lock(&cpupool_lock);
     if ( c != cpupool_cpu_moving )
     {
-        ret = -EBUSY;
+        ret = -EADDRNOTAVAIL;
         goto out;
     }
 
@@ -366,7 +366,7 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
                     c->cpupool_id, cpu);
 
     spin_lock(&cpupool_lock);
-    ret = -EBUSY;
+    ret = -EADDRNOTAVAIL;
     if ( (cpupool_moving_cpu != -1) && (cpu != cpupool_moving_cpu) )
         goto out;
     if ( cpumask_test_cpu(cpu, &cpupool_locked_cpus) )
@@ -537,7 +537,7 @@ static int cpupool_cpu_add(unsigned int cpu)
  */
 static int cpupool_cpu_remove(unsigned int cpu)
 {
-    int ret = -EBUSY;
+    int ret = -ENODEV;
 
     spin_lock(&cpupool_lock);
     if ( system_state == SYS_STATE_suspend )
@@ -647,7 +647,7 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
         ret = -EINVAL;
         if ( cpu >= nr_cpu_ids )
             goto addcpu_out;
-        ret = -EBUSY;
+        ret = -ENODEV;
         if ( !cpumask_test_cpu(cpu, &cpupool_free_cpus) )
             goto addcpu_out;
         c = cpupool_find_by_id(op->cpupool_id);
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 013e5f1..5546999 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -688,7 +688,7 @@ int cpu_disable_scheduler(unsigned int cpu)
                 {
                     /* The vcpu is temporarily pinned, can't move it. */
                     vcpu_schedule_unlock_irqrestore(lock, flags, v);
-                    ret = -EBUSY;
+                    ret = -EADDRINUSE;
                     break;
                 }
 
-- 
2.6.6


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

[PATCH 2/3] libxc: adjust retry loop in xc_cpupool_removecpu()

[Juergen Gross]

Commit 1ef6beea187b ("libxc: do some retries in xc_cpupool_removecpu()
for EBUSY case") added a retry loop in xc_cpupool_removecpu() for the
EBUSY case. As EBUSY was returned in multiple error situations the
loop would have been executed in situations where a retry would not
be successful. Additionally calling sleep(1) between the rerires is a
bad idea when being called in a daemon.

The hypervisor has been changed to return different error values now.
The retry added in above mentioned commit should be done in the
EADDRINUSE case now. As the error condition should last only for a
very short time, the sleep(1) call can be removed.

Requested-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxc/xc_cpupool.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_cpupool.c b/tools/libxc/xc_cpupool.c
index 261b9c9..bb99e3b 100644
--- a/tools/libxc/xc_cpupool.c
+++ b/tools/libxc/xc_cpupool.c
@@ -139,7 +139,7 @@ int xc_cpupool_addcpu(xc_interface *xch,
 }
 
 /*
- * The hypervisor might return EBUSY when trying to remove a cpu from a
+ * The hypervisor might return EADDRINUSE 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.
@@ -160,9 +160,7 @@ int xc_cpupool_removecpu(xc_interface *xch,
     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
+        if ( err == 0 || errno != EADDRINUSE )
             break;
     }
     return err;
-- 
2.6.6


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

[PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_* error codes

[Juergen Gross]

Requested-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/include/public/sysctl.h | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 4596d20..82a2a3e 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -559,6 +559,42 @@ 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);
 
+/*
+ * Error return values of cpupool operations:
+ *
+ * -EADDRINUSE:
+ *  XEN_SYSCTL_CPUPOOL_OP_RMCPU: A vcpu is temporarily pinned to the cpu
+ *    which is to be removed from a cpupool.
+ * -EADDRNOTAVAIL:
+ *  XEN_SYSCTL_CPUPOOL_OP_ADDCPU, XEN_SYSCTL_CPUPOOL_OP_RMCPU: A previous
+ *    request to remove a cpu from a cpupool was terminated with -EAGAIN
+ *    and has not been retried using the same parameters.
+ * -EAGAIN:
+ *  XEN_SYSCTL_CPUPOOL_OP_RMCPU: The cpu can't be removed from the cpupool
+ *    as it is active in the hypervisor. A retry will succeed soon.
+ * -EBUSY:
+ *  XEN_SYSCTL_CPUPOOL_OP_DESTROY, XEN_SYSCTL_CPUPOOL_OP_RMCPU: A cpupool
+ *    can't be destroyed or the last cpu can't be removed as there is still
+ *    a running domain in that cpupool.
+ * -EEXIST:
+ *  XEN_SYSCTL_CPUPOOL_OP_CREATE: A cpupool_id was specified and is already
+ *    existing.
+ * -EINVAL:
+ *  XEN_SYSCTL_CPUPOOL_OP_ADDCPU, XEN_SYSCTL_CPUPOOL_OP_RMCPU: An illegal
+ *    cpu was specified (cpu does not exist).
+ *  XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN: An illegal domain was specified
+ *    (domain id illegal or not suitable for operation).
+ * -ENODEV:
+ *  XEN_SYSCTL_CPUPOOL_OP_ADDCPU, XEN_SYSCTL_CPUPOOL_OP_RMCPU: The specified
+ *    cpu is either not free (add) or not member of the specified cpupool
+ *    (remove).
+ * -ENOENT:
+ *  all: The cpupool with the specified cpupool_id doesn't exist.
+ *
+ * Some common error return values like -ENOMEM and -EFAULT are possible for
+ * all the operations.
+ */
+
 #define ARINC653_MAX_DOMAINS_PER_SCHEDULE   64
 /*
  * This structure is used to pass a new ARINC653 schedule from a
-- 
2.6.6


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

Re: [PATCH 0/3] adjust error return values for cpupool operations

[Wei Liu]

On Fri, Apr 15, 2016 at 04:54:14PM +0200, Juergen Gross wrote:
> Some cpupool operations return the same error value for multiple
> situations. This makes it hard for the tools to react accordingly e.g.
> by issuing a suitable error message.
> 
> Return appropriate error values and document them.
> 
> Juergen Gross (3):
>   xen: return different error values for cpupool operations
>   libxc: adjust retry loop in xc_cpupool_removecpu()
>   xen: Document XEN_SYSCTL_CPUPOOL_OP_* error codes
> 
>  tools/libxc/xc_cpupool.c    |  6 ++----
>  xen/common/cpupool.c        | 10 +++++-----
>  xen/common/schedule.c       |  2 +-
>  xen/include/public/sysctl.h | 36 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 44 insertions(+), 10 deletions(-)
> 

In principle I agree we need better semantics and documentation of this
hypercall, and because this is new code so it's risk free, so:

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

I will leave this series for Ian to review.

> -- 
> 2.6.6
> 

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

Re: [PATCH 1/3] xen: return different error values for cpupool operations

[Ian Jackson]

Juergen Gross writes ("[PATCH 1/3] xen: return different error values for cpupool operations"):
> Today there are several different situations in which moving a cpu
> from or to a cpupool will return -EBUSY. This makes it hard for the
> user to know what he did wrong, as the Xen tools are not capable to
> print a detailed error message.
> 
> Depending on the situation return different error codes in order to
> enable the tools to print useful messages.

For what it's worth

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

although I have no real ability to review this code for correctness.

Ian.

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

Re: [PATCH 2/3] libxc: adjust retry loop in xc_cpupool_removecpu()

[Ian Jackson]

Juergen Gross writes ("[PATCH 2/3] libxc: adjust retry loop in xc_cpupool_removecpu()"):
> Commit 1ef6beea187b ("libxc: do some retries in xc_cpupool_removecpu()
> for EBUSY case") added a retry loop in xc_cpupool_removecpu() for the
> EBUSY case. As EBUSY was returned in multiple error situations the
> loop would have been executed in situations where a retry would not
> be successful. Additionally calling sleep(1) between the rerires is a
> bad idea when being called in a daemon.
> 
> The hypervisor has been changed to return different error values now.
> The retry added in above mentioned commit should be done in the
> EADDRINUSE case now. As the error condition should last only for a
> very short time, the sleep(1) call can be removed.
> 
> Requested-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Thanks.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I will follow up with further patches to improve the behaviour of xl.

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_* error codes

[Ian Jackson]

Juergen Gross writes ("[PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_* error codes"):
> Requested-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks this is very helpful.  I think it should go in as soon as the
actual hypervisor code side has the appropriate ack.

But I have some suggestions for enhancement:

> +/*
> + * Error return values of cpupool operations:
> + *
> + * -EADDRINUSE:
> + *  XEN_SYSCTL_CPUPOOL_OP_RMCPU: A vcpu is temporarily pinned to the cpu
> + *    which is to be removed from a cpupool.

I think this ought to mention the anomalous state the pcpu is left in,
and advise what should be done about it.  I think it would be helpful
to crib from my earlier xxx-ful text.  How about:

  In this case RMCPU 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.
 
> The anomalous situation can be recovered by adding the pcpu back to
> the cpupool it came from, and then retrying the RMCPU.

But I notice that the code you propose for libxc doesn't do the
re-add: it just retries the remove.  So either the text above (which
you and Dario seemed to agree with) is wrong, or the libxc code is
wrong.


And, perhaps we ought to mention here that this temporary pinning can
only be done by the hardware domain ?  Or at least refer to the
temporary pin operation.

Ian.

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

Re: [PATCH 1/3] xen: return different error values for cpupool operations

[Dario Faggioli]

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

On Fri, 2016-04-15 at 16:18 +0100, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH 1/3] xen: return different error values
> for cpupool operations"):
> > 
> > Today there are several different situations in which moving a cpu
> > from or to a cpupool will return -EBUSY. This makes it hard for the
> > user to know what he did wrong, as the Xen tools are not capable to
> > print a detailed error message.
> > 
> > Depending on the situation return different error codes in order to
> > enable the tools to print useful messages.
> For what it's worth
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
Acked-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: 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] libxc: adjust retry loop in xc_cpupool_removecpu()

[Dario Faggioli]

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

On Fri, 2016-04-15 at 16:19 +0100, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH 2/3] libxc: adjust retry loop in
> xc_cpupool_removecpu()"):
> > 
> > Commit 1ef6beea187b ("libxc: do some retries in
> > xc_cpupool_removecpu()
> > for EBUSY case") added a retry loop in xc_cpupool_removecpu() for
> > the
> > EBUSY case. As EBUSY was returned in multiple error situations the
> > loop would have been executed in situations where a retry would not
> > be successful. Additionally calling sleep(1) between the rerires is
> > a
> > bad idea when being called in a daemon.
> > 
> > The hypervisor has been changed to return different error values
> > now.
> > The retry added in above mentioned commit should be done in the
> > EADDRINUSE case now. As the error condition should last only for a
> > very short time, the sleep(1) call can be removed.
> > 
> > Requested-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> Thanks.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.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: 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_* error codes

[Juergen Gross]

On 15/04/16 17:25, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH 3/3] xen: Document XEN_SYSCTL_CPUPOOL_OP_* error codes"):
>> Requested-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Thanks this is very helpful.  I think it should go in as soon as the
> actual hypervisor code side has the appropriate ack.
> 
> But I have some suggestions for enhancement:
> 
>> +/*
>> + * Error return values of cpupool operations:
>> + *
>> + * -EADDRINUSE:
>> + *  XEN_SYSCTL_CPUPOOL_OP_RMCPU: A vcpu is temporarily pinned to the cpu
>> + *    which is to be removed from a cpupool.
> 
> I think this ought to mention the anomalous state the pcpu is left in,
> and advise what should be done about it.  I think it would be helpful
> to crib from my earlier xxx-ful text.  How about:
> 
>   In this case RMCPU 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.
>  
>> The anomalous situation can be recovered by adding the pcpu back to
>> the cpupool it came from, and then retrying the RMCPU.
> 
> But I notice that the code you propose for libxc doesn't do the
> re-add: it just retries the remove.  So either the text above (which
> you and Dario seemed to agree with) is wrong, or the libxc code is
> wrong.

Re-adding it not necessary. A retry is all that is needed. In case
retries don't succeed for a long time either re-adding the cpu to
it's original pool or doing a "xl vcpu-pin -f ..." and a retry should
clean up the situation.

> And, perhaps we ought to mention here that this temporary pinning can
> only be done by the hardware domain ?  Or at least refer to the
> temporary pin operation.

Yes, together with the vcpu-pin -f hint.

I think this can wait until next week.


Juergen

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

Re: [PATCH 2/3] libxc: adjust retry loop in xc_cpupool_removecpu()

[Alan Robinson]

On Fri, Apr 15, 2016 at 04:54:16PM +0200, Juergen Gross wrote:
> Subject: [Xen-devel] [PATCH 2/3] libxc: adjust retry loop in
> 	xc_cpupool_removecpu()
> List-Id: Xen developer discussion <xen-devel.lists.xen.org>
> 
> Commit 1ef6beea187b ("libxc: do some retries in xc_cpupool_removecpu()
> for EBUSY case") added a retry loop in xc_cpupool_removecpu() for the
> EBUSY case. As EBUSY was returned in multiple error situations the
> loop would have been executed in situations where a retry would not
> be successful. Additionally calling sleep(1) between the rerires is a

s/rerires/retries/

Reviewed-by: Alan Robinson <alan.robinson@ts.fujitsu.com>

Alan


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

Re: [PATCH 1/3] xen: return different error values for cpupool operations

[Jan Beulich]

>>> Juergen Gross <jgross@suse.com> 04/15/16 4:54 PM >>>
>Today there are several different situations in which moving a cpu
>from or to a cpupool will return -EBUSY. This makes it hard for the
>user to know what he did wrong, as the Xen tools are not capable to
>print a detailed error message.
>
>Depending on the situation return different error codes in order to
>enable the tools to print useful messages.
>
>Requested-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH 0/3] adjust error return values for cpupool operations

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 0/3] adjust error return values for cpupool operations"):
> In principle I agree we need better semantics and documentation of this
> hypercall, and because this is new code so it's risk free, so:
> 
>   Release-acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> I will leave this series for Ian to review.

Thanks for all the reviews and acks.  I have queued these three
patches.

Ian.

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