Re: [Xen-devel] [PATCH 1/3] libxc: Revert "do some retries in xc_cpupool_removecpu() for EBUSY case"

[Juergen Gross <jgross@xxxxxxxx>]

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@xxxxxxxxxxxxx>
> CC: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  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,
> 


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