Re: [Xen-devel] [PATCH RFC 0/4] 'reset everything' approach to PVHVM guest kexec

[Dave Scott <Dave.Scott@xxxxxxxxxx>]

> On 8 Jun 2015, at 18:43, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> 
> On Mon, Jun 08, 2015 at 04:53:46PM +0100, Wei Liu wrote:
>> On Wed, Jun 03, 2015 at 03:35:18PM +0200, Vitaly Kuznetsov wrote:
>>> Jan and Tim,
>>> 
>>> last week you expressed some concerns about if the toolstack-based
>>> approach to PVHVM guest kexec is the best. Here you can see the 'reset
>>> everything' approach to the same problem. It is the bare minimum of what
>>> should be done to make it possible for the new kernel to boot. Despite
>>> the fact that this implementation is much smaller in size and that it
>>> doesn't require any toolstack changes I'm personally in favor of the
>>> previous toolstack-based approach as it looks more general, less fragile
>>> and easier to support to me. Please let me know your thoughts.
>>> 
>>> I used SCHEDOP_ interface here as DOMCTL_* is not currently supported in
>>> Linux kernel and I seriously doubt we need to support something different
>>> from DOMID_SELF for soft reset.
>>> 
>>> Current Linux kernel requires two changes to make use of these hypervisor
>>> changes:
>>> 1) As XS_RESET_WATCHES is not supported by oxenstored we need to try 
>>> removing
>>> the watch in case add operation failed, e.g.:
>> 
>> The changeset to implement XS_RESET_WATCHES in cxenstored is
>> 1f9d04fb021cbf35cc420d401a88c696d6524c14
>> 
>> It doesn't look too complicated to do that in oxenstored. Dave
>> (oxenstored maintainer, CC'ed) might have insight.
>> 
>> Wei.
>> 
> 
> And I took a stab at it. Here is my oxenstored patch. May contain
> bugs. :-)

That was quick :-)

I have only one question, see below:

> 
> ---8<---
> From 8dd35370442340493f856b0be8d99c87243e79f6 Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@xxxxxxxxxx>
> Date: Mon, 8 Jun 2015 18:39:45 +0100
> Subject: [PATCH] oxenstored: implement XS_RESET_WATCHES
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> tools/ocaml/libs/xb/op.ml           | 6 ++++--
> tools/ocaml/libs/xb/xb.mli          | 1 +
> tools/ocaml/xenstored/connection.ml | 8 ++++++++
> tools/ocaml/xenstored/process.ml    | 6 ++++++
> 4 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/ocaml/libs/xb/op.ml b/tools/ocaml/libs/xb/op.ml
> index 0ee8666..69346d8 100644
> --- a/tools/ocaml/libs/xb/op.ml
> +++ b/tools/ocaml/libs/xb/op.ml
> @@ -19,7 +19,8 @@ type operation = Debug | Directory | Read | Getperms |
>                  Transaction_end | Introduce | Release |
>                  Getdomainpath | Write | Mkdir | Rm |
>                  Setperms | Watchevent | Error | Isintroduced |
> -                 Resume | Set_target | Restrict | Invalid
> +                 Resume | Set_target | Restrict | Reset_watches |
> +                 Invalid
> 
> let operation_c_mapping =
>       [| Debug; Directory; Read; Getperms;
> @@ -27,7 +28,7 @@ let operation_c_mapping =
>            Transaction_end; Introduce; Release;
>            Getdomainpath; Write; Mkdir; Rm;
>            Setperms; Watchevent; Error; Isintroduced;
> -           Resume; Set_target; Restrict |]
> +           Resume; Set_target; Restrict; Reset_watches |]
> let size = Array.length operation_c_mapping
> 
> let array_search el a =
> @@ -68,4 +69,5 @@ let to_string ty =
>       | Resume                -> "RESUME"
>       | Set_target            -> "SET_TARGET"
>       | Restrict              -> "RESTRICT"
> +     | Reset_watches         -> "RESET_WATCHES"
>       | Invalid               -> "INVALID"
> diff --git a/tools/ocaml/libs/xb/xb.mli b/tools/ocaml/libs/xb/xb.mli
> index 4e1f833..6c242da 100644
> --- a/tools/ocaml/libs/xb/xb.mli
> +++ b/tools/ocaml/libs/xb/xb.mli
> @@ -23,6 +23,7 @@ module Op :
>       | Resume
>       | Set_target
>       | Restrict
> +      | Reset_watches
>       | Invalid
>     val operation_c_mapping : operation array
>     val size : int
> diff --git a/tools/ocaml/xenstored/connection.ml 
> b/tools/ocaml/xenstored/connection.ml
> index b4dc9cb..5e31588 100644
> --- a/tools/ocaml/xenstored/connection.ml
> +++ b/tools/ocaml/xenstored/connection.ml
> @@ -186,6 +186,14 @@ let del_watch con path token =
>       con.nb_watches <- con.nb_watches - 1;
>       apath, w
> 
> +let del_watches con =
> +  Hashtbl.clear con.watches;
> +  con.next_tid <- initial_next_tid

I couldnât spot the part in the C version where the next transaction id is 
reset â is this a (minor) difference between the two implementations, or have I 
misread the code?

If you could clarify that for me, and assuming this all compiled ok, then feel 
free to add

Acked-by: David Scott <dave.scott@xxxxxxxxxx>

Cheers,
Dave

> +
> +let del_transactions con =
> +  Hashtbl.clear con.transactions;
> +  con.nb_watches <- 0
> +
> let list_watches con =
>       let ll = Hashtbl.fold 
>               (fun _ watches acc -> List.map (fun watch -> watch.path, 
> watch.token) watches :: acc)
> diff --git a/tools/ocaml/xenstored/process.ml 
> b/tools/ocaml/xenstored/process.ml
> index 0620585..e827678 100644
> --- a/tools/ocaml/xenstored/process.ml
> +++ b/tools/ocaml/xenstored/process.ml
> @@ -272,6 +272,11 @@ let do_restrict con t domains cons data =
>       in
>       Connection.restrict con domid
> 
> +(* only in xen >= 4.2 *)
> +let do_reset_watches con t domains cons data =
> +  Connection.del_watches con;
> +  Connection.del_transactions con
> +
> (* only in >= xen3.3                                                          
>                           *)
> (* we ensure backward compatibility with restrict by counting the number of 
> argument of set_target ...  *)
> (* This is not very elegant, but it is safe as 'restrict' only restricts 
> permission of dom0 connections *)
> @@ -324,6 +329,7 @@ let function_of_type ty =
>       | Xenbus.Xb.Op.Resume            -> reply_ack do_resume
>       | Xenbus.Xb.Op.Set_target        -> reply_ack do_set_target
>       | Xenbus.Xb.Op.Restrict          -> reply_ack do_restrict
> +     | Xenbus.Xb.Op.Reset_watches     -> reply_ack do_reset_watches
>       | Xenbus.Xb.Op.Invalid           -> reply_ack do_error
>       | _                              -> reply_ack do_error
> 
> -- 
> 1.9.1

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