Patchwork [2/8] netback: add module unload function

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

Comments

Wei Liu - Feb. 15, 2013, 4 p.m.
Enable users to unload netback module. Users should make sure there is not vif
runnig.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 drivers/net/xen-netback/common.h  |    1 +
 drivers/net/xen-netback/netback.c |   18 ++++++++++++++++++
 drivers/net/xen-netback/xenbus.c  |    5 +++++
 3 files changed, 24 insertions(+)
Konrad Rzeszutek Wilk - March 4, 2013, 8:55 p.m.
On Fri, Feb 15, 2013 at 04:00:03PM +0000, Wei Liu wrote:
> Enable users to unload netback module. Users should make sure there is not vif
> runnig.

'sure there are no vif's running.'

Any way of making this VIF part be automatic? Meaning that netback
can figure out if there are VIFs running and if so don't unload
all of the parts and just mention that you are leaking memory.

This looks quite dangerous - meaning if there are guests running and
we for fun do 'rmmod xen_netback' it looks like we could crash dom0?

> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  drivers/net/xen-netback/common.h  |    1 +
>  drivers/net/xen-netback/netback.c |   18 ++++++++++++++++++
>  drivers/net/xen-netback/xenbus.c  |    5 +++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> index 9d7f172..35d8772 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -120,6 +120,7 @@ void xenvif_get(struct xenvif *vif);
>  void xenvif_put(struct xenvif *vif);
>  
>  int xenvif_xenbus_init(void);
> +void xenvif_xenbus_exit(void);
>  
>  int xenvif_schedulable(struct xenvif *vif);
>  
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index db8d45a..de59098 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1761,5 +1761,23 @@ failed_init:
>  
>  module_init(netback_init);
>  
> +static void __exit netback_exit(void)
> +{
> +	int group, i;
> +	xenvif_xenbus_exit();

You should check the return code of this function.

> +	for (group = 0; group < xen_netbk_group_nr; group++) {
> +		struct xen_netbk *netbk = &xen_netbk[group];
> +		for (i = 0; i < MAX_PENDING_REQS; i++) {
> +			if (netbk->mmap_pages[i])
> +				__free_page(netbk->mmap_pages[i]);
> +		}
> +		del_timer_sync(&netbk->net_timer);
> +		kthread_stop(netbk->task);
> +	}
> +	vfree(xen_netbk);
> +}
> +
> +module_exit(netback_exit);
> +
>  MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_ALIAS("xen-backend:vif");
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 410018c..65d14f2 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -485,3 +485,8 @@ int xenvif_xenbus_init(void)
>  {
>  	return xenbus_register_backend(&netback_driver);
>  }
> +
> +void xenvif_xenbus_exit(void)
> +{
> +	return xenbus_unregister_driver(&netback_driver);
> +}
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> 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
Andrew Cooper - March 4, 2013, 8:58 p.m.
On 04/03/13 20:55, Konrad Rzeszutek Wilk wrote:
> On Fri, Feb 15, 2013 at 04:00:03PM +0000, Wei Liu wrote:
>> Enable users to unload netback module. Users should make sure there is not vif
>> runnig.
> 'sure there are no vif's running.'

If we are picking at grammar, no apostrophe in 'vifs'

~Andrew

