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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Jun 09, 2015 at 09:19:19AM +0100, Dave Scott wrote:
> 
> > 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
> 

Yes, it compiles OK.

I think I will just delete that line. I couldn't recollect why I added
that. Maybe it's copy-n-paste error.

Can I still have your ack if I drop that line?

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

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