| Message ID | 1360944010-15336-4-git-send-email-wei.liu2@citrix.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Feb 15, 2013 at 04:00:04PM +0000, Wei Liu wrote: > If there is vif running and user unloads netback, guest's network interface > just mysteriously stops working. So we need to prevent unloading netback > module if there is vif running. > > The disconnect function of vif may get called by the generic framework even > before vif connects, so thers is an extra check on whether we actually need to > put module when disconnecting a vif. Ah, I think this patch should come before the "netback: add module unload function" > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > drivers/net/xen-netback/interface.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c > index 221f426..db638e1 100644 > --- a/drivers/net/xen-netback/interface.c > +++ b/drivers/net/xen-netback/interface.c > @@ -314,6 +314,8 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref, > if (vif->irq) > return 0; > > + __module_get(THIS_MODULE); > + > err = xen_netbk_map_frontend_rings(vif, tx_ring_ref, rx_ring_ref); > if (err < 0) > goto err; > @@ -341,6 +343,7 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref, > err_unmap: > xen_netbk_unmap_frontend_rings(vif); > err: > + module_put(THIS_MODULE); > return err; > } > > @@ -358,18 +361,31 @@ void xenvif_carrier_off(struct xenvif *vif) > > void xenvif_disconnect(struct xenvif *vif) > { > + /* > + * This function may get called even before vif connets, set connects > + * need_module_put if vif->irq != 0, which means vif has > + * already connected, we should call module_put to balance the > + * previous __module_get. > + */ > + int need_module_put = 0; > + > if (netif_carrier_ok(vif->dev)) > xenvif_carrier_off(vif); > > atomic_dec(&vif->refcnt); > wait_event(vif->waiting_to_free, atomic_read(&vif->refcnt) == 0); > > - if (vif->irq) > + if (vif->irq) { > unbind_from_irqhandler(vif->irq, vif); > + need_module_put = 1; > + } > > unregister_netdev(vif->dev); > > xen_netbk_unmap_frontend_rings(vif); > > free_netdev(vif->dev); > + > + if (need_module_put) > + module_put(THIS_MODULE); > } > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15/02/13 16:00, Wei Liu wrote: > If there is vif running and user unloads netback, guest's network interface > just mysteriously stops working. So we need to prevent unloading netback > module if there is vif running. It's not mysterious -- it is cleanly disconnected, and will reconnect when the module is reinserted. Being able to unload modules while they are in use is standard so I don't think this should be applied. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-03-05 at 10:02 +0000, David Vrabel wrote: > On 15/02/13 16:00, Wei Liu wrote: > > If there is vif running and user unloads netback, guest's network interface > > just mysteriously stops working. So we need to prevent unloading netback > > module if there is vif running. > > It's not mysterious -- it is cleanly disconnected, and will reconnect > when the module is reinserted. > From a guest's POV, it just stops without any sign. This should be prevented IMHO. Netback / netfront lose all states when netback is unloaded. And netfront doesn't support reconfiguration at the moment. My guess is that this is the reason why netback doesn't even have unload function at first. > Being able to unload modules while they are in use is standard so I > don't think this should be applied. > I don't think this is true from a module dependency point of view - just try to unload any in use module, rmmod / modprobe will give you a fatal error. The situation for netback is a bit different. The module dependency is not within the same kernel, we can only do this by explicitly get/put this module. Wei. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/03/13 13:30, Wei Liu wrote: > On Tue, 2013-03-05 at 10:02 +0000, David Vrabel wrote: >> On 15/02/13 16:00, Wei Liu wrote: >>> If there is vif running and user unloads netback, guest's network interface >>> just mysteriously stops working. So we need to prevent unloading netback >>> module if there is vif running. >> >> It's not mysterious -- it is cleanly disconnected, and will reconnect >> when the module is reinserted. >> > > From a guest's POV, it just stops without any sign. This should be > prevented IMHO. This is a bug in the frontend or a bug in the backend failing to disconnect correctly. I posted a series of "xen-foofront: handle backend CLOSED without CLOSING" patches that may help here. (I didn't get applied to netfront for some reason.) Disabling module unload doesn't prevent this from happening away. You can always manually unbind the backend device from the xen-netback driver which has the same effect as unloading the module. > Netback / netfront lose all states when netback is unloaded. And > netfront doesn't support reconfiguration at the moment. My guess is that > this is the reason why netback doesn't even have unload function at > first. If netfront cannot handle reconnect then that's a bug in the frontend or a bug in the backend xenbus code not setting up the reconnect correctly. >> Being able to unload modules while they are in use is standard so I >> don't think this should be applied. > > I don't think this is true from a module dependency point of view - just > try to unload any in use module, rmmod / modprobe will give you a fatal > error. Try it with any other network interface driver and it will unload just fine. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-03-05 at 14:07 +0000, David Vrabel wrote: > On 05/03/13 13:30, Wei Liu wrote: > > On Tue, 2013-03-05 at 10:02 +0000, David Vrabel wrote: > >> On 15/02/13 16:00, Wei Liu wrote: > >>> If there is vif running and user unloads netback, guest's network interface > >>> just mysteriously stops working. So we need to prevent unloading netback > >>> module if there is vif running. > >> > >> It's not mysterious -- it is cleanly disconnected, and will reconnect > >> when the module is reinserted. > >> > > > > From a guest's POV, it just stops without any sign. This should be > > prevented IMHO. > > This is a bug in the frontend or a bug in the backend failing to > disconnect correctly. > > I posted a series of "xen-foofront: handle backend CLOSED without > CLOSING" patches that may help here. (I didn't get applied to netfront > for some reason.) > Any links? And the reason why it was not applied? > Disabling module unload doesn't prevent this from happening away. You > can always manually unbind the backend device from the xen-netback > driver which has the same effect as unloading the module. > Yes, but that's not a normal use case. > > Netback / netfront lose all states when netback is unloaded. And > > netfront doesn't support reconfiguration at the moment. My guess is that > > this is the reason why netback doesn't even have unload function at > > first. > > If netfront cannot handle reconnect then that's a bug in the frontend or > a bug in the backend xenbus code not setting up the reconnect correctly. > AFAICT, various frontends (hvc, fb, blk etc.) don't respond to Closing Closed Reconfigur{ing,ed} XenbusState. Is it the "bug" you're referring to? > >> Being able to unload modules while they are in use is standard so I > >> don't think this should be applied. > > > > I don't think this is true from a module dependency point of view - just > > try to unload any in use module, rmmod / modprobe will give you a fatal > > error. > > Try it with any other network interface driver and it will unload just fine. > I don't think we are talking about the same thing here... If you unload a network module in Dom0, that's fine, because you lose your interface in Dom0 as well. But for a DomU, frontend don't know about this. Wei. > David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 05, 2013 at 02:07:42PM +0000, David Vrabel wrote: > On 05/03/13 13:30, Wei Liu wrote: > > On Tue, 2013-03-05 at 10:02 +0000, David Vrabel wrote: > >> On 15/02/13 16:00, Wei Liu wrote: > >>> If there is vif running and user unloads netback, guest's network interface > >>> just mysteriously stops working. So we need to prevent unloading netback > >>> module if there is vif running. > >> > >> It's not mysterious -- it is cleanly disconnected, and will reconnect > >> when the module is reinserted. > >> > > > > From a guest's POV, it just stops without any sign. This should be > > prevented IMHO. > > This is a bug in the frontend or a bug in the backend failing to > disconnect correctly. > > I posted a series of "xen-foofront: handle backend CLOSED without > CLOSING" patches that may help here. (I didn't get applied to netfront > for some reason.) Hm, could you resent it please and make sure that the networking maintainer is on the To list? > > Disabling module unload doesn't prevent this from happening away. You > can always manually unbind the backend device from the xen-netback > driver which has the same effect as unloading the module. > > > Netback / netfront lose all states when netback is unloaded. And > > netfront doesn't support reconfiguration at the moment. My guess is that > > this is the reason why netback doesn't even have unload function at > > first. > > If netfront cannot handle reconnect then that's a bug in the frontend or > a bug in the backend xenbus code not setting up the reconnect correctly. > > >> Being able to unload modules while they are in use is standard so I > >> don't think this should be applied. > > > > I don't think this is true from a module dependency point of view - just > > try to unload any in use module, rmmod / modprobe will give you a fatal > > error. > > Try it with any other network interface driver and it will unload just fine. > > David > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c index 221f426..db638e1 100644 --- a/drivers/net/xen-netback/interface.c +++ b/drivers/net/xen-netback/interface.c @@ -314,6 +314,8 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref, if (vif->irq) return 0; + __module_get(THIS_MODULE); + err = xen_netbk_map_frontend_rings(vif, tx_ring_ref, rx_ring_ref); if (err < 0) goto err; @@ -341,6 +343,7 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref, err_unmap: xen_netbk_unmap_frontend_rings(vif); err: + module_put(THIS_MODULE); return err; } @@ -358,18 +361,31 @@ void xenvif_carrier_off(struct xenvif *vif) void xenvif_disconnect(struct xenvif *vif) { + /* + * This function may get called even before vif connets, set + * need_module_put if vif->irq != 0, which means vif has + * already connected, we should call module_put to balance the + * previous __module_get. + */ + int need_module_put = 0; + if (netif_carrier_ok(vif->dev)) xenvif_carrier_off(vif); atomic_dec(&vif->refcnt); wait_event(vif->waiting_to_free, atomic_read(&vif->refcnt) == 0); - if (vif->irq) + if (vif->irq) { unbind_from_irqhandler(vif->irq, vif); + need_module_put = 1; + } unregister_netdev(vif->dev); xen_netbk_unmap_frontend_rings(vif); free_netdev(vif->dev); + + if (need_module_put) + module_put(THIS_MODULE); }
If there is vif running and user unloads netback, guest's network interface just mysteriously stops working. So we need to prevent unloading netback module if there is vif running. The disconnect function of vif may get called by the generic framework even before vif connects, so thers is an extra check on whether we actually need to put module when disconnecting a vif. Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- drivers/net/xen-netback/interface.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)