>
> Any way of making this VIF part be automatic? Meaning that netback
> can figure out if there are VIFs running and if so don't unload
> all of the parts and just mention that you are leaking memory.
>
> This looks quite dangerous - meaning if there are guests running and
> we for fun do 'rmmod xen_netback' it looks like we could crash dom0?
>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  drivers/net/xen-netback/common.h  |    1 +
>>  drivers/net/xen-netback/netback.c |   18 ++++++++++++++++++
>>  drivers/net/xen-netback/xenbus.c  |    5 +++++
>>  3 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
>> index 9d7f172..35d8772 100644
>> --- a/drivers/net/xen-netback/common.h
>> +++ b/drivers/net/xen-netback/common.h
>> @@ -120,6 +120,7 @@ void xenvif_get(struct xenvif *vif);
>>  void xenvif_put(struct xenvif *vif);
>>  
>>  int xenvif_xenbus_init(void);
>> +void xenvif_xenbus_exit(void);
>>  
>>  int xenvif_schedulable(struct xenvif *vif);
>>  
>> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
>> index db8d45a..de59098 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -1761,5 +1761,23 @@ failed_init:
>>  
>>  module_init(netback_init);
>>  
>> +static void __exit netback_exit(void)
>> +{
>> +	int group, i;
>> +	xenvif_xenbus_exit();
> You should check the return code of this function.
>
>> +	for (group = 0; group < xen_netbk_group_nr; group++) {
>> +		struct xen_netbk *netbk = &xen_netbk[group];
>> +		for (i = 0; i < MAX_PENDING_REQS; i++) {
>> +			if (netbk->mmap_pages[i])
>> +				__free_page(netbk->mmap_pages[i]);
>> +		}
>> +		del_timer_sync(&netbk->net_timer);
>> +		kthread_stop(netbk->task);
>> +	}
>> +	vfree(xen_netbk);
>> +}
>> +
>> +module_exit(netback_exit);
>> +
>>  MODULE_LICENSE("Dual BSD/GPL");
>>  MODULE_ALIAS("xen-backend:vif");
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>> index 410018c..65d14f2 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -485,3 +485,8 @@ int xenvif_xenbus_init(void)
>>  {
>>  	return xenbus_register_backend(&netback_driver);
>>  }
>> +
>> +void xenvif_xenbus_exit(void)
>> +{
>> +	return xenbus_unregister_driver(&netback_driver);
>> +}
>> -- 
>> 1.7.10.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>
> _______________________________________________
> 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
stephen hemminger - March 4, 2013, 9:58 p.m.
On Fri, 15 Feb 2013 16:00:03 +0000
Wei Liu <wei.liu2@citrix.com> wrote:

> Enable users to unload netback module. Users should make sure there is not vif
> runnig.


Isn't it likely that some admin might be trying to cleanup or do shutdown and there
is an ordering problem. What happens if vif's are still running?

Why depend on users? You should allow it anytime and do refcounting and auto-destroy the vif's for them.
--
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 Mon, 2013-03-04 at 21:58 +0000, Stephen Hemminger wrote:
> On Fri, 15 Feb 2013 16:00:03 +0000
> Wei Liu <wei.liu2@citrix.com> wrote:
> 
> > Enable users to unload netback module. Users should make sure there is not vif
> > runnig.
> 
> 
> Isn't it likely that some admin might be trying to cleanup or do shutdown and there
> is an ordering problem. What happens if vif's are still running?
> 
> Why depend on users? You should allow it anytime and do refcounting and auto-destroy the vif's for them.

Sure, we should not depend on users. I will move my vif get/put module
patch before this one.


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
Wei Liu - March 5, 2013, 1:30 p.m.
On Mon, 2013-03-04 at 20:55 +0000, Konrad Rzeszutek Wilk wrote:
> On Fri, Feb 15, 2013 at 04:00:03PM +0000, Wei Liu wrote:
> > Enable users to unload netback module. Users should make sure there is not vif
> > runnig.
> 
> 'sure there are no vif's running.'
> 
> Any way of making this VIF part be automatic? Meaning that netback
> can figure out if there are VIFs running and if so don't unload
> all of the parts and just mention that you are leaking memory.
> 
> This looks quite dangerous - meaning if there are guests running and
> we for fun do 'rmmod xen_netback' it looks like we could crash dom0?
> 

Dom0 will not crash. But as you suggested in a latter email, I should
move the get/put module patch before this one.

The rationale behind this patch is that if there's anything wrong
discovered inside netback, we can just migrate all VMs to a new host,
unload old netback, load new netback then migrate all VMs back. This
should be useful for both production and testing.


Wei.

> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  drivers/net/xen-netback/common.h  |    1 +
> >  drivers/net/xen-netback/netback.c |   18 ++++++++++++++++++
> >  drivers/net/xen-netback/xenbus.c  |    5 +++++
> >  3 files changed, 24 insertions(+)
> > 
> > diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> > index 9d7f172..35d8772 100644
> > --- a/drivers/net/xen-netback/common.h
> > +++ b/drivers/net/xen-netback/common.h
> > @@ -120,6 +120,7 @@ void xenvif_get(struct xenvif *vif);
> >  void xenvif_put(struct xenvif *vif);
> >  
> >  int xenvif_xenbus_init(void);
> > +void xenvif_xenbus_exit(void);
> >  
> >  int xenvif_schedulable(struct xenvif *vif);
> >  
> > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> > index db8d45a..de59098 100644
> > --- a/drivers/net/xen-netback/netback.c
> > +++ b/drivers/net/xen-netback/netback.c
> > @@ -1761,5 +1761,23 @@ failed_init:
> >  
> >  module_init(netback_init);
> >  
> > +static void __exit netback_exit(void)
> > +{
> > +	int group, i;
> > +	xenvif_xenbus_exit();
> 
> You should check the return code of this function.
> 
> > +	for (group = 0; group < xen_netbk_group_nr; group++) {
> > +		struct xen_netbk *netbk = &xen_netbk[group];
> > +		for (i = 0; i < MAX_PENDING_REQS; i++) {
> > +			if (netbk->mmap_pages[i])
> > +				__free_page(netbk->mmap_pages[i]);
> > +		}
> > +		del_timer_sync(&netbk->net_timer);
> > +		kthread_stop(netbk->task);
> > +	}
> > +	vfree(xen_netbk);
> > +}
> > +
> > +module_exit(netback_exit);
> > +
> >  MODULE_LICENSE("Dual BSD/GPL");
> >  MODULE_ALIAS("xen-backend:vif");
> > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> > index 410018c..65d14f2 100644
> > --- a/drivers/net/xen-netback/xenbus.c
> > +++ b/drivers/net/xen-netback/xenbus.c
> > @@ -485,3 +485,8 @@ int xenvif_xenbus_init(void)
> >  {
> >  	return xenbus_register_backend(&netback_driver);
> >  }
> > +
> > +void xenvif_xenbus_exit(void)
> > +{
> > +	return xenbus_unregister_driver(&netback_driver);
> > +}
> > -- 
> > 1.7.10.4
> > 
> > 
> > _______________________________________________
> > 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/common.h b/drivers/net/xen-netback/common.h
index 9d7f172..35d8772 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -120,6 +120,7 @@  void xenvif_get(struct xenvif *vif);
 void xenvif_put(struct xenvif *vif);
 
 int xenvif_xenbus_init(void);
+void xenvif_xenbus_exit(void);
 
 int xenvif_schedulable(struct xenvif *vif);
 
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index db8d45a..de59098 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1761,5 +1761,23 @@  failed_init:
 
 module_init(netback_init);
 
+static void __exit netback_exit(void)
+{
+	int group, i;
+	xenvif_xenbus_exit();
+	for (group = 0; group < xen_netbk_group_nr; group++) {
+		struct xen_netbk *netbk = &xen_netbk[group];
+		for (i = 0; i < MAX_PENDING_REQS; i++) {
+			if (netbk->mmap_pages[i])
+				__free_page(netbk->mmap_pages[i]);
+		}
+		del_timer_sync(&netbk->net_timer);
+		kthread_stop(netbk->task);
+	}
+	vfree(xen_netbk);
+}
+
+module_exit(netback_exit);
+
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_ALIAS("xen-backend:vif");
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 410018c..65d14f2 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -485,3 +485,8 @@  int xenvif_xenbus_init(void)
 {
 	return xenbus_register_backend(&netback_driver);
 }
+
+void xenvif_xenbus_exit(void)
+{
+	return xenbus_unregister_driver(&netback_driver);
+}