Re: [PATCH v1 5/7] tools/livepatch: Remove pointless retry loop

[Wei Liu <wei.liu2@citrix.com>]

On Mon, Dec 12, 2016 at 04:18:08PM +0000, Ross Lagerwall wrote:
> The default timeout in the hypervisor for a livepatch operation is 30 ms,
> but xen-livepatch currently waits for up to 30 seconds for the operation
> to complete. Instead, remove the retry loop and simply wait for 2 * 30 ms
> for the operation to complete. The extra period is to account for the
> time to actually start the operation.
> 
> Furthermore, have xen-livepatch set the hypervisor timeout rather than
> relying on the hypervisor default since the tool doesn't know how long
> it will be. Use nanosleep rather than usleep since usleep has been
> removed from POSIX.1-2008.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  tools/misc/xen-livepatch.c | 102 ++++++++++++++++++++++-----------------------
>  1 file changed, 51 insertions(+), 51 deletions(-)
> 
> diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
> index afd8c48..d683860 100644
> --- a/tools/misc/xen-livepatch.c
> +++ b/tools/misc/xen-livepatch.c
> @@ -7,6 +7,7 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <time.h>
>  #include <sys/mman.h>
>  #include <sys/stat.h>
>  #include <unistd.h>

Please order the inclusion in alphabetic order.

> @@ -265,17 +266,31 @@ struct {
>      },
>  };
>  
> -/* Go around 300 * 0.1 seconds = 30 seconds. */
> -#define RETRIES 300
> -/* aka 0.1 second */
> -#define DELAY 100000
> +/* The hypervisor timeout for the live patching operation is 30 msec,
> + * but it could take some time for the operation to start, so wait twice
> + * that period. */
> +#define HYPERVISOR_TIMEOUT 30000000 /* in ns */

Use HYPERVISOR_TIMEOUT_NS and remove the comment?

> +#define DELAY (2 * HYPERVISOR_TIMEOUT)
> +
> +static void nanosleep_retry(long ns)
> +{
> +    struct timespec req, rem;
> +    int rc;
> +
> +    rem.tv_sec = 0;
> +    rem.tv_nsec = ns;
> +
> +    do {
> +        req = rem;
> +        rc = nanosleep(&req, &rem);
> +    } while (rc != -1 && errno == EINTR);

Coding style.

And shouldn't it be ( rc == -1 && errno == EINTR ) ?

I don't really have more comment on this approach.

Wei.

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