| Message ID | 20221130165455.31125-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | More Oxenstored live update fixes | expand |
> On 30 Nov 2022, at 16:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > Generally speaking, the event channel local/remote port is fixed for the > lifetime of the associated domain object. The exception to this is a > secondary XS_INTRODUCE (defined to re-bind to a new event channel) which pokes > around at the domain object's internal state. > > We need to refactor the evtchn handling to support live update, so start by > moving the relevant manipulation into Domain. > > No practical change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Christian Lindig <christian.lindig@citrix.com> > CC: David Scott <dave@recoil.org> > CC: Edwin Torok <edvin.torok@citrix.com> > CC: Rob Hoes <Rob.Hoes@citrix.com> > > Note: This change deliberately doesn't reuse Domain.bind_interdomain, which is > removed by the end of the refactoring. Reviewed-by: Edwin Török <edvin.torok@citrix.com> > > v2: > * New. > --- > tools/ocaml/xenstored/domain.ml | 12 ++++++++++++ > tools/ocaml/xenstored/process.ml | 6 ++---- > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml > index ab08dcf37f62..d59a9401e211 100644 > --- a/tools/ocaml/xenstored/domain.ml > +++ b/tools/ocaml/xenstored/domain.ml > @@ -63,6 +63,18 @@ let string_of_port = function > let dump d chan = > fprintf chan "dom,%d,%nd,%d\n" d.id d.mfn d.remote_port > > +let rebind_evtchn d remote_port = > + begin match d.port with > + | None -> () > + | Some p -> Event.unbind d.eventchn p > + end; > + let local = Event.bind_interdomain d.eventchn d.id remote_port in > + debug "domain %d rebind (l %s, r %d) => (l %d, r %d)" > + d.id (string_of_port d.port) d.remote_port > + (Xeneventchn.to_int local) remote_port; > + d.remote_port <- remote_port; > + d.port <- Some (local) > + > let notify dom = > match dom.port with > | None -> warn "domain %d: attempt to notify on unknown port" dom.id > diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml > index b2973aca2a82..2ea940d7e2d5 100644 > --- a/tools/ocaml/xenstored/process.ml > +++ b/tools/ocaml/xenstored/process.ml > @@ -567,11 +567,9 @@ let do_introduce con t domains cons data = > let dom = > if Domains.exist domains domid then > let edom = Domains.find domains domid in > - if (Domain.get_mfn edom) = mfn && (Connections.find_domain cons domid) != con then begin > + if (Domain.get_mfn edom) = mfn && (Connections.find_domain cons domid) != con then > (* Use XS_INTRODUCE for recreating the xenbus event-channel. *) > - edom.remote_port <- remote_port; > - Domain.bind_interdomain edom; > - end; > + Domain.rebind_evtchn edom remote_port; > edom > else try > let ndom = Domains.create domains domid mfn remote_port in > -- > 2.11.0 >
> On 30 Nov 2022, at 16:54, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > Generally speaking, the event channel local/remote port is fixed for the > lifetime of the associated domain object. The exception to this is a > secondary XS_INTRODUCE (defined to re-bind to a new event channel) which pokes > around at the domain object's internal state. > > We need to refactor the evtchn handling to support live update, so start by > moving the relevant manipulation into Domain. > > No practical change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Christian Lindig <christian.lindig@citrix.com> > CC: David Scott <dave@recoil.org> > CC: Edwin Torok <edvin.torok@citrix.com> > CC: Rob Hoes <Rob.Hoes@citrix.com> Acked-by: Christian Lindig <christian.lindig@citrix.com> The code makes changes around if-expressions and it is easy to get mislead by indentation which parts are covered by an if and which are not in the presence of sequential code. I would be more confident about this with automatic formatting (but also believe this is correct). — C > Note: This change deliberately doesn't reuse Domain.bind_interdomain, which is > removed by the end of the refactoring. > > v2: > * New. > --- > tools/ocaml/xenstored/domain.ml | 12 ++++++++++++ > tools/ocaml/xenstored/process.ml | 6 ++---- > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml > index ab08dcf37f62..d59a9401e211 100644 > --- a/tools/ocaml/xenstored/domain.ml > +++ b/tools/ocaml/xenstored/domain.ml > @@ -63,6 +63,18 @@ let string_of_port = function > let dump d chan = > fprintf chan "dom,%d,%nd,%d\n" d.id d.mfn d.remote_port > > +let rebind_evtchn d remote_port = > + begin match d.port with > + | None -> () > + | Some p -> Event.unbind d.eventchn p > + end; > + let local = Event.bind_interdomain d.eventchn d.id remote_port in > + debug "domain %d rebind (l %s, r %d) => (l %d, r %d)" > + d.id (string_of_port d.port) d.remote_port > + (Xeneventchn.to_int local) remote_port; > + d.remote_port <- remote_port; > + d.port <- Some (local) > + > let notify dom = > match dom.port with > | None -> warn "domain %d: attempt to notify on unknown port" dom.id > diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml > index b2973aca2a82..2ea940d7e2d5 100644 > --- a/tools/ocaml/xenstored/process.ml > +++ b/tools/ocaml/xenstored/process.ml > @@ -567,11 +567,9 @@ let do_introduce con t domains cons data = > let dom = > if Domains.exist domains domid then > let edom = Domains.find domains domid in > - if (Domain.get_mfn edom) = mfn && (Connections.find_domain cons domid) != con then begin > + if (Domain.get_mfn edom) = mfn && (Connections.find_domain cons domid) != con then > (* Use XS_INTRODUCE for recreating the xenbus event-channel. *) > - edom.remote_port <- remote_port; > - Domain.bind_interdomain edom; > - end; > + Domain.rebind_evtchn edom remote_port; > edom > else try > let ndom = Domains.create domains domid mfn remote_port in > -- > 2.11.0 >
On 01/12/2022 11:20, Christian Lindig wrote: > >> On 30 Nov 2022, at 16:54, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: >> >> Generally speaking, the event channel local/remote port is fixed for the >> lifetime of the associated domain object. The exception to this is a >> secondary XS_INTRODUCE (defined to re-bind to a new event channel) which pokes >> around at the domain object's internal state. >> >> We need to refactor the evtchn handling to support live update, so start by >> moving the relevant manipulation into Domain. >> >> No practical change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Christian Lindig <christian.lindig@citrix.com> >> CC: David Scott <dave@recoil.org> >> CC: Edwin Torok <edvin.torok@citrix.com> >> CC: Rob Hoes <Rob.Hoes@citrix.com> > Acked-by: Christian Lindig <christian.lindig@citrix.com> Thanks. > The code makes changes around if-expressions and it is easy to get mislead by indentation which parts are covered by an if and which are not in the presence of sequential code. I would be more confident about this with automatic formatting (but also believe this is correct). I can keep the being/end if you'd prefer. Looking at the end result, it would actually shrink the patch, so is probably worth doing anyway for clarity. The net result is: diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml index b2973aca2a82..1c80e7198dbe 100644 --- a/tools/ocaml/xenstored/process.ml +++ b/tools/ocaml/xenstored/process.ml @@ -569,8 +569,7 @@ let do_introduce con t domains cons data = let edom = Domains.find domains domid in if (Domain.get_mfn edom) = mfn && (Connections.find_domain cons domid) != con then begin (* Use XS_INTRODUCE for recreating the xenbus event-channel. *) - edom.remote_port <- remote_port; - Domain.bind_interdomain edom; + Domain.rebind_evtchn edom remote_port; end; edom else try I'm happy to adjust on commit. When I started this, I tried rearranging things to avoid the "if exists then find" pattern, but quickly got into a mess, then realised that this is (almost) a dead logic path... I've got no idea why this is supported; looking through history, I can't find a case where a redundant XS_INTRODUCE was ever used, but its a common behaviour between C and O so there was clearly some reason... ~Andrew
> On 1 Dec 2022, at 12:10, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > I can keep the being/end if you'd prefer. > > Looking at the end result, it would actually shrink the patch, so is > probably worth doing anyway for clarity. The net result is: I think keeping the begin/end is a good idea - as it keeps the patch small. I was mostly arguing for automated formatting because in OCaml the unfortunate difference in what constitutes the resulting expression in if vs. match has lead to subtle bugs in the past. — C
> On 1 Dec 2022, at 12:10, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > On 01/12/2022 11:20, Christian Lindig wrote: >> >>> On 30 Nov 2022, at 16:54, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: >>> >>> Generally speaking, the event channel local/remote port is fixed for the >>> lifetime of the associated domain object. The exception to this is a >>> secondary XS_INTRODUCE (defined to re-bind to a new event channel) which pokes >>> around at the domain object's internal state. >>> >>> We need to refactor the evtchn handling to support live update, so start by >>> moving the relevant manipulation into Domain. >>> >>> No practical change. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Christian Lindig <christian.lindig@citrix.com> >>> CC: David Scott <dave@recoil.org> >>> CC: Edwin Torok <edvin.torok@citrix.com> >>> CC: Rob Hoes <Rob.Hoes@citrix.com> >> Acked-by: Christian Lindig <christian.lindig@citrix.com> > > Thanks. > >> The code makes changes around if-expressions and it is easy to get mislead by indentation which parts are covered by an if and which are not in the presence of sequential code. I would be more confident about this with automatic formatting (but also believe this is correct). > > I can keep the being/end if you'd prefer. > > Looking at the end result, it would actually shrink the patch, so is > probably worth doing anyway for clarity. The net result is: > > diff --git a/tools/ocaml/xenstored/process.ml > b/tools/ocaml/xenstored/process.ml > index b2973aca2a82..1c80e7198dbe 100644 > --- a/tools/ocaml/xenstored/process.ml > +++ b/tools/ocaml/xenstored/process.ml > @@ -569,8 +569,7 @@ let do_introduce con t domains cons data = > let edom = Domains.find domains domid in > if (Domain.get_mfn edom) = mfn && > (Connections.find_domain cons domid) != con then begin > (* Use XS_INTRODUCE for recreating the > xenbus event-channel. *) > - edom.remote_port <- remote_port; > - Domain.bind_interdomain edom; > + Domain.rebind_evtchn edom remote_port; > end; > edom > else try > > I'm happy to adjust on commit. > > When I started this, I tried rearranging things to avoid the "if exists > then find" pattern, but quickly got into a mess, then realised that this > is (almost) a dead logic path... I've got no idea why this is supported; > looking through history, I can't find a case where a redundant > XS_INTRODUCE was ever used, but its a common behaviour between C and O > so there was clearly some reason... Currently the soft reset code in xenopsd uses it, but as you say there must've been another reason too (the soft reset code is a lot more recent than this). Best regards, --Edwin
diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml index ab08dcf37f62..d59a9401e211 100644 --- a/tools/ocaml/xenstored/domain.ml +++ b/tools/ocaml/xenstored/domain.ml @@ -63,6 +63,18 @@ let string_of_port = function let dump d chan = fprintf chan "dom,%d,%nd,%d\n" d.id d.mfn d.remote_port +let rebind_evtchn d remote_port = + begin match d.port with + | None -> () + | Some p -> Event.unbind d.eventchn p + end; + let local = Event.bind_interdomain d.eventchn d.id remote_port in + debug "domain %d rebind (l %s, r %d) => (l %d, r %d)" + d.id (string_of_port d.port) d.remote_port + (Xeneventchn.to_int local) remote_port; + d.remote_port <- remote_port; + d.port <- Some (local) + let notify dom = match dom.port with | None -> warn "domain %d: attempt to notify on unknown port" dom.id diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml index b2973aca2a82..2ea940d7e2d5 100644 --- a/tools/ocaml/xenstored/process.ml +++ b/tools/ocaml/xenstored/process.ml @@ -567,11 +567,9 @@ let do_introduce con t domains cons data = let dom = if Domains.exist domains domid then let edom = Domains.find domains domid in - if (Domain.get_mfn edom) = mfn && (Connections.find_domain cons domid) != con then begin + if (Domain.get_mfn edom) = mfn && (Connections.find_domain cons domid) != con then (* Use XS_INTRODUCE for recreating the xenbus event-channel. *) - edom.remote_port <- remote_port; - Domain.bind_interdomain edom; - end; + Domain.rebind_evtchn edom remote_port; edom else try let ndom = Domains.create domains domid mfn remote_port in
Generally speaking, the event channel local/remote port is fixed for the lifetime of the associated domain object. The exception to this is a secondary XS_INTRODUCE (defined to re-bind to a new event channel) which pokes around at the domain object's internal state. We need to refactor the evtchn handling to support live update, so start by moving the relevant manipulation into Domain. No practical change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Christian Lindig <christian.lindig@citrix.com> CC: David Scott <dave@recoil.org> CC: Edwin Torok <edvin.torok@citrix.com> CC: Rob Hoes <Rob.Hoes@citrix.com> Note: This change deliberately doesn't reuse Domain.bind_interdomain, which is removed by the end of the refactoring. v2: * New. --- tools/ocaml/xenstored/domain.ml | 12 ++++++++++++ tools/ocaml/xenstored/process.ml | 6 ++---- 2 files changed, 14 insertions(+), 4 deletions(-)