[Xen-devel] [PATCH v2] xen-netback: fix error handling on netback_probe()
In case of error during netback_probe() (e.g. an entry missing on the
xenstore) netback_remove() is called on the new device, which will set
the device backend state to XenbusStateClosed by calling
set_backend_state(). However, the backend state wasn't initialized by
netback_probe() at this point, which will cause and invalid transaction
and set_backend_state() to BUG().
Initialize the backend state at the beginning of netback_probe() to
XenbusStateInitialising, and create two new valid state transitions on
set_backend_state(), from XenbusStateInitialising to XenbusStateClosed,
and from XenbusStateInitialising to XenbusStateInitWait.
Signed-off-by: Filipe Manco <filipe.ma...@neclab.eu>
---
drivers/net/xen-netback/xenbus.c | 46 ++--
1 file changed, 30 insertions(+), 16 deletions(-)
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 6a31f2610c23..daf4c7867102 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -271,6 +271,11 @@ static int netback_probe(struct xenbus_device *dev,
be->dev = dev;
dev_set_drvdata(>dev, be);
+ be->state = XenbusStateInitialising;
+ err = xenbus_switch_state(dev, XenbusStateInitialising);
+ if (err)
+ goto fail;
+
sg = 1;
do {
@@ -383,11 +388,6 @@ static int netback_probe(struct xenbus_device *dev,
be->hotplug_script = script;
- err = xenbus_switch_state(dev, XenbusStateInitWait);
- if (err)
- goto fail;
-
- be->state = XenbusStateInitWait;
/* This kicks hotplug scripts, so do it immediately. */
err = backend_create_xenvif(be);
@@ -492,20 +492,20 @@ static inline void backend_switch_state(struct
backend_info *be,
/* Handle backend state transitions:
*
- * The backend state starts in InitWait and the following transitions are
+ * The backend state starts in Initialising and the following transitions are
* allowed.
*
- * InitWait -> Connected
- *
- *^\ |
- *| \|
- *| \ |
- *| \ |
- *|\ |
- *| \|
- *| V V
+ * Initialising -> InitWait -> Connected
+ * \
+ * \^\ |
+ *\ | \|
+ * \ | \ |
+ * \ | \ |
+ * \|\ |
+ *\ | \|
+ * V | V V
*
- * Closed <-> Closing
+ * Closed <-> Closing
*
* The state argument specifies the eventual state of the backend and the
* function transitions to that state via the shortest path.
@@ -515,6 +515,20 @@ static void set_backend_state(struct backend_info *be,
{
while (be->state != state) {
switch (be->state) {
+ case XenbusStateInitialising:
+ switch (state) {
+ case XenbusStateInitWait:
+ case XenbusStateConnected:
+ case XenbusStateClosing:
+ backend_switch_state(be, XenbusStateInitWait);
+ break;
+ case XenbusStateClosed:
+ backend_switch_state(be, XenbusStateClosed);
+ break;
+ default:
+ BUG();
+ }
+ break;
case XenbusStateClosed:
switch (state) {
case XenbusStateInitWait:
--
2.7.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] xen-netback: fix error handling on netback_probe()
On 14-09-2016 12:10, Wei Liu wrote:
CC xen-devel as well.
On Tue, Sep 13, 2016 at 02:11:27PM +0200, Filipe Manco wrote:
In case of error during netback_probe() (e.g. an entry missing on the
xenstore) netback_remove() is called on the new device, which will set
the device backend state to XenbusStateClosed by calling
set_backend_state(). However, the backend state wasn't initialized by
netback_probe() at this point, which will cause and invalid transaction
and set_backend_state() to BUG().
Initialize the backend state at the beginning of netback_probe() to
XenbusStateInitialising, and create a new valid state transaction on
set_backend_state(), from XenbusStateInitialising to XenbusStateClosed.
Signed-off-by: Filipe Manco <filipe.ma...@neclab.eu>
There is a state machine right before set_backend_state. You would also
need to update that.
Good point I'll update the diagram.
After looking at the diagram and for consistency, shouldn't the transition
Initialising -> InitWait be handled using set_backend_state()? Currently it
is done directly in netback_probe() code. If you agree I'll submit a v2 with
these two changes.
According to the definition of XenbusStateInitialising, this patch looks
plausible to me.
Wei.
Filipe
---
drivers/net/xen-netback/xenbus.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 6a31f2610c23..c0e5f6994d01 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -270,6 +270,7 @@ static int netback_probe(struct xenbus_device *dev,
be->dev = dev;
dev_set_drvdata(>dev, be);
+ be->state = XenbusStateInitialising;
sg = 1;
@@ -515,6 +516,15 @@ static void set_backend_state(struct backend_info *be,
{
while (be->state != state) {
switch (be->state) {
+ case XenbusStateInitialising:
+ switch (state) {
+ case XenbusStateClosed:
+ backend_switch_state(be, XenbusStateClosed);
+ break;
+ default:
+ BUG();
+ }
+ break;
case XenbusStateClosed:
switch (state) {
case XenbusStateInitWait:
--
2.7.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] 2016 Xen hackathon notes - xenstored
Hi Regarding LiXS, our goal is to make it one of the upstream xenstore alternatives. For that I already started getting internal approvals to release the code open source, which should happen somewhere around next month. I also need to fix some bugs and would like to do some performance testing before the release. Once it is released, it would be nice to get some comments from the community on the implementation, specifically about how to make it upstreamable; I’ll let you know when the code is available. Does this plan sound reasonable? Best regards Filipe Manco P.S. The code will be released under a BSD License On 28-04-2016 20:34, Luis R. Rodriguez wrote: At the 2016 Xen Hackathon I raised the topic of the default xenstored used. Here are my notes with some new additions and supported documentation. It would seem we're moving to oxenstored as the default on Linux distributions and FreeBSD now, if you have issues or concerns with this please let us know. Notes: = Although we have had oxenstored be the the default *iff* you have ocaml dev libs installled when compiling Xen from source both Linux distributions (Debian, SUSE*, Gentoo) and FreeBSD were still using the C xenstored as the default. This begged the question of why not make the switch given Citrix has already been using oxensotred in production for years with these known gains [0]: * 1/5th the size in terms of line of code in comparison to the C xenstored * better performance increasing support for the number of guests, it supports 3 times number of guests for an upper limit of 160 guests At the 2014 summit Anil's presented work on a Xenstore 2.0 which hinted also towards the future ability to provide git-like capabilities for the xenstore, all still written in Ocaml [1]. There are others who have worked on a C++ replacement lixs (Lightweight XenStore) as well [2], such work revealed oxenstored had the CPU pegged after just a few dozen guests. Such work hinted that the oxenstored that should be considered for more serious work was the Mirage OS xenstore [3], but that the C++ lixs was also performing better than the current oxenstored. Although there are questions about the future of the Xenestore we know oxenstored performs better than cxenstored and since we are building and using it by default already it begged the question why haven't distributions made the switch to use it by default. Since Mirage OS work seems promising we agreed to just set oxenstored as the default in distributions and in the future hope that Mirage's work or other contending efforts make it upstream to consider them as alternatives. Given Mirage Xenstore would still require ocaml, it would be a good stepping stone now to just use oxenstored by default more widely on Linux distributions and FreeBSD. A concern was raised about expertise over Ocaml, however it would seem that we will have no option but to rely on the community / folks supporting upstream oxenstored for this. [0] http://www.do-not-panic.com/2014/04/summary-of-gains-of-xen-oxenstored.html [1] http://decks.openmirage.org/xendevsummit14#/ [2] http://events.linuxfoundation.org/sites/events/files/slides/xendevsummit14_0.pdf [3] https://github.com/mirage/ocaml-xenstore Luis ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
