xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: "Durrant, Paul" <pdurrant@amazon.co.uk>
To: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>,
	"Michael Brown" <mbrown@fensystems.co.uk>,
	"paul@xen.org" <paul@xen.org>
Cc: "paul@xen.org" <paul@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"wei.liu@kernel.org" <wei.liu@kernel.org>
Subject: RE: [PATCH] xen-netback: Check for hotplug-status existence before watching
Date: Tue, 11 May 2021 07:06:55 +0000	[thread overview]
Message-ID: <df9e9a32b0294aee814eeb58d2d71edd@EX13D32EUC003.ant.amazon.com> (raw)
In-Reply-To: <YJmMvTkp2Y1hlLLm@mail-itl>

> -----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.


  reply	other threads:[~2021-05-11  7:33 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-10 18:25 xen-netback hotplug-status regression bug Michael Brown
2021-04-13  7:12 ` Paul Durrant
2021-04-13 10:48   ` Michael Brown
2021-04-13 10:55     ` Paul Durrant
2021-04-13 15:14       ` Michael Brown
2021-04-13 15:25       ` [PATCH] xen-netback: Check for hotplug-status existence before watching Michael Brown
2021-04-13 19:12         ` Paul Durrant
2021-04-13 22:30         ` patchwork-bot+netdevbpf
2021-05-10 18:32         ` Marek Marczykowski-Górecki
2021-05-10 18:47           ` Michael Brown
2021-05-10 18:53             ` Marek Marczykowski-Górecki
2021-05-10 19:06               ` Michael Brown
2021-05-10 19:42                 ` Marek Marczykowski-Górecki
2021-05-11  7:06                   ` Durrant, Paul [this message]
2021-05-11 10:40                     ` Marek Marczykowski-Górecki
2021-05-11 10:45                       ` Marek Marczykowski-Górecki
2021-05-11 12:46                         ` Durrant, Paul
2021-05-17 21:43                           ` Marek Marczykowski-Górecki
2021-05-17 21:51                             ` Michael Brown
2021-05-17 21:58                               ` Marek Marczykowski-Górecki
2021-05-18  6:57                             ` Paul Durrant
2021-05-18  9:18                               ` Marek Marczykowski-Górecki
     [not found]                                 ` <887f9533f5c54bfabfbff7231eb99b08@EX13D32EUC003.ant.amazon.com>
     [not found]                                   ` <YKOMpXwcnr9QiXy8@mail-itl>
     [not found]                                     ` <2c23e102b6254e42877eb1e8fe68a4f7@EX13D32EUC003.ant.amazon.com>
2021-05-18 10:42                                       ` Marek Marczykowski-Górecki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=df9e9a32b0294aee814eeb58d2d71edd@EX13D32EUC003.ant.amazon.com \
    --to=pdurrant@amazon.co.uk \
    --cc=marmarek@invisiblethingslab.com \
    --cc=mbrown@fensystems.co.uk \
    --cc=netdev@vger.kernel.org \
    --cc=paul@xen.org \
    --cc=wei.liu@kernel.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='RE: [PATCH] xen-netback: Check for hotplug-status existence before watching' \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).