Patchwork [3/8] netback: get/put module along with vif connect/disconnect

login
register
mail settings
Submitter Wei Liu
Date Feb. 15, 2013, 4 p.m.
Message ID <1360944010-15336-4-git-send-email-wei.liu2@citrix.com>
Download mbox | patch
Permalink /patch/220764/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Wei Liu - Feb. 15, 2013, 4 p.m.
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(-)
Konrad Rzeszutek Wilk - March 4, 2013, 8:56 p.m.
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
David Vrabel - March 5, 2013, 10:02 a.m.
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
Wei Liu - March 5, 2013, 1:30 p.m.
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
David Vrabel - March 5, 2013, 2:07 p.m.
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
Wei Liu - March 5, 2013, 2:44 p.m.
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
Konrad Rzeszutek Wilk - March 5, 2013, 3:53 p.m.
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

Patch

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);
 }