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

[Juergen Gross <jgross@suse.com>]

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


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