Re: [Xen-devel] [PATCH v3 4/6] libxc: do some retries in xc_cpupool_removecpu() for EBUSY case

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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
-- 
<<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: signature.asc
Description: This is a digitally signed message part

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