Re: [Xen-devel] [PATCH] xen-netfront: wait xenbus state change when load module manually

[Xiao Liang <xiliang@xxxxxxxxxx>]

Thanks, Boris

Please see my reply inline.


On 07/28/2018 02:40 AM, Boris Ostrovsky wrote:
> On 07/27/2018 05:56 AM, Xiao Liang wrote:
> > When loading module manually, after call xenbus_switch_state to initializes
> > the state of the netfront device, the driver state did not change so fast
> > that may lead no dev created in latest kernel. This patch adds wait to make
> > sure xenbus knows the driver is not in closed/unknown state.
> >
> > Current state:
> > [vm]# ethtool eth0
> > Settings for eth0:
> >         Link detected: yes
> > [vm]# modprobe -r xen_netfront
> > [vm]# modprobe  xen_netfront
> > [vm]# ethtool eth0
> > Settings for eth0:
> > Cannot get device settings: No such device
> > Cannot get wake-on-lan settings: No such device
> > Cannot get message level: No such device
> > Cannot get link status: No such device
> > No data available
> >
> > With the patch installed.
> > [vm]# ethtool eth0
> > Settings for eth0:
> >         Link detected: yes
> > [vm]# modprobe -r xen_netfront
> > [vm]# modprobe xen_netfront
> > [vm]# ethtool eth0
> > Settings for eth0:
> >         Link detected: yes
> >
> > Signed-off-by: Xiao Liang <xiliang@xxxxxxxxxx>
> > ---
> >   drivers/net/xen-netfront.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> > index a57daecf1d57..2d8812dd1534 100644
> > --- a/drivers/net/xen-netfront.c
> > +++ b/drivers/net/xen-netfront.c
> > @@ -87,6 +87,7 @@ struct netfront_cb {
> >   /* IRQ name is queue name with "-tx" or "-rx" appended */
> >   #define IRQ_NAME_SIZE (QUEUE_NAME_SIZE + 3)
> >   
> > +static DECLARE_WAIT_QUEUE_HEAD(module_load_q);
> >   static DECLARE_WAIT_QUEUE_HEAD(module_unload_q);
> >   
> >   struct netfront_stats {
> > @@ -1330,6 +1331,11 @@ static struct net_device *xennet_create_dev(struct 
> > xenbus_device *dev)
> >         netif_carrier_off(netdev);
> >   
> >   	xenbus_switch_state(dev, XenbusStateInitialising);
> > +       wait_event(module_load_q,
> > +                          xenbus_read_driver_state(dev->otherend) !=
> > +                          XenbusStateClosed &&
> > +                          xenbus_read_driver_state(dev->otherend) !=
> > +                          XenbusStateUnknown);
> >         return netdev;
> >   
> >    exit:
> Should we have a wake_up somewhere?
In my understanding, netback_changed handles it if dev state is in 
XenbusStateInitialising and otherend is in XenbusStateInitWait, and then 
create connection to backend.
But in most cases, it breaks out as dev->state not in 
XenbusStateInitialising. So I added a wait here.
> And what about other states --- is,
> for example, XenbusStateClosing a valid reason to continue?
I think XenbusStateClosing should not be a valid reason to continue.
My purpose is waiting otherend status to be XenbusStateInitWait(after 
new dev created).To avoid unnecessary impact, I  only check it leaves 
closed and unknow state in this patch.
In my testing, hotplug vifs from guest in host or load/unload module in 
guest over 100 times, only waiting XenbusStateInitWait or as this patch 
does, both are working.
vifs can be created each time successfully.

Thanks,
Xiao Liang

> -boris
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel