| Message ID | 1457587634-22819-2-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-03-10 at 06:27 +0100, Juergen Gross wrote: > 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. > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > > Signed-off-by: Juergen Gross <jgross@suse.com> > Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com> Thanks and Regards, Dario
On Thu, Mar 10, 2016 at 06:27:12AM +0100, Juergen Gross wrote: > 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. > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > > Signed-off-by: Juergen Gross <jgross@suse.com> Acked-by: Wei Liu <wei.liu2@citrix.com> > --- > V3: adjust coding style as requested by Wei Liu > > V4: minor code modifications as suggested by Dario Faggioli > --- > tools/libxc/xc_cpupool.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/tools/libxc/xc_cpupool.c b/tools/libxc/xc_cpupool.c > index c42273e..261b9c9 100644 > --- a/tools/libxc/xc_cpupool.c > +++ b/tools/libxc/xc_cpupool.c > @@ -20,6 +20,7 @@ > */ > > #include <stdarg.h> > +#include <unistd.h> > #include "xc_private.h" > > static int do_sysctl_save(xc_interface *xch, struct xen_sysctl *sysctl) > @@ -137,17 +138,34 @@ 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; > - return do_sysctl_save(xch, &sysctl); > + 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; > } > > int xc_cpupool_movedomain(xc_interface *xch, > -- > 2.6.2 >
On Thu, Mar 10, Juergen Gross wrote: > +#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; > - return do_sysctl_save(xch, &sysctl); > + 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; This may fail with gcc-4.8, at least with -Og in 13.1: [ 105s] xc_cpupool.c: In function 'xc_cpupool_removecpu': [ 105s] xc_cpupool.c:168:5: error: 'err' may be used uninitialized in this function [-Werror=maybe-uninitialized] [ 105s] return err; [ 105s] ^ Olaf
On 12/04/16 15:02, Olaf Hering wrote: > On Thu, Mar 10, Juergen Gross wrote: > >> +#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; >> - return do_sysctl_save(xch, &sysctl); >> + 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; > > This may fail with gcc-4.8, at least with -Og in 13.1: > > [ 105s] xc_cpupool.c: In function 'xc_cpupool_removecpu': > [ 105s] xc_cpupool.c:168:5: error: 'err' may be used uninitialized in this function [-Werror=maybe-uninitialized] > [ 105s] return err; > [ 105s] ^ IMO this is a compiler bug. The compiler could detect easily that err can't be uninitialized at the return statement (e.g. via loop unrolling). I can do a patch, of course. The question is whether I should. :-) Juergen
On Tue, Apr 12, Juergen Gross wrote: > IMO this is a compiler bug. The compiler could detect easily that err > can't be uninitialized at the return statement (e.g. via loop > unrolling). There is another place like that in blktap2. There was some discussion about -Og usage in tools, not sure if it will replace -O0 one day. I will send a patch to fix both cases so that xen is prepared for such switch. Olaf
On Tue, Apr 12, 2016 at 03:45:06PM +0200, Juergen Gross wrote: > On 12/04/16 15:02, Olaf Hering wrote: > > On Thu, Mar 10, Juergen Gross wrote: > > > >> +#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; > >> - return do_sysctl_save(xch, &sysctl); > >> + 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; > > > > This may fail with gcc-4.8, at least with -Og in 13.1: > > > > [ 105s] xc_cpupool.c: In function 'xc_cpupool_removecpu': > > [ 105s] xc_cpupool.c:168:5: error: 'err' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > [ 105s] return err; > > [ 105s] ^ > > IMO this is a compiler bug. The compiler could detect easily that err > can't be uninitialized at the return statement (e.g. via loop > unrolling). > > I can do a patch, of course. The question is whether I should. :-) > You should -- and document that it is to make buggy compiler happy. We've done this before. Wei. > > Juergen
Juergen Gross writes ("[PATCH v4 1/3] libxc: do some retries in xc_cpupool_removecpu() for EBUSY case"):
> 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.
Unfortunately this patch is not acceptable because:
> + for ( retries = 0; retries < NUM_RMCPU_BUSY_RETRIES; retries++ ) {
> + err = do_sysctl_save(xch, &sysctl);
> + if ( err < 0 && errno == EBUSY )
> + sleep(1);
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.
Sorry,
Ian.
diff --git a/tools/libxc/xc_cpupool.c b/tools/libxc/xc_cpupool.c index c42273e..261b9c9 100644 --- a/tools/libxc/xc_cpupool.c +++ b/tools/libxc/xc_cpupool.c @@ -20,6 +20,7 @@ */ #include <stdarg.h> +#include <unistd.h> #include "xc_private.h" static int do_sysctl_save(xc_interface *xch, struct xen_sysctl *sysctl) @@ -137,17 +138,34 @@ 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; - return do_sysctl_save(xch, &sysctl); + 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; } int xc_cpupool_movedomain(xc_interface *xch,
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. Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- V3: adjust coding style as requested by Wei Liu V4: minor code modifications as suggested by Dario Faggioli --- tools/libxc/xc_cpupool.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)