Xen devel - Feb 2013 - [PATCH 3/8] netback: get/put module along with vif connect/disconnect

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

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

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
>

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

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.

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

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

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
>