| Message ID | 1457023730-10997-5-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-03-03 at 17:48 +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. > I now I'm at high risk of being called nitpicker (or, more likely, much worse names), but I think that: > --- a/tools/libxc/xc_cpupool.c > +++ b/tools/libxc/xc_cpupool.c > @@ -20,8 +20,11 @@ > */ > > #include <stdarg.h> > +#include <unistd.h> > #include "xc_private.h" > > +#define LIBXC_BUSY_RETRIES 5 > + This name makes me think about something which wants to be more generic than it is actually the case... Like some number of retries that libxc does in general, while it's only applicable to a very specific cpupool operation. Just something like CPUPOOL_NUM_REMOVECPU_RETRIES (or, maybe, even without the CPUPOOL_ prefix, as we're already inside cpupool.c) would be more appropriate. I'd also define it closer to xc_cpupool_removecpu() (but that is a lot about personal taste, I guess) and would add a brief comment (basically, a summary of what's in the changelog already), if only to save people having to go through `git blame'. > @@ -141,13 +144,21 @@ 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 < LIBXC_BUSY_RETRIES; retries++ ) { > + err = do_sysctl_save(xch, &sysctl); > + if ( err >= 0 || errno != EBUSY ) > + break; > + sleep(1); > + } > Doing this the other way round (basically, exactly as the same thing is done in do_sysctl_save() already), reads, IMHO, more natural: for (...) { err = do_sysctl_save(..); if ( err < 0 && errno == EBUSY ) sleep(1); else break; } But yeah, this really is nitpicking. :-) Regards, Dario
On 08/03/16 14:16, Dario Faggioli wrote: > On Thu, 2016-03-03 at 17:48 +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. >> > I now I'm at high risk of being called nitpicker (or, more likely, much > worse names), but I think that: > >> --- a/tools/libxc/xc_cpupool.c >> +++ b/tools/libxc/xc_cpupool.c >> @@ -20,8 +20,11 @@ >> */ >> >> #include <stdarg.h> >> +#include <unistd.h> >> #include "xc_private.h" >> >> +#define LIBXC_BUSY_RETRIES 5 >> + > This name makes me think about something which wants to be more generic > than it is actually the case... Like some number of retries that libxc > does in general, while it's only applicable to a very specific cpupool > operation. > > Just something like CPUPOOL_NUM_REMOVECPU_RETRIES (or, maybe, even > without the CPUPOOL_ prefix, as we're already inside cpupool.c) would > be more appropriate. > > I'd also define it closer to xc_cpupool_removecpu() (but that is a lot > about personal taste, I guess) and would add a brief comment > (basically, a summary of what's in the changelog already), if only to > save people having to go through `git blame'. > >> @@ -141,13 +144,21 @@ 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 < LIBXC_BUSY_RETRIES; retries++ ) { >> + err = do_sysctl_save(xch, &sysctl); >> + if ( err >= 0 || errno != EBUSY ) >> + break; >> + sleep(1); >> + } >> > Doing this the other way round (basically, exactly as the same thing is > done in do_sysctl_save() already), reads, IMHO, more natural: > > for (...) { > err = do_sysctl_save(..); > if ( err < 0 && errno == EBUSY ) > sleep(1); > else > break; > } > > But yeah, this really is nitpicking. :-) Nevertheless I can do it. Need to respin anyway. Juergen
diff --git a/tools/libxc/xc_cpupool.c b/tools/libxc/xc_cpupool.c index c42273e..9626699 100644 --- a/tools/libxc/xc_cpupool.c +++ b/tools/libxc/xc_cpupool.c @@ -20,8 +20,11 @@ */ #include <stdarg.h> +#include <unistd.h> #include "xc_private.h" +#define LIBXC_BUSY_RETRIES 5 + static int do_sysctl_save(xc_interface *xch, struct xen_sysctl *sysctl) { int ret; @@ -141,13 +144,21 @@ 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 < LIBXC_BUSY_RETRIES; retries++ ) { + err = do_sysctl_save(xch, &sysctl); + if ( err >= 0 || errno != EBUSY ) + break; + sleep(1); + } + 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 --- tools/libxc/xc_cpupool.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)