* [PATCH net v5 0/1] xen-netback: windows frontend compatibility fixes
@ 2013-09-25 9:59 Paul Durrant
2013-09-25 9:59 ` [PATCH net v5 1/1] xen-netback: Handle backend state transitions in a more robust way Paul Durrant
0 siblings, 1 reply; 5+ messages in thread
From: Paul Durrant @ 2013-09-25 9:59 UTC (permalink / raw)
To: xen-devel, netdev
The following patches fix a couple more issues found when testing with
Windows frontends.
v5:
- Fix typos pointed out by Wei Liu
v4:
- Fix problem with hotplug failure noticed by Wei Liu
v3:
- Collapse both v2 patches into a single patch that introduces a new
function to handle backend state transtions. By doing this we ensure that
we always transition through intermediate states and that we don't attempt
repeated connects or disconnects.
v2:
- Add comment in 2/2 to note that state transitions from Connected to Closed
are incorrect.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net v5 1/1] xen-netback: Handle backend state transitions in a more robust way
2013-09-25 9:59 [PATCH net v5 0/1] xen-netback: windows frontend compatibility fixes Paul Durrant
@ 2013-09-25 9:59 ` Paul Durrant
2013-09-26 10:17 ` Ian Campbell
0 siblings, 1 reply; 5+ messages in thread
From: Paul Durrant @ 2013-09-25 9:59 UTC (permalink / raw)
To: xen-devel, netdev; +Cc: Paul Durrant, Ian Campbell, Wei Liu, David Vrabel
When the frontend state changes netback now specifies its desired state to
a new function, set_backend_state(), which transitions through any
necessary intermediate states.
This fixes an issue observed with some old Windows frontend drivers where
they failed to transition through the Closing state and netback would not
behave correctly.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
drivers/net/xen-netback/xenbus.c | 143 ++++++++++++++++++++++++++++++--------
1 file changed, 113 insertions(+), 30 deletions(-)
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index a53782e..01329b2 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -24,6 +24,7 @@
struct backend_info {
struct xenbus_device *dev;
struct xenvif *vif;
+ enum xenbus_state state;
enum xenbus_state frontend_state;
struct xenbus_watch hotplug_status_watch;
u8 have_hotplug_status_watch:1;
@@ -136,6 +137,8 @@ static int netback_probe(struct xenbus_device *dev,
if (err)
goto fail;
+ be->state = XenbusStateInitWait;
+
/* This kicks hotplug scripts, so do it immediately. */
backend_create_xenvif(be);
@@ -208,24 +211,113 @@ static void backend_create_xenvif(struct backend_info *be)
kobject_uevent(&dev->dev.kobj, KOBJ_ONLINE);
}
-
-static void disconnect_backend(struct xenbus_device *dev)
+static void backend_disconnect(struct backend_info *be)
{
- struct backend_info *be = dev_get_drvdata(&dev->dev);
-
if (be->vif)
xenvif_disconnect(be->vif);
}
-static void destroy_backend(struct xenbus_device *dev)
+static void backend_connect(struct backend_info *be)
{
- struct backend_info *be = dev_get_drvdata(&dev->dev);
+ if (be->vif)
+ connect(be);
+}
- if (be->vif) {
- kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
- xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
- xenvif_free(be->vif);
- be->vif = NULL;
+static inline void backend_switch_state(struct backend_info *be,
+ enum xenbus_state state)
+{
+ struct xenbus_device *dev = be->dev;
+
+ pr_debug("%s -> %s\n", dev->nodename, xenbus_strstate(state));
+ be->state = state;
+
+ /* If we are waiting for a hotplug script then defer the
+ * actual xenbus state change.
+ */
+ if (!be->have_hotplug_status_watch)
+ xenbus_switch_state(dev, state);
+}
+
+/* Handle backend state transitions:
+ *
+ * The backend state starts in InitWait and the following transitions are
+ * allowed.
+ *
+ * InitWait -> Connected
+ *
+ * ^ \ |
+ * | \ |
+ * | \ |
+ * | \ |
+ * | \ |
+ * | \ |
+ * | V V
+ *
+ * Closed <-> Closing
+ *
+ * The state argument specifies the eventual state of the backend and the
+ * function transitions to that state via the shortest path.
+ */
+static void set_backend_state(struct backend_info *be,
+ enum xenbus_state state)
+{
+ while (be->state != state) {
+ switch (be->state) {
+ case XenbusStateClosed:
+ switch (state) {
+ case XenbusStateInitWait:
+ case XenbusStateConnected:
+ pr_info("%s: prepare for reconnect\n",
+ be->dev->nodename);
+ backend_switch_state(be, XenbusStateInitWait);
+ break;
+ case XenbusStateClosing:
+ backend_switch_state(be, XenbusStateClosing);
+ break;
+ default:
+ BUG();
+ }
+ break;
+ case XenbusStateInitWait:
+ switch (state) {
+ case XenbusStateConnected:
+ backend_connect(be);
+ backend_switch_state(be, XenbusStateConnected);
+ break;
+ case XenbusStateClosing:
+ case XenbusStateClosed:
+ backend_switch_state(be, XenbusStateClosing);
+ break;
+ default:
+ BUG();
+ }
+ break;
+ case XenbusStateConnected:
+ switch (state) {
+ case XenbusStateInitWait:
+ case XenbusStateClosing:
+ case XenbusStateClosed:
+ backend_disconnect(be);
+ backend_switch_state(be, XenbusStateClosing);
+ break;
+ default:
+ BUG();
+ }
+ break;
+ case XenbusStateClosing:
+ switch (state) {
+ case XenbusStateInitWait:
+ case XenbusStateConnected:
+ case XenbusStateClosed:
+ backend_switch_state(be, XenbusStateClosed);
+ break;
+ default:
+ BUG();
+ }
+ break;
+ default:
+ BUG();
+ }
}
}
@@ -237,40 +329,33 @@ static void frontend_changed(struct xenbus_device *dev,
{
struct backend_info *be = dev_get_drvdata(&dev->dev);
- pr_debug("frontend state %s\n", xenbus_strstate(frontend_state));
+ pr_debug("%s -> %s\n", dev->otherend, xenbus_strstate(frontend_state));
be->frontend_state = frontend_state;
switch (frontend_state) {
case XenbusStateInitialising:
- if (dev->state == XenbusStateClosed) {
- pr_info("%s: prepare for reconnect\n", dev->nodename);
- xenbus_switch_state(dev, XenbusStateInitWait);
- }
+ set_backend_state(be, XenbusStateInitWait);
break;
case XenbusStateInitialised:
break;
case XenbusStateConnected:
- if (dev->state == XenbusStateConnected)
- break;
- if (be->vif)
- connect(be);
+ set_backend_state(be, XenbusStateConnected);
break;
case XenbusStateClosing:
- disconnect_backend(dev);
- xenbus_switch_state(dev, XenbusStateClosing);
+ set_backend_state(be, XenbusStateClosing);
break;
case XenbusStateClosed:
- xenbus_switch_state(dev, XenbusStateClosed);
+ set_backend_state(be, XenbusStateClosed);
if (xenbus_dev_is_online(dev))
break;
- destroy_backend(dev);
/* fall through if not online */
case XenbusStateUnknown:
+ set_backend_state(be, XenbusStateClosed);
device_unregister(&dev->dev);
break;
@@ -363,7 +448,9 @@ static void hotplug_status_changed(struct xenbus_watch *watch,
if (IS_ERR(str))
return;
if (len == sizeof("connected")-1 && !memcmp(str, "connected", len)) {
- xenbus_switch_state(be->dev, XenbusStateConnected);
+ /* Complete any pending state change */
+ xenbus_switch_state(be->dev, be->state);
+
/* Not interested in this watch anymore. */
unregister_hotplug_status_watch(be);
}
@@ -393,12 +480,8 @@ static void connect(struct backend_info *be)
err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch,
hotplug_status_changed,
"%s/%s", dev->nodename, "hotplug-status");
- if (err) {
- /* Switch now, since we can't do a watch. */
- xenbus_switch_state(dev, XenbusStateConnected);
- } else {
+ if (!err)
be->have_hotplug_status_watch = 1;
- }
netif_wake_queue(be->vif->dev);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net v5 1/1] xen-netback: Handle backend state transitions in a more robust way
2013-09-25 9:59 ` [PATCH net v5 1/1] xen-netback: Handle backend state transitions in a more robust way Paul Durrant
@ 2013-09-26 10:17 ` Ian Campbell
2013-09-26 10:51 ` Paul Durrant
0 siblings, 1 reply; 5+ messages in thread
From: Ian Campbell @ 2013-09-26 10:17 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, netdev, Wei Liu, David Vrabel
On Wed, 2013-09-25 at 10:59 +0100, Paul Durrant wrote:
> When the frontend state changes netback now specifies its desired state to
> a new function, set_backend_state(), which transitions through any
> necessary intermediate states.
> This fixes an issue observed with some old Windows frontend drivers where
> they failed to transition through the Closing state and netback would not
> behave correctly.
I'm somewhat terrified of breaking compatibility with some old or
obscure frontend here. I don't think it is reasonable to try and test
all of those so I guess we'll just have to deal with that when it
happens...
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> ---
> drivers/net/xen-netback/xenbus.c | 143 ++++++++++++++++++++++++++++++--------
> 1 file changed, 113 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index a53782e..01329b2 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -24,6 +24,7 @@
> struct backend_info {
> struct xenbus_device *dev;
> struct xenvif *vif;
> + enum xenbus_state state;
This cannot just be read from xenstore because of the
wait-for-hotplug-script deferral stuff, right?
I'm not sure it is worth clarifying that, either by a comment or by
naming it deferred_state_change or something?
> -static void destroy_backend(struct xenbus_device *dev)
> +static void backend_connect(struct backend_info *be)
> {
> - struct backend_info *be = dev_get_drvdata(&dev->dev);
> + if (be->vif)
> + connect(be);
> +}
>
> - if (be->vif) {
> - kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
> - xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
> - xenvif_free(be->vif);
> - be->vif = NULL;
Where did this uevent offlining functionality end up?
> +static inline void backend_switch_state(struct backend_info *be,
> + enum xenbus_state state)
> +{
> + struct xenbus_device *dev = be->dev;
> +
> + pr_debug("%s -> %s\n", dev->nodename, xenbus_strstate(state));
> + be->state = state;
> +
> + /* If we are waiting for a hotplug script then defer the
> + * actual xenbus state change.
Is the right/wise regardless of the state we are moving too? Might the
state change have effectively "cancelled" the need to wait for the
script?
The previous code only deferred when moving to Connected. If we are now
moving to Closed/Closing do we still need to wait?
Ian.
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH net v5 1/1] xen-netback: Handle backend state transitions in a more robust way
2013-09-26 10:17 ` Ian Campbell
@ 2013-09-26 10:51 ` Paul Durrant
2013-09-26 11:05 ` Ian Campbell
0 siblings, 1 reply; 5+ messages in thread
From: Paul Durrant @ 2013-09-26 10:51 UTC (permalink / raw)
To: Ian Campbell
Cc: xen-devel@lists.xen.org, netdev@vger.kernel.org, Wei Liu,
David Vrabel
> -----Original Message-----
> From: Ian Campbell
> Sent: 26 September 2013 11:17
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; David Vrabel
> Subject: Re: [PATCH net v5 1/1] xen-netback: Handle backend state
> transitions in a more robust way
>
> On Wed, 2013-09-25 at 10:59 +0100, Paul Durrant wrote:
> > When the frontend state changes netback now specifies its desired state
> to
> > a new function, set_backend_state(), which transitions through any
> > necessary intermediate states.
> > This fixes an issue observed with some old Windows frontend drivers
> where
> > they failed to transition through the Closing state and netback would not
> > behave correctly.
>
> I'm somewhat terrified of breaking compatibility with some old or
> obscure frontend here. I don't think it is reasonable to try and test
> all of those so I guess we'll just have to deal with that when it
> happens...
>
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: David Vrabel <david.vrabel@citrix.com>
> > ---
> > drivers/net/xen-netback/xenbus.c | 143
> ++++++++++++++++++++++++++++++--------
> > 1 file changed, 113 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-
> netback/xenbus.c
> > index a53782e..01329b2 100644
> > --- a/drivers/net/xen-netback/xenbus.c
> > +++ b/drivers/net/xen-netback/xenbus.c
> > @@ -24,6 +24,7 @@
> > struct backend_info {
> > struct xenbus_device *dev;
> > struct xenvif *vif;
> > + enum xenbus_state state;
>
> This cannot just be read from xenstore because of the
> wait-for-hotplug-script deferral stuff, right?
>
Yes, that's right.
> I'm not sure it is worth clarifying that, either by a comment or by
> naming it deferred_state_change or something?
>
It’s the xenbus state that's deferred, the backend state leads so I could name it 'desired_state' or 'target_state' perhaps; probably a bit ugly so I'll add a comment above that field I think.
> > -static void destroy_backend(struct xenbus_device *dev)
> > +static void backend_connect(struct backend_info *be)
> > {
> > - struct backend_info *be = dev_get_drvdata(&dev->dev);
> > + if (be->vif)
> > + connect(be);
> > +}
> >
> > - if (be->vif) {
> > - kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
> > - xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
> > - xenvif_free(be->vif);
> > - be->vif = NULL;
>
> Where did this uevent offlining functionality end up?
>
It's done in netback_remove() .
> > +static inline void backend_switch_state(struct backend_info *be,
> > + enum xenbus_state state)
> > +{
> > + struct xenbus_device *dev = be->dev;
> > +
> > + pr_debug("%s -> %s\n", dev->nodename, xenbus_strstate(state));
> > + be->state = state;
> > +
> > + /* If we are waiting for a hotplug script then defer the
> > + * actual xenbus state change.
>
> Is the right/wise regardless of the state we are moving too? Might the
> state change have effectively "cancelled" the need to wait for the
> script?
>
> The previous code only deferred when moving to Connected. If we are now
> moving to Closed/Closing do we still need to wait?
>
I debated about this; we never really dealt with this before. I think it's legitimate for the frontend to give up waiting for a backend hotplug so we should be able to go from InitWait to Closing, so in this case be->state will be Closing when the script (hopefully) finishes. Thus we move to that when the watch fires rather than to Connected. Then the frontend can move to Closed. If the hotplug script never finishes then I don't think we're any more screwed than we were before.
Paul
>
> Ian.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v5 1/1] xen-netback: Handle backend state transitions in a more robust way
2013-09-26 10:51 ` Paul Durrant
@ 2013-09-26 11:05 ` Ian Campbell
0 siblings, 0 replies; 5+ messages in thread
From: Ian Campbell @ 2013-09-26 11:05 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel@lists.xen.org, netdev@vger.kernel.org, Wei Liu,
David Vrabel
On Thu, 2013-09-26 at 11:51 +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Ian Campbell
> > Sent: 26 September 2013 11:17
> > To: Paul Durrant
> > Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; David Vrabel
> > Subject: Re: [PATCH net v5 1/1] xen-netback: Handle backend state
> > transitions in a more robust way
> >
> > On Wed, 2013-09-25 at 10:59 +0100, Paul Durrant wrote:
> > > When the frontend state changes netback now specifies its desired state
> > to
> > > a new function, set_backend_state(), which transitions through any
> > > necessary intermediate states.
> > > This fixes an issue observed with some old Windows frontend drivers
> > where
> > > they failed to transition through the Closing state and netback would not
> > > behave correctly.
> >
> > I'm somewhat terrified of breaking compatibility with some old or
> > obscure frontend here. I don't think it is reasonable to try and test
> > all of those so I guess we'll just have to deal with that when it
> > happens...
> >
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > Cc: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > Cc: David Vrabel <david.vrabel@citrix.com>
> > > ---
> > > drivers/net/xen-netback/xenbus.c | 143
> > ++++++++++++++++++++++++++++++--------
> > > 1 file changed, 113 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-
> > netback/xenbus.c
> > > index a53782e..01329b2 100644
> > > --- a/drivers/net/xen-netback/xenbus.c
> > > +++ b/drivers/net/xen-netback/xenbus.c
> > > @@ -24,6 +24,7 @@
> > > struct backend_info {
> > > struct xenbus_device *dev;
> > > struct xenvif *vif;
> > > + enum xenbus_state state;
> >
> > This cannot just be read from xenstore because of the
> > wait-for-hotplug-script deferral stuff, right?
> >
>
> Yes, that's right.
>
> > I'm not sure it is worth clarifying that, either by a comment or by
> > naming it deferred_state_change or something?
> >
>
> It’s the xenbus state that's deferred, the backend state leads so I
> could name it 'desired_state' or 'target_state' perhaps; probably a
> bit ugly so I'll add a comment above that field I think.
OK.
>
> > > -static void destroy_backend(struct xenbus_device *dev)
> > > +static void backend_connect(struct backend_info *be)
> > > {
> > > - struct backend_info *be = dev_get_drvdata(&dev->dev);
> > > + if (be->vif)
> > > + connect(be);
> > > +}
> > >
> > > - if (be->vif) {
> > > - kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
> > > - xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
> > > - xenvif_free(be->vif);
> > > - be->vif = NULL;
> >
> > Where did this uevent offlining functionality end up?
> >
>
> It's done in netback_remove() .
>
> > > +static inline void backend_switch_state(struct backend_info *be,
> > > + enum xenbus_state state)
> > > +{
> > > + struct xenbus_device *dev = be->dev;
> > > +
> > > + pr_debug("%s -> %s\n", dev->nodename, xenbus_strstate(state));
> > > + be->state = state;
> > > +
> > > + /* If we are waiting for a hotplug script then defer the
> > > + * actual xenbus state change.
> >
> > Is the right/wise regardless of the state we are moving too? Might the
> > state change have effectively "cancelled" the need to wait for the
> > script?
> >
> > The previous code only deferred when moving to Connected. If we are now
> > moving to Closed/Closing do we still need to wait?
> >
>
> I debated about this; we never really dealt with this before. I think
> it's legitimate for the frontend to give up waiting for a backend
> hotplug so we should be able to go from InitWait to Closing, so in
> this case be->state will be Closing when the script (hopefully)
> finishes. Thus we move to that when the watch fires rather than to
> Connected. Then the frontend can move to Closed. If the hotplug script
> never finishes then I don't think we're any more screwed than we were
> before.
OK.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-26 11:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-25 9:59 [PATCH net v5 0/1] xen-netback: windows frontend compatibility fixes Paul Durrant
2013-09-25 9:59 ` [PATCH net v5 1/1] xen-netback: Handle backend state transitions in a more robust way Paul Durrant
2013-09-26 10:17 ` Ian Campbell
2013-09-26 10:51 ` Paul Durrant
2013-09-26 11:05 ` Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).