RE: [Xen-devel] [PATCH net-next] xen-netback: get rid of old udev related code

[Durrant, Paul]

> -----Original Message-----
> From: jandryuk@xxxxxxxxx <jandryuk@xxxxxxxxx>
> Sent: 12 December 2019 16:32
> To: Durrant, Paul <pdurrant@xxxxxxxxxx>
> Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> open list <linux-kernel@xxxxxxxxxxxxxxx>; Wei Liu <wei.liu@xxxxxxxxxx>;
> David S. Miller <davem@xxxxxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH net-next] xen-netback: get rid of old udev
> related code
> 
> On Thu, Dec 12, 2019 at 8:56 AM Paul Durrant <pdurrant@xxxxxxxxxx> wrote:
> >
> > In the past it used to be the case that the Xen toolstack relied upon
> > udev to execute backend hotplug scripts. However this has not been the
> > case for many releases now and removal of the associated code in
> > xen-netback shortens the source by more than 100 lines, and removes much
> > complexity in the interaction with the xenstore backend state.
> >
> > NOTE: xen-netback is the only xenbus driver to have a functional
> uevent()
> >       method. The only other driver to have a method at all is
> >       pvcalls-back, and currently pvcalls_back_uevent() simply returns
> 0.
> >       Hence this patch also facilitates further cleanup.
> >
> > Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> > ---
> > Cc: Wei Liu <wei.liu@xxxxxxxxxx>
> > Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
> > ---
> >  drivers/net/xen-netback/common.h |  11 ---
> >  drivers/net/xen-netback/xenbus.c | 125 ++++---------------------------
> >  2 files changed, 14 insertions(+), 122 deletions(-)
> >
> > diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-
> netback/common.h
> > index 05847eb91a1b..e48da004c1a3 100644
> 
> <snip>
> 
> > -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);
> 
> have_hotplug_status_watch prevents xen-netback from switching to
> connected state unless the the backend scripts have written
> "hotplug-status" "success".  I had always thought that was intentional
> so the frontend doesn't connect when the backend is unconnected.  i.e.
> if the backend scripts fails, it writes "hotplug-status" "error" and
> the frontend doesn't connect.
> 
> That behavior is independent of using udev to run the scripts.  I'm
> not opposed to removing it, but I think it at least warrants
> mentioning in the commit message.

True, but it's probably related. The netback probe would previously kick udev, the hotplug script would then run, and then the state would go connected. I think, because the hotplug is invoked directly by the toolstack now, these things really ought not to be tied together. TBH I can't see any harm in the frontend seeing the network connection before the backend plumbing is done... If the frontend should have any sort of indication of whether the backend is plumbed or not then IMO it ought to be as a virtual carrier/link status, because unplumbing and re-plumbing could be done at any time really without any need for the shared ring to go away (and in fact I will be following up at some point with a patch to allow unbind and re-bind of netback).

I'll elaborate in the commit message as you suggest :-)

Cheers,

  Paul

> 
> Regards,
> Jason