| Message ID | 1460653660-6654-2-git-send-email-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, >
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
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,
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(-)