Re: [Xen-devel] [PATCH v10 11/11] (lib)xl: soft reset support

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Jul 28, 2015 at 03:28:16PM +0200, Vitaly Kuznetsov wrote:
> Use existing create/restore path to perform 'soft reset' for HVM
> domains. Tear everything down, e.g. destroy domain's device model,
> remove the domain from xenstore, save toolstack record and start
> over.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> ---
> Changes since v9:
> - s,reset,Reset, in xl.cfg.pod.5 [Konrad Rzeszutek Wilk]
> - full stops in comments/log messages [Konrad Rzeszutek Wilk]
> - reword the need to remove the domain from xs [Konrad Rzeszutek Wilk]
> - rebase on top of current staging, minor fixes to adapt to Andrew's
>   changes.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>

Some comments below. There is only one major concern.

> ---
>  docs/man/xl.cfg.pod.5        |   7 +-
>  tools/libxl/libxl.c          |  23 ++++-
>  tools/libxl/libxl.h          |  15 ++++
>  tools/libxl/libxl_create.c   | 195 
> ++++++++++++++++++++++++++++++++++++++-----
>  tools/libxl/libxl_internal.h |   4 +
>  tools/libxl/libxl_types.idl  |   2 +
>  tools/libxl/xl.h             |   1 +
>  tools/libxl/xl_cmdimpl.c     |  25 +++++-
>  8 files changed, 243 insertions(+), 29 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 69935d9..561710a 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -346,6 +346,11 @@ destroy the domain.
>  write a "coredump" of the domain to F</var/lib/xen/dump/NAME> and then
>  restart the domain.
>  
> +=item B<soft-reset>
> +
> +Reset all Xen specific interfaces for the Xen-aware HVM domain allowing
> +it to reastablish these interfaces and continue executing the domain.
> +

"reestablish" ?

It would also be good if you can tell users what to expect if they use
this on not Xen-aware HVM and PV guests. Maybe something like:

"PV guest is not supported. Not Xen-aware HVM guest will crash."

Subject to correction to actual support status.

>  =back
>  
>  The default for C<on_poweroff> is C<destroy>.
> @@ -367,7 +372,7 @@ Action to take if the domain crashes.  Default is 
> C<destroy>.
>  =item B<on_soft_reset="ACTION">
>  
>  Action to take if the domain performs 'soft reset' (e.g. does kexec).
> -Default is C<restart>.
> +Default is C<soft-reset>.
>  
>  =back
>  
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index ff0d616..5f5559b 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1478,6 +1478,7 @@ void libxl__domain_destroy(libxl__egc *egc, 
> libxl__domain_destroy_state *dds)
>          dds->stubdom.ao = ao;
>          dds->stubdom.domid = stubdomid;
>          dds->stubdom.callback = stubdom_destroy_callback;
> +        dds->stubdom.soft_reset = false;
>          libxl__destroy_domid(egc, &dds->stubdom);
>      } else {
>          dds->stubdom_finished = 1;
> @@ -1486,6 +1487,7 @@ void libxl__domain_destroy(libxl__egc *egc, 
> libxl__domain_destroy_state *dds)
>      dds->domain.ao = ao;
>      dds->domain.domid = dds->domid;
>      dds->domain.callback = domain_destroy_callback;
> +    dds->domain.soft_reset = dds->soft_reset;
>      libxl__destroy_domid(egc, &dds->domain);
>  }

OOI have you tested stubdom case?

Of course stubdom.soft_reset should be set to false, but do you really
want to destroy stubdom?

Sorry if my question is dumb or the answer is obvious. I expect stubdom
QEMU would be rebuilt just like normal QEMU running in Dom0? From
reading the code I think this is the case, but I prefer confirmation
from you.

This is the only concern I have with this patch.

