| Message ID | 20210413152512.903750-1-mbrown@fensystems.co.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2afeec08ab5c86ae21952151f726bfe184f6b23d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | xen-netback: Check for hotplug-status existence before watching | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: davem@davemloft.net kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 19 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On 13/04/2021 16:25, Michael Brown wrote: > The logic in connect() is currently written with the assumption that > xenbus_watch_pathfmt() will return an error for a node that does not > exist. This assumption is incorrect: xenstore does allow a watch to > be registered for a nonexistent node (and will send notifications > should the node be subsequently created). > > As of commit 1f2565780 ("xen-netback: remove 'hotplug-status' once it > has served its purpose"), this leads to a failure when a domU > transitions into XenbusStateConnected more than once. On the first > domU transition into Connected state, the "hotplug-status" node will > be deleted by the hotplug_status_changed() callback in dom0. On the > second or subsequent domU transition into Connected state, the > hotplug_status_changed() callback will therefore never be invoked, and > so the backend will remain stuck in InitWait. > > This failure prevents scenarios such as reloading the xen-netfront > module within a domU, or booting a domU via iPXE. There is > unfortunately no way for the domU to work around this dom0 bug. > > Fix by explicitly checking for existence of the "hotplug-status" node, > thereby creating the behaviour that was previously assumed to exist. > > Signed-off-by: Michael Brown <mbrown@fensystems.co.uk> Reviewed-by: Paul Durrant <paul@xen.org> > --- > drivers/net/xen-netback/xenbus.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c > index a5439c130130..d24b7a7993aa 100644 > --- a/drivers/net/xen-netback/xenbus.c > +++ b/drivers/net/xen-netback/xenbus.c > @@ -824,11 +824,15 @@ static void connect(struct backend_info *be) > xenvif_carrier_on(be->vif); > > unregister_hotplug_status_watch(be); > - err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch, NULL, > - hotplug_status_changed, > - "%s/%s", dev->nodename, "hotplug-status"); > - if (!err) > + if (xenbus_exists(XBT_NIL, dev->nodename, "hotplug-status")) { > + err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch, > + NULL, hotplug_status_changed, > + "%s/%s", dev->nodename, > + "hotplug-status"); > + if (err) > + goto err; > be->have_hotplug_status_watch = 1; > + } > > netif_tx_wake_all_queues(be->vif->dev); > >
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Tue, 13 Apr 2021 16:25:12 +0100 you wrote: > The logic in connect() is currently written with the assumption that > xenbus_watch_pathfmt() will return an error for a node that does not > exist. This assumption is incorrect: xenstore does allow a watch to > be registered for a nonexistent node (and will send notifications > should the node be subsequently created). > > As of commit 1f2565780 ("xen-netback: remove 'hotplug-status' once it > has served its purpose"), this leads to a failure when a domU > transitions into XenbusStateConnected more than once. On the first > domU transition into Connected state, the "hotplug-status" node will > be deleted by the hotplug_status_changed() callback in dom0. On the > second or subsequent domU transition into Connected state, the > hotplug_status_changed() callback will therefore never be invoked, and > so the backend will remain stuck in InitWait. > > [...] Here is the summary with links: - xen-netback: Check for hotplug-status existence before watching https://git.kernel.org/netdev/net/c/2afeec08ab5c You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On Tue, Apr 13, 2021 at 04:25:12PM +0100, Michael Brown wrote: > The logic in connect() is currently written with the assumption that > xenbus_watch_pathfmt() will return an error for a node that does not > exist. This assumption is incorrect: xenstore does allow a watch to > be registered for a nonexistent node (and will send notifications > should the node be subsequently created). > > As of commit 1f2565780 ("xen-netback: remove 'hotplug-status' once it > has served its purpose"), this leads to a failure when a domU > transitions into XenbusStateConnected more than once. On the first > domU transition into Connected state, the "hotplug-status" node will > be deleted by the hotplug_status_changed() callback in dom0. On the > second or subsequent domU transition into Connected state, the > hotplug_status_changed() callback will therefore never be invoked, and > so the backend will remain stuck in InitWait. > > This failure prevents scenarios such as reloading the xen-netfront > module within a domU, or booting a domU via iPXE. There is > unfortunately no way for the domU to work around this dom0 bug. > > Fix by explicitly checking for existence of the "hotplug-status" node, > thereby creating the behaviour that was previously assumed to exist. This change is wrong. The 'hotplug-status' node is created _only_ by a hotplug script and done so when it's executed. When kernel waits for hotplug script to be executed it waits for the node to _appear_, not _change_. So, this change basically made the kernel not waiting for the hotplug script at all. Furthermore, there is an additional side effect: in case of a driver domain, xl devd may be started after the backend node is created (this may happen if you start the frontend domain in parallel with the backend). In this case, 'xl devd' will see the vif backend in XenbusStateConnected state already and will not execute hotplug script at all. I think the proper fix is to re-register the watch when necessary, instead of not registering it at all. > Signed-off-by: Michael Brown <mbrown@fensystems.co.uk> > --- > drivers/net/xen-netback/xenbus.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c > index a5439c130130..d24b7a7993aa 100644 > --- a/drivers/net/xen-netback/xenbus.c > +++ b/drivers/net/xen-netback/xenbus.c > @@ -824,11 +824,15 @@ static void connect(struct backend_info *be) > xenvif_carrier_on(be->vif); > > unregister_hotplug_status_watch(be); > - err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch, NULL, > - hotplug_status_changed, > - "%s/%s", dev->nodename, "hotplug-status"); > - if (!err) > + if (xenbus_exists(XBT_NIL, dev->nodename, "hotplug-status")) { > + err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch, > + NULL, hotplug_status_changed, > + "%s/%s", dev->nodename, > + "hotplug-status"); > + if (err) > + goto err; > be->have_hotplug_status_watch = 1; > + } > > netif_tx_wake_all_queues(be->vif->dev); >
On 10/05/2021 19:32, Marek Marczykowski-Górecki wrote: > On Tue, Apr 13, 2021 at 04:25:12PM +0100, Michael Brown wrote: >> The logic in connect() is currently written with the assumption that >> xenbus_watch_pathfmt() will return an error for a node that does not >> exist. This assumption is incorrect: xenstore does allow a watch to >> be registered for a nonexistent node (and will send notifications >> should the node be subsequently created). >> >> As of commit 1f2565780 ("xen-netback: remove 'hotplug-status' once it >> has served its purpose"), this leads to a failure when a domU >> transitions into XenbusStateConnected more than once. On the first >> domU transition into Connected state, the "hotplug-status" node will >> be deleted by the hotplug_status_changed() callback in dom0. On the >> second or subsequent domU transition into Connected state, the >> hotplug_status_changed() callback will therefore never be invoked, and >> so the backend will remain stuck in InitWait. >> >> This failure prevents scenarios such as reloading the xen-netfront >> module within a domU, or booting a domU via iPXE. There is >> unfortunately no way for the domU to work around this dom0 bug. >> >> Fix by explicitly checking for existence of the "hotplug-status" node, >> thereby creating the behaviour that was previously assumed to exist. > > This change is wrong. The 'hotplug-status' node is created _only_ by a > hotplug script and done so when it's executed. When kernel waits for > hotplug script to be executed it waits for the node to _appear_, not > _change_. So, this change basically made the kernel not waiting for the > hotplug script at all. That doesn't sound plausible to me. In the setup as you describe, how is the kernel expected to differentiate between "hotplug script has not yet created the node" and "hotplug script does not exist and will therefore never create any node"? Michael
On Mon, May 10, 2021 at 07:47:01PM +0100, Michael Brown wrote: > On 10/05/2021 19:32, Marek Marczykowski-Górecki wrote: > > On Tue, Apr 13, 2021 at 04:25:12PM +0100, Michael Brown wrote: > > > The logic in connect() is currently written with the assumption that > > > xenbus_watch_pathfmt() will return an error for a node that does not > > > exist. This assumption is incorrect: xenstore does allow a watch to > > > be registered for a nonexistent node (and will send notifications > > > should the node be subsequently created). > > > > > > As of commit 1f2565780 ("xen-netback: remove 'hotplug-status' once it > > > has served its purpose"), this leads to a failure when a domU > > > transitions into XenbusStateConnected more than once. On the first > > > domU transition into Connected state, the "hotplug-status" node will > > > be deleted by the hotplug_status_changed() callback in dom0. On the > > > second or subsequent domU transition into Connected state, the > > > hotplug_status_changed() callback will therefore never be invoked, and > > > so the backend will remain stuck in InitWait. > > > > > > This failure prevents scenarios such as reloading the xen-netfront > > > module within a domU, or booting a domU via iPXE. There is > > > unfortunately no way for the domU to work around this dom0 bug. > > > > > > Fix by explicitly checking for existence of the "hotplug-status" node, > > > thereby creating the behaviour that was previously assumed to exist. > > > > This change is wrong. The 'hotplug-status' node is created _only_ by a > > hotplug script and done so when it's executed. When kernel waits for > > hotplug script to be executed it waits for the node to _appear_, not > > _change_. So, this change basically made the kernel not waiting for the > > hotplug script at all. > > That doesn't sound plausible to me. In the setup as you describe, how is > the kernel expected to differentiate between "hotplug script has not yet > created the node" and "hotplug script does not exist and will therefore > never create any node"? Is the later valid at all? From what I can see in the toolstack, it always sets some hotplug script (if not specified explicitly - then "vif-bridge"),
On 10/05/2021 19:53, Marek Marczykowski-Górecki wrote: > On Mon, May 10, 2021 at 07:47:01PM +0100, Michael Brown wrote: >> That doesn't sound plausible to me. In the setup as you describe, how is >> the kernel expected to differentiate between "hotplug script has not yet >> created the node" and "hotplug script does not exist and will therefore >> never create any node"? > > Is the later valid at all? From what I can see in the toolstack, it > always sets some hotplug script (if not specified explicitly - then > "vif-bridge"), I don't see any definitive documentation around that so I can't answer for sure. It's probably best to let one of the Xen guys answer that. If you have a suggested patch, I'm happy to test that it doesn't reintroduce the regression bug that was fixed by this commit. Michael
On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote: > If you have a suggested patch, I'm happy to test that it doesn't reintroduce > the regression bug that was fixed by this commit. Actually, I've just tested with a simple reloading xen-netfront module. It seems in this case, the hotplug script is not re-executed. In fact, I think it should not be re-executed at all, since the vif interface remains in place (it just gets NO-CARRIER flag). This brings a question, why removing hotplug-status in the first place? The interface remains correctly configured by the hotplug script after all. From the commit message: xen-netback: remove 'hotplug-status' once it has served its purpose Removing the 'hotplug-status' node in netback_remove() is wrong; the script may not have completed. Only remove the node once the watch has fired and has been unregistered. I think the intention was to remove 'hotplug-status' node _later_ in case of quickly adding and removing the interface. Is that right, Paul? In that case, letting hotplug_status_changed() remove the entry wont work, because the watch was unregistered few lines earlier in netback_remove(). And keeping the watch is not an option, because the whole backend_info struct is going to be free-ed already. If my guess about the original reason for the change is right, I think it should be fixed at the hotplug script level - it should check if the device is still there before writing 'hotplug-status' node. I'm not sure if doing it race-free is possible from a shell script (I think it requires doing xenstore read _and_ write in a single transaction). But in the worst case, the aftermath of loosing the race is leaving stray 'hotplug-status' xenstore node - not ideal, but also less harmful than failing to bring up an interface. At this point, the toolstack could cleanup it later, perhaps while setting up that interface again (if it gets re-connected)? Anyway, perhaps the best thing to do now, is to revert both commits, and think of an alternative solution for the original issue? That of course assumes I guessed correctly why it was done in the first place...
> -----Original Message----- > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > Sent: 10 May 2021 20:43 > To: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org > Cc: paul@xen.org; xen-devel@lists.xenproject.org; netdev@vger.kernel.org; wei.liu@kernel.org; Durrant, > Paul <pdurrant@amazon.co.uk> > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching > > On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote: > > If you have a suggested patch, I'm happy to test that it doesn't reintroduce > > the regression bug that was fixed by this commit. > > Actually, I've just tested with a simple reloading xen-netfront module. It > seems in this case, the hotplug script is not re-executed. In fact, I > think it should not be re-executed at all, since the vif interface > remains in place (it just gets NO-CARRIER flag). > > This brings a question, why removing hotplug-status in the first place? > The interface remains correctly configured by the hotplug script after > all. From the commit message: > > xen-netback: remove 'hotplug-status' once it has served its purpose > > Removing the 'hotplug-status' node in netback_remove() is wrong; the script > may not have completed. Only remove the node once the watch has fired and > has been unregistered. > > I think the intention was to remove 'hotplug-status' node _later_ in > case of quickly adding and removing the interface. Is that right, Paul? The removal was done to allow unbind/bind to function correctly. IIRC before the original patch doing a bind would stall forever waiting for the hotplug status to change, which would never happen. > In that case, letting hotplug_status_changed() remove the entry wont > work, because the watch was unregistered few lines earlier in > netback_remove(). And keeping the watch is not an option, because the > whole backend_info struct is going to be free-ed already. > > If my guess about the original reason for the change is right, I think > it should be fixed at the hotplug script level - it should check if the > device is still there before writing 'hotplug-status' node. > I'm not sure if doing it race-free is possible from a shell script (I think it > requires doing xenstore read _and_ write in a single transaction). But > in the worst case, the aftermath of loosing the race is leaving stray > 'hotplug-status' xenstore node - not ideal, but also less harmful than > failing to bring up an interface. At this point, the toolstack could cleanup > it later, perhaps while setting up that interface again (if it gets > re-connected)? > > Anyway, perhaps the best thing to do now, is to revert both commits, and > think of an alternative solution for the original issue? That of course > assumes I guessed correctly why it was done in the first place... > Simply reverting everything would likely break the ability to do unbind and bind (which is useful e.g to allow update the netback module whilst guests are still running) so I don't think that's an option. Paul
On Tue, May 11, 2021 at 07:06:55AM +0000, Durrant, Paul wrote: > > -----Original Message----- > > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > Sent: 10 May 2021 20:43 > > To: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org > > Cc: paul@xen.org; xen-devel@lists.xenproject.org; netdev@vger.kernel.org; wei.liu@kernel.org; Durrant, > > Paul <pdurrant@amazon.co.uk> > > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching > > > > On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote: > > > If you have a suggested patch, I'm happy to test that it doesn't reintroduce > > > the regression bug that was fixed by this commit. > > > > Actually, I've just tested with a simple reloading xen-netfront module. It > > seems in this case, the hotplug script is not re-executed. In fact, I > > think it should not be re-executed at all, since the vif interface > > remains in place (it just gets NO-CARRIER flag). > > > > This brings a question, why removing hotplug-status in the first place? > > The interface remains correctly configured by the hotplug script after > > all. From the commit message: > > > > xen-netback: remove 'hotplug-status' once it has served its purpose > > > > Removing the 'hotplug-status' node in netback_remove() is wrong; the script > > may not have completed. Only remove the node once the watch has fired and > > has been unregistered. > > > > I think the intention was to remove 'hotplug-status' node _later_ in > > case of quickly adding and removing the interface. Is that right, Paul? > > The removal was done to allow unbind/bind to function correctly. IIRC before the original patch doing a bind would stall forever waiting for the hotplug status to change, which would never happen. Hmm, in that case maybe don't remove it at all in the backend, and let it be cleaned up by the toolstack, when it removes other backend-related nodes?
On Tue, May 11, 2021 at 12:40:54PM +0200, Marek Marczykowski-Górecki wrote: > On Tue, May 11, 2021 at 07:06:55AM +0000, Durrant, Paul wrote: > > > -----Original Message----- > > > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > > Sent: 10 May 2021 20:43 > > > To: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org > > > Cc: paul@xen.org; xen-devel@lists.xenproject.org; netdev@vger.kernel.org; wei.liu@kernel.org; Durrant, > > > Paul <pdurrant@amazon.co.uk> > > > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching > > > > > > On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote: > > > > If you have a suggested patch, I'm happy to test that it doesn't reintroduce > > > > the regression bug that was fixed by this commit. > > > > > > Actually, I've just tested with a simple reloading xen-netfront module. It > > > seems in this case, the hotplug script is not re-executed. In fact, I > > > think it should not be re-executed at all, since the vif interface > > > remains in place (it just gets NO-CARRIER flag). > > > > > > This brings a question, why removing hotplug-status in the first place? > > > The interface remains correctly configured by the hotplug script after > > > all. From the commit message: > > > > > > xen-netback: remove 'hotplug-status' once it has served its purpose > > > > > > Removing the 'hotplug-status' node in netback_remove() is wrong; the script > > > may not have completed. Only remove the node once the watch has fired and > > > has been unregistered. > > > > > > I think the intention was to remove 'hotplug-status' node _later_ in > > > case of quickly adding and removing the interface. Is that right, Paul? > > > > The removal was done to allow unbind/bind to function correctly. IIRC before the original patch doing a bind would stall forever waiting for the hotplug status to change, which would never happen. > > Hmm, in that case maybe don't remove it at all in the backend, and let > it be cleaned up by the toolstack, when it removes other backend-related > nodes? No, unbind/bind _does_ require hotplug script to be called again. When exactly vif interface appears in the system (starts to be available for the hotplug script)? Maybe remove 'hotplug-status' just before that point?
> -----Original Message----- > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > Sent: 11 May 2021 11:45 > To: Durrant, Paul <pdurrant@amazon.co.uk> > Cc: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org; xen-devel@lists.xenproject.org; > netdev@vger.kernel.org; wei.liu@kernel.org > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching > > On Tue, May 11, 2021 at 12:40:54PM +0200, Marek Marczykowski-Górecki wrote: > > On Tue, May 11, 2021 at 07:06:55AM +0000, Durrant, Paul wrote: > > > > -----Original Message----- > > > > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > > > Sent: 10 May 2021 20:43 > > > > To: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org > > > > Cc: paul@xen.org; xen-devel@lists.xenproject.org; netdev@vger.kernel.org; wei.liu@kernel.org; > Durrant, > > > > Paul <pdurrant@amazon.co.uk> > > > > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching > > > > > > > > On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote: > > > > > If you have a suggested patch, I'm happy to test that it doesn't reintroduce > > > > > the regression bug that was fixed by this commit. > > > > > > > > Actually, I've just tested with a simple reloading xen-netfront module. It > > > > seems in this case, the hotplug script is not re-executed. In fact, I > > > > think it should not be re-executed at all, since the vif interface > > > > remains in place (it just gets NO-CARRIER flag). > > > > > > > > This brings a question, why removing hotplug-status in the first place? > > > > The interface remains correctly configured by the hotplug script after > > > > all. From the commit message: > > > > > > > > xen-netback: remove 'hotplug-status' once it has served its purpose > > > > > > > > Removing the 'hotplug-status' node in netback_remove() is wrong; the script > > > > may not have completed. Only remove the node once the watch has fired and > > > > has been unregistered. > > > > > > > > I think the intention was to remove 'hotplug-status' node _later_ in > > > > case of quickly adding and removing the interface. Is that right, Paul? > > > > > > The removal was done to allow unbind/bind to function correctly. IIRC before the original patch > doing a bind would stall forever waiting for the hotplug status to change, which would never happen. > > > > Hmm, in that case maybe don't remove it at all in the backend, and let > > it be cleaned up by the toolstack, when it removes other backend-related > > nodes? > > No, unbind/bind _does_ require hotplug script to be called again. > Yes, sorry I was misremembering. My memory is hazy but there was definitely a problem at the time with leaving the node in place. > When exactly vif interface appears in the system (starts to be available > for the hotplug script)? Maybe remove 'hotplug-status' just before that > point? > I really can't remember any detail. Perhaps try reverting both patches then and check that the unbind/rmmod/modprobe/bind sequence still works (and the backend actually makes it into connected state). Paul
On Tue, May 11, 2021 at 12:46:38PM +0000, Durrant, Paul wrote: > > -----Original Message----- > > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > Sent: 11 May 2021 11:45 > > To: Durrant, Paul <pdurrant@amazon.co.uk> > > Cc: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org; xen-devel@lists.xenproject.org; > > netdev@vger.kernel.org; wei.liu@kernel.org > > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching > > > > On Tue, May 11, 2021 at 12:40:54PM +0200, Marek Marczykowski-Górecki wrote: > > > On Tue, May 11, 2021 at 07:06:55AM +0000, Durrant, Paul wrote: > > > > > -----Original Message----- > > > > > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > > > > Sent: 10 May 2021 20:43 > > > > > To: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org > > > > > Cc: paul@xen.org; xen-devel@lists.xenproject.org; netdev@vger.kernel.org; wei.liu@kernel.org; > > Durrant, > > > > > Paul <pdurrant@amazon.co.uk> > > > > > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching > > > > > > > > > > On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote: > > > > > > If you have a suggested patch, I'm happy to test that it doesn't reintroduce > > > > > > the regression bug that was fixed by this commit. > > > > > > > > > > Actually, I've just tested with a simple reloading xen-netfront module. It > > > > > seems in this case, the hotplug script is not re-executed. In fact, I > > > > > think it should not be re-executed at all, since the vif interface > > > > > remains in place (it just gets NO-CARRIER flag). > > > > > > > > > > This brings a question, why removing hotplug-status in the first place? > > > > > The interface remains correctly configured by the hotplug script after > > > > > all. From the commit message: > > > > > > > > > > xen-netback: remove 'hotplug-status' once it has served its purpose > > > > > > > > > > Removing the 'hotplug-status' node in netback_remove() is wrong; the script > > > > > may not have completed. Only remove the node once the watch has fired and > > > > > has been unregistered. > > > > > > > > > > I think the intention was to remove 'hotplug-status' node _later_ in > > > > > case of quickly adding and removing the interface. Is that right, Paul? > > > > > > > > The removal was done to allow unbind/bind to function correctly. IIRC before the original patch > > doing a bind would stall forever waiting for the hotplug status to change, which would never happen. > > > > > > Hmm, in that case maybe don't remove it at all in the backend, and let > > > it be cleaned up by the toolstack, when it removes other backend-related > > > nodes? > > > > No, unbind/bind _does_ require hotplug script to be called again. > > > > Yes, sorry I was misremembering. My memory is hazy but there was definitely a problem at the time with leaving the node in place. > > > When exactly vif interface appears in the system (starts to be available > > for the hotplug script)? Maybe remove 'hotplug-status' just before that > > point? > > > > I really can't remember any detail. Perhaps try reverting both patches then and check that the unbind/rmmod/modprobe/bind sequence still works (and the backend actually makes it into connected state). Ok, I've tried this. I've reverted both commits, then used your test script from the 9476654bd5e8ad42abe8ee9f9e90069ff8e60c17: This has been tested by running iperf as a server in the test VM and then running a client against it in a continuous loop, whilst also running: while true; do echo vif-$DOMID-$VIF >unbind; echo down; rmmod xen-netback; echo unloaded; modprobe xen-netback; cd $(pwd); brctl addif xenbr0 vif$DOMID.$VIF; ip link set vif$DOMID.$VIF up; echo up; sleep 5; done in dom0 from /sys/bus/xen-backend/drivers/vif to continuously unbind, unload, re-load, re-bind and re-plumb the backend. In fact, the need to call `brctl` and `ip link` manually is exactly because the hotplug script isn't executed. When I execute it manually, the backend properly gets back to working. So, removing 'hotplug-status' was in the correct place (netback_remove). The missing part is the toolstack calling the hotplug script on xen-netback re-bind. In this case, I'm not sure what is the proper way. If I restart xendriverdomain service (I do run the backend in domU), it properly executes hotplug script and the backend interface gets properly configured. But it doesn't do it on its own. It seems to be related to device "state" in xenstore. The specific part of the libxl is backend_watch_callback(): https://github.com/xen-project/xen/blob/master/tools/libs/light/libxl_device.c#L1664 ddev = search_for_device(dguest, dev); if (ddev == NULL && state == XenbusStateClosed) { /* * Spurious state change, device has already been disconnected * or never attached. */ goto skip; } else if (ddev == NULL) { rc = add_device(egc, nested_ao, dguest, dev); if (rc > 0) free_ao = true; } else if (state == XenbusStateClosed && online == 0) { rc = remove_device(egc, nested_ao, dguest, ddev); if (rc > 0) free_ao = true; check_and_maybe_remove_guest(gc, ddomain, dguest); } In short: if device gets XenbusStateInitWait for the first time (ddev == NULL case), it goes to add_device() which executes the hotplug script and stores the device. Then, if device goes to XenbusStateClosed + online==0 state, then it executes hotplug script again (with "offline" parameter) and forgets the device. If you unbind the driver, the device stays in XenbusStateConnected state (in xenstore), and after you bind it again, it goes to XenbusStateInitWait. It don't think it goes through XenbusStateClosed, and online stays at 1 too, so libxl doesn't execute the hotplug script again. Some solution could be to add an extra case at the end, like "ddev != NULL && state == XenbusStateInitWait && hotplug-status != connected". And make sure xl devd won't call the same hotplug script multiple times for the same device _at the same time_ (I'm not sure about the async machinery here). But even if xl devd (aka xendriverdomain service) gets "fixes" to execute hotplug script in that case, I don't think it would work in backend in dom0 case - there, I think nothing watches already configured vif interfaces (there is no xl devd daemon in dom0, and xl background process watches only domain death and cdrom eject events). I'm adding toolstack maintainers, maybe they'll have some idea... In any case, the issue is not calling the hotplug script, responsible for configuring newly created vif interface. Not kernel waiting for it. So, I think both commits should still be reverted.
On 17/05/2021 22:43, Marek Marczykowski-Górecki wrote: > In any case, the issue is not calling the hotplug script, responsible > for configuring newly created vif interface. Not kernel waiting for it. > So, I think both commits should still be reverted. Did you also test the ability for a domU to have the netfront driver reloaded? (That should be roughly equivalent to my original test scenario comprising the iPXE netfront driver used to boot a kernel that then loads the Linux netfront driver.) Michael
On Mon, May 17, 2021 at 10:51:38PM +0100, Michael Brown wrote: > On 17/05/2021 22:43, Marek Marczykowski-Górecki wrote: > > In any case, the issue is not calling the hotplug script, responsible > > for configuring newly created vif interface. Not kernel waiting for it. > > So, I think both commits should still be reverted. > > Did you also test the ability for a domU to have the netfront driver > reloaded? (That should be roughly equivalent to my original test scenario > comprising the iPXE netfront driver used to boot a kernel that then loads > the Linux netfront driver.) Yes, with both commits reverted, it just works. Without the need to do anything at the backend side.
On 17/05/2021 22:43, Marek Marczykowski-Górecki wrote: > On Tue, May 11, 2021 at 12:46:38PM +0000, Durrant, Paul wrote: >>> -----Original Message----- >>> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >>> Sent: 11 May 2021 11:45 >>> To: Durrant, Paul <pdurrant@amazon.co.uk> >>> Cc: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org; xen-devel@lists.xenproject.org; >>> netdev@vger.kernel.org; wei.liu@kernel.org >>> Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching >>> >>> On Tue, May 11, 2021 at 12:40:54PM +0200, Marek Marczykowski-Górecki wrote: >>>> On Tue, May 11, 2021 at 07:06:55AM +0000, Durrant, Paul wrote: >>>>>> -----Original Message----- >>>>>> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >>>>>> Sent: 10 May 2021 20:43 >>>>>> To: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org >>>>>> Cc: paul@xen.org; xen-devel@lists.xenproject.org; netdev@vger.kernel.org; wei.liu@kernel.org; >>> Durrant, >>>>>> Paul <pdurrant@amazon.co.uk> >>>>>> Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching >>>>>> >>>>>> On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote: >>>>>>> If you have a suggested patch, I'm happy to test that it doesn't reintroduce >>>>>>> the regression bug that was fixed by this commit. >>>>>> >>>>>> Actually, I've just tested with a simple reloading xen-netfront module. It >>>>>> seems in this case, the hotplug script is not re-executed. In fact, I >>>>>> think it should not be re-executed at all, since the vif interface >>>>>> remains in place (it just gets NO-CARRIER flag). >>>>>> >>>>>> This brings a question, why removing hotplug-status in the first place? >>>>>> The interface remains correctly configured by the hotplug script after >>>>>> all. From the commit message: >>>>>> >>>>>> xen-netback: remove 'hotplug-status' once it has served its purpose >>>>>> >>>>>> Removing the 'hotplug-status' node in netback_remove() is wrong; the script >>>>>> may not have completed. Only remove the node once the watch has fired and >>>>>> has been unregistered. >>>>>> >>>>>> I think the intention was to remove 'hotplug-status' node _later_ in >>>>>> case of quickly adding and removing the interface. Is that right, Paul? >>>>> >>>>> The removal was done to allow unbind/bind to function correctly. IIRC before the original patch >>> doing a bind would stall forever waiting for the hotplug status to change, which would never happen. >>>> >>>> Hmm, in that case maybe don't remove it at all in the backend, and let >>>> it be cleaned up by the toolstack, when it removes other backend-related >>>> nodes? >>> >>> No, unbind/bind _does_ require hotplug script to be called again. >>> >> >> Yes, sorry I was misremembering. My memory is hazy but there was definitely a problem at the time with leaving the node in place. >> >>> When exactly vif interface appears in the system (starts to be available >>> for the hotplug script)? Maybe remove 'hotplug-status' just before that >>> point? >>> >> >> I really can't remember any detail. Perhaps try reverting both patches then and check that the unbind/rmmod/modprobe/bind sequence still works (and the backend actually makes it into connected state). > > Ok, I've tried this. I've reverted both commits, then used your test > script from the 9476654bd5e8ad42abe8ee9f9e90069ff8e60c17: > > This has been tested by running iperf as a server in the test VM and > then running a client against it in a continuous loop, whilst also > running: > > while true; > do echo vif-$DOMID-$VIF >unbind; > echo down; > rmmod xen-netback; > echo unloaded; > modprobe xen-netback; > cd $(pwd); > brctl addif xenbr0 vif$DOMID.$VIF; > ip link set vif$DOMID.$VIF up; > echo up; > sleep 5; > done > > in dom0 from /sys/bus/xen-backend/drivers/vif to continuously unbind, > unload, re-load, re-bind and re-plumb the backend. > > In fact, the need to call `brctl` and `ip link` manually is exactly > because the hotplug script isn't executed. When I execute it manually, > the backend properly gets back to working. So, removing 'hotplug-status' > was in the correct place (netback_remove). The missing part is the toolstack > calling the hotplug script on xen-netback re-bind. > Why is that missing? We're going behind the back of the toolstack to do the unbind and bind so why should we expect it to re-execute a hotplug script? > In this case, I'm not sure what is the proper way. If I restart > xendriverdomain service (I do run the backend in domU), it properly > executes hotplug script and the backend interface gets properly > configured. But it doesn't do it on its own. It seems to be related to > device "state" in xenstore. The specific part of the libxl is > backend_watch_callback(): > https://github.com/xen-project/xen/blob/master/tools/libs/light/libxl_device.c#L1664 > > ddev = search_for_device(dguest, dev); > if (ddev == NULL && state == XenbusStateClosed) { > /* > * Spurious state change, device has already been disconnected > * or never attached. > */ > goto skip; > } else if (ddev == NULL) { > rc = add_device(egc, nested_ao, dguest, dev); > if (rc > 0) > free_ao = true; > } else if (state == XenbusStateClosed && online == 0) { > rc = remove_device(egc, nested_ao, dguest, ddev); > if (rc > 0) > free_ao = true; > check_and_maybe_remove_guest(gc, ddomain, dguest); > } > > In short: if device gets XenbusStateInitWait for the first time (ddev == > NULL case), it goes to add_device() which executes the hotplug script > and stores the device. > Then, if device goes to XenbusStateClosed + online==0 state, then it > executes hotplug script again (with "offline" parameter) and forgets the > device. If you unbind the driver, the device stays in > XenbusStateConnected state (in xenstore), and after you bind it again, > it goes to XenbusStateInitWait. It don't think it goes through > XenbusStateClosed, and online stays at 1 too, so libxl doesn't execute > the hotplug script again. This is pretty key. The frontend should not notice an unbind/bind i.e. there should be no evidence of it happening by examining states in xenstore (from the guest side). Paul > > Some solution could be to add an extra case at the end, like "ddev != > NULL && state == XenbusStateInitWait && hotplug-status != connected". > And make sure xl devd won't call the same hotplug script multiple times > for the same device _at the same time_ (I'm not sure about the async > machinery here). > > But even if xl devd (aka xendriverdomain service) gets "fixes" to > execute hotplug script in that case, I don't think it would work in > backend in dom0 case - there, I think nothing watches already configured > vif interfaces (there is no xl devd daemon in dom0, and xl background > process watches only domain death and cdrom eject events). > > I'm adding toolstack maintainers, maybe they'll have some idea... > > In any case, the issue is not calling the hotplug script, responsible > for configuring newly created vif interface. Not kernel waiting for it. > So, I think both commits should still be reverted. >
On Tue, May 18, 2021 at 07:57:16AM +0100, Paul Durrant wrote: > On 17/05/2021 22:43, Marek Marczykowski-Górecki wrote: > > On Tue, May 11, 2021 at 12:46:38PM +0000, Durrant, Paul wrote: > > > I really can't remember any detail. Perhaps try reverting both patches then and check that the unbind/rmmod/modprobe/bind sequence still works (and the backend actually makes it into connected state). > > > > Ok, I've tried this. I've reverted both commits, then used your test > > script from the 9476654bd5e8ad42abe8ee9f9e90069ff8e60c17: > > This has been tested by running iperf as a server in the test VM and > > then running a client against it in a continuous loop, whilst also > > running: > > while true; > > do echo vif-$DOMID-$VIF >unbind; > > echo down; > > rmmod xen-netback; > > echo unloaded; > > modprobe xen-netback; > > cd $(pwd); > > brctl addif xenbr0 vif$DOMID.$VIF; > > ip link set vif$DOMID.$VIF up; > > echo up; > > sleep 5; > > done > > in dom0 from /sys/bus/xen-backend/drivers/vif to continuously unbind, > > unload, re-load, re-bind and re-plumb the backend. > > In fact, the need to call `brctl` and `ip link` manually is exactly > > because the hotplug script isn't executed. When I execute it manually, > > the backend properly gets back to working. So, removing 'hotplug-status' > > was in the correct place (netback_remove). The missing part is the toolstack > > calling the hotplug script on xen-netback re-bind. > > > > Why is that missing? We're going behind the back of the toolstack to do the > unbind and bind so why should we expect it to re-execute a hotplug script? Ok, then simply execute the whole hotplug script (instead of its subset) after re-loading the backend module and everything will be fine. For example like this: XENBUS_PATH=backend/vif/$DOMID/$VIF \ XENBUS_TYPE=vif \ XENBUS_BASE_PATH=backend \ script=/etc/xen/scripts/vif-bridge \ vif=vif.$DOMID.$VIF \ /etc/xen/scripts/vif-bridge online (...) > > In short: if device gets XenbusStateInitWait for the first time (ddev == > > NULL case), it goes to add_device() which executes the hotplug script > > and stores the device. > > Then, if device goes to XenbusStateClosed + online==0 state, then it > > executes hotplug script again (with "offline" parameter) and forgets the > > device. If you unbind the driver, the device stays in > > XenbusStateConnected state (in xenstore), and after you bind it again, > > it goes to XenbusStateInitWait. It don't think it goes through > > XenbusStateClosed, and online stays at 1 too, so libxl doesn't execute > > the hotplug script again. > > This is pretty key. The frontend should not notice an unbind/bind i.e. there > should be no evidence of it happening by examining states in xenstore (from > the guest side). If you update the backend module, I think the frontend needs at least to re-evaluate feature-* nodes. In case of applying just a bug fix, they should not change (in theory), but technically that would be the correct thing to do.
On Tue, May 18, 2021 at 09:48:25AM +0000, Durrant, Paul wrote: > > -----Original Message----- > > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > > > On Tue, May 18, 2021 at 09:34:45AM +0000, Durrant, Paul wrote: > > > > -----Original Message----- > > > > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > > > > > > > On Tue, May 18, 2021 at 07:57:16AM +0100, Paul Durrant wrote: > > > > > Why is that missing? We're going behind the back of the toolstack to do the > > > > > unbind and bind so why should we expect it to re-execute a hotplug script? > > > > > > > > Ok, then simply execute the whole hotplug script (instead of its subset) > > > > after re-loading the backend module and everything will be fine. > > > > > > > > For example like this: > > > > XENBUS_PATH=backend/vif/$DOMID/$VIF \ > > > > XENBUS_TYPE=vif \ > > > > XENBUS_BASE_PATH=backend \ > > > > script=/etc/xen/scripts/vif-bridge \ > > > > vif=vif.$DOMID.$VIF \ > > > > /etc/xen/scripts/vif-bridge online > > > > > > > > > > ... as long as there's no xenstore fall-out that the guest can observe. > > > > Backend will set state to XenbusStateInitWait on load anyway... > > > > Oh, that sounds like a bug then... It ought to go straight to connected if the frontend is already there. To me this sounds very suspicious. But if that's really what should backend do, then it would "solve" also hotplug-status node issue. See the end of netback_probe() function. But I think if you start processing traffic before hotplug script configures the interface (so - without switching to XenbusStateInitWait and waiting for hotplug-status node), you'll post some packets into not enabled interface, which I think will drop them (not queue). TCP will be fine with that, but many other protocols not.
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c index a5439c130130..d24b7a7993aa 100644 --- a/drivers/net/xen-netback/xenbus.c +++ b/drivers/net/xen-netback/xenbus.c @@ -824,11 +824,15 @@ static void connect(struct backend_info *be) xenvif_carrier_on(be->vif); unregister_hotplug_status_watch(be); - err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch, NULL, - hotplug_status_changed, - "%s/%s", dev->nodename, "hotplug-status"); - if (!err) + if (xenbus_exists(XBT_NIL, dev->nodename, "hotplug-status")) { + err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch, + NULL, hotplug_status_changed, + "%s/%s", dev->nodename, + "hotplug-status"); + if (err) + goto err; be->have_hotplug_status_watch = 1; + } netif_tx_wake_all_queues(be->vif->dev);
The logic in connect() is currently written with the assumption that xenbus_watch_pathfmt() will return an error for a node that does not exist. This assumption is incorrect: xenstore does allow a watch to be registered for a nonexistent node (and will send notifications should the node be subsequently created). As of commit 1f2565780 ("xen-netback: remove 'hotplug-status' once it has served its purpose"), this leads to a failure when a domU transitions into XenbusStateConnected more than once. On the first domU transition into Connected state, the "hotplug-status" node will be deleted by the hotplug_status_changed() callback in dom0. On the second or subsequent domU transition into Connected state, the hotplug_status_changed() callback will therefore never be invoked, and so the backend will remain stuck in InitWait. This failure prevents scenarios such as reloading the xen-netfront module within a domU, or booting a domU via iPXE. There is unfortunately no way for the domU to work around this dom0 bug. Fix by explicitly checking for existence of the "hotplug-status" node, thereby creating the behaviour that was previously assumed to exist. Signed-off-by: Michael Brown <mbrown@fensystems.co.uk> --- drivers/net/xen-netback/xenbus.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)