>  
> @@ -1666,10 +1668,14 @@ static void devices_destroy_cb(libxl__egc *egc,
>  
>      /* Clean up qemu-save and qemu-resume files. They are
>       * intermediate files created by libxc. Unfortunately they
> -     * don't fit in existing userdata scheme very well.
> +     * don't fit in existing userdata scheme very well. In soft reset
> +     * case we need to keep the file.
>       */
> -    rc = libxl__remove_file(gc, libxl__device_model_savefile(gc, domid));
> -    if (rc < 0) goto out;
> +    if (!dis->soft_reset) {
> +        rc = libxl__remove_file(gc,
> +                                libxl__device_model_savefile(gc, domid));
> +        if (rc < 0) goto out;
> +    }
>      rc = libxl__remove_file(gc,
>               GCSPRINTF(XC_DEVICE_MODEL_RESTORE_FILE".%u", domid));
>      if (rc < 0) goto out;
> @@ -1680,7 +1686,16 @@ static void devices_destroy_cb(libxl__egc *egc,
>          ctx->xch = xc_interface_open(ctx->lg,0,0);
>          if (!ctx->xch) goto badchild;
>  
> -        rc = xc_domain_destroy(ctx->xch, domid);
> +        if (!dis->soft_reset) {
> +            rc = xc_domain_destroy(ctx->xch, domid);
> +        }
> +        else {

Coding style.

> +            rc = xc_domain_pause(ctx->xch, domid);
> +            if (rc < 0) goto badchild;
> +            rc = xc_domain_soft_reset(ctx->xch, domid);
> +            if (rc < 0) goto badchild;
> +            rc = xc_domain_unpause(ctx->xch, domid);
> +        }
[...]
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 9f6ec00..87d8255 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -138,6 +138,8 @@ libxl_action_on_shutdown = 
> Enumeration("action_on_shutdown", [
>  
>      (5, "COREDUMP_DESTROY"),
>      (6, "COREDUMP_RESTART"),
> +

Stray blank line.

> +    (7, "SOFT_RESET"),
>      ], init_val = "LIBXL_ACTION_ON_SHUTDOWN_DESTROY")
>  
>  libxl_trigger = Enumeration("trigger", [
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index 6c19c0d..0021112 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -194,6 +194,7 @@ typedef enum {
>      DOMAIN_RESTART_NONE = 0,     /* No domain restart */
>      DOMAIN_RESTART_NORMAL,       /* Domain should be restarted */
>      DOMAIN_RESTART_RENAME,       /* Domain should be renamed and restarted */
> +    DOMAIN_RESTART_SOFT_RESET,   /* Soft reset should be performed */
>  } domain_restart_type;
>  
>  extern void printf_info_sexp(int domid, libxl_domain_config *d_config, FILE 
> *fh);
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2e9e1bd..0b29d34 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -133,6 +133,8 @@ static const char *action_on_shutdown_names[] = {
>  
>      [LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_DESTROY] = "coredump-destroy",
>      [LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_RESTART] = "coredump-restart",
> +

Stray blank line.

> +    [LIBXL_ACTION_ON_SHUTDOWN_SOFT_RESET] = "soft-reset",
>  };
>  
>  /* Optional data, in order:
> @@ -1396,7 +1398,7 @@ static void parse_config_data(const char *config_source,
>      }
>  
>      if (xlu_cfg_get_string (config, "on_soft_reset", &buf, 0))
> -        buf = "restart";
> +        buf = "soft-reset";
>      if (!parse_action_on_shutdown(buf, &d_config->on_soft_reset)) {
>          fprintf(stderr, "Unknown on_soft_reset action \"%s\" specified\n", 
> buf);
>          exit(1);
> @@ -2469,6 +2471,11 @@ static domain_restart_type 
> handle_domain_death(uint32_t *r_domid,
>          *r_domid = INVALID_DOMID;
>          break;
>  
> +    case LIBXL_ACTION_ON_SHUTDOWN_SOFT_RESET:
> +        reload_domain_config(*r_domid, d_config);
> +        restart = DOMAIN_RESTART_SOFT_RESET;
> +        break;
> +
>      case LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_DESTROY:
>      case LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_RESTART:
>          /* Already handled these above. */
> @@ -2644,6 +2651,7 @@ static uint32_t create_domain(struct domain_create 
> *dom_info)
>      int restore_fd = -1;
>      const libxl_asyncprogress_how *autoconnect_console_how;
>      struct save_file_header hdr;
> +    uint32_t domid_soft_reset = INVALID_DOMID;
>  
>      int restoring = (restore_file || (migrate_fd >= 0));
>  
> @@ -2857,7 +2865,13 @@ start:
>           * restore/migrate-receive it again.
>           */
>          restoring = 0;
> -    }else{
> +    } else if ( domid_soft_reset != INVALID_DOMID ) {

Coding style. No space afer '(' and before ')'.

> +        /* Do soft reset. */
> +        ret = libxl_domain_soft_reset(ctx, &d_config, domid_soft_reset,
> +                                      0, autoconnect_console_how);
> +        domid = domid_soft_reset;
> +        domid_soft_reset = INVALID_DOMID;
> +    } else {
>          ret = libxl_domain_create_new(ctx, &d_config, &domid,
>                                        0, autoconnect_console_how);
>      }
> @@ -2918,8 +2932,13 @@ start:
>                  event->u.domain_shutdown.shutdown_reason,
>                  event->u.domain_shutdown.shutdown_reason);
>              switch (handle_domain_death(&domid, event, &d_config)) {
> +            case DOMAIN_RESTART_SOFT_RESET:
> +                domid_soft_reset = domid;
> +                domid = INVALID_DOMID;
> +                /* fall through */
>              case DOMAIN_RESTART_RENAME:
> -                if (!preserve_domain(&domid, event, &d_config)) {
> +                if (domid_soft_reset == INVALID_DOMID &&
> +                    !preserve_domain(&domid, event, &d_config)) {
>                      /* If we fail then exit leaving the old domain in place. 
> */
>                      ret = -1;
>                      goto out;
> -- 
> 2.4.3

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