[PATCH] drop_monitor: convert to modular building

[PATCH] drop_monitor: convert to modular building

[Neil Horman]

When I first wrote drop monitor I wrote it to just build monolithically.  There
is no reason it can't be built modularly as well, so lets give it that
flexibiity.

I've tested this by building it as both a module and monolithically, and it
seems to work quite well

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
---
 net/Kconfig             |    2 +-
 net/core/drop_monitor.c |   46 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/net/Kconfig b/net/Kconfig
index e07272d..76ad6fa 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -295,7 +295,7 @@ config NET_TCPPROBE
 	module will be called tcp_probe.
 
 config NET_DROP_MONITOR
-	boolean "Network packet drop alerting service"
+	tristate "Network packet drop alerting service"
 	depends on INET && EXPERIMENTAL && TRACEPOINTS
 	---help---
 	This feature provides an alerting service to userspace in the
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 5c3c81a..145f399 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -22,6 +22,7 @@
 #include <linux/timer.h>
 #include <linux/bitops.h>
 #include <linux/slab.h>
+#include <linux/module.h>
 #include <net/genetlink.h>
 #include <net/netevent.h>
 
@@ -223,9 +224,15 @@ static int set_all_monitor_traces(int state)
 
 	switch (state) {
 	case TRACE_ON:
+		if (!try_module_get(THIS_MODULE)) {
+			rc = -EINVAL;
+			break;
+		}
+
 		rc |= register_trace_kfree_skb(trace_kfree_skb_hit, NULL);
 		rc |= register_trace_napi_poll(trace_napi_poll_hit, NULL);
 		break;
+
 	case TRACE_OFF:
 		rc |= unregister_trace_kfree_skb(trace_kfree_skb_hit, NULL);
 		rc |= unregister_trace_napi_poll(trace_napi_poll_hit, NULL);
@@ -241,6 +248,9 @@ static int set_all_monitor_traces(int state)
 				kfree_rcu(new_stat, rcu);
 			}
 		}
+
+		module_put(THIS_MODULE);
+
 		break;
 	default:
 		rc = 1;
@@ -383,4 +393,38 @@ out:
 	return rc;
 }
 
-late_initcall(init_net_drop_monitor);
+static void exit_net_drop_monitor(void)
+{
+	struct per_cpu_dm_data *data;
+	int cpu;
+
+	if (unregister_netdevice_notifier(&dropmon_net_notifier))
+		printk(KERN_WARNING "Unable to unregiser dropmon notifer\n");
+
+	/*
+	 * Because of the module_get/put we do in the trace state change path
+	 * we are guarnateed not to have any current users when we get here
+	 * all we need to do is make sure that we don't have any running timers
+	 * or pending schedule calls
+	 */
+
+	for_each_present_cpu(cpu) {
+		data = &per_cpu(dm_cpu_data, cpu);
+		del_timer(&data->send_timer);
+		cancel_work_sync(&data->dm_alert_work);
+		/*
+		 * At this point, we should have exclusive access
+		 * to this struct and can free the skb inside it
+		 */
+		kfree_skb(data->skb);
+	}
+
+	if (genl_unregister_family(&net_drop_monitor_family))
+		printk(KERN_WARNING "Unable to unregister drop monitor socket family\n");
+}
+
+module_init(init_net_drop_monitor);
+module_exit(exit_net_drop_monitor);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Neil Horman <nhorman@tuxdriver.com>");
-- 
1.7.7.6

Re: [PATCH] drop_monitor: convert to modular building

[Eric Dumazet]

On Wed, 2012-05-16 at 10:27 -0400, Neil Horman wrote:
> When I first wrote drop monitor I wrote it to just build monolithically.  There
> is no reason it can't be built modularly as well, so lets give it that
> flexibiity.

> +	for_each_present_cpu(cpu) {
> +		data = &per_cpu(dm_cpu_data, cpu);
> +		del_timer(&data->send_timer);
> +		cancel_work_sync(&data->dm_alert_work);
> +		/*
> +		 * At this point, we should have exclusive access
> +		 * to this struct and can free the skb inside it
> +		 */
> +		kfree_skb(data->skb);
> +	}
> +

I dont think for_each_present_cpu(cpu) is right 

(I realize drop_monitor already uses this, but its a bug)

To use it, you must have a notifier to react to cpu HOTPLUG events.

-> for_each_possible_cpu() is more correct.

Also, please dont add new printk(KERN_WARNING ...), use pr_warn(...)
instead

Re: [PATCH] drop_monitor: convert to modular building

[Neil Horman]

On Wed, May 16, 2012 at 04:48:01PM +0200, Eric Dumazet wrote:
> On Wed, 2012-05-16 at 10:27 -0400, Neil Horman wrote:
> > When I first wrote drop monitor I wrote it to just build monolithically.  There
> > is no reason it can't be built modularly as well, so lets give it that
> > flexibiity.
> 
> > +	for_each_present_cpu(cpu) {
> > +		data = &per_cpu(dm_cpu_data, cpu);
> > +		del_timer(&data->send_timer);
> > +		cancel_work_sync(&data->dm_alert_work);
> > +		/*
> > +		 * At this point, we should have exclusive access
> > +		 * to this struct and can free the skb inside it
> > +		 */
> > +		kfree_skb(data->skb);
> > +	}
> > +
> 
> I dont think for_each_present_cpu(cpu) is right 
> 
> (I realize drop_monitor already uses this, but its a bug)
> 
> To use it, you must have a notifier to react to cpu HOTPLUG events.
> 
> -> for_each_possible_cpu() is more correct.
> 
Ok, i can do that.
> Also, please dont add new printk(KERN_WARNING ...), use pr_warn(...)
> instead
> 
Ack, I'll add a patch to this series to convert the existing printks to their
corresponding pr_* macros
Neil

> 
> 
> 

Re: [PATCH] drop_monitor: convert to modular building

[Ben Hutchings]

On Wed, 2012-05-16 at 10:27 -0400, Neil Horman wrote:
> When I first wrote drop monitor I wrote it to just build monolithically.  There
> is no reason it can't be built modularly as well, so lets give it that
> flexibiity.

Yes, please.

[...]
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -22,6 +22,7 @@
>  #include <linux/timer.h>
>  #include <linux/bitops.h>
>  #include <linux/slab.h>
> +#include <linux/module.h>
>  #include <net/genetlink.h>
>  #include <net/netevent.h>
>  
> @@ -223,9 +224,15 @@ static int set_all_monitor_traces(int state)
>  
>  	switch (state) {
>  	case TRACE_ON:
> +		if (!try_module_get(THIS_MODULE)) {
> +			rc = -EINVAL;

Minor issue, but this isn't the right error code - there is nothing
invalid about the request, it just came at the wrong time.  Perhaps
ENODEV or ECANCELED?

> +			break;
> +		}
> +
>  		rc |= register_trace_kfree_skb(trace_kfree_skb_hit, NULL);
>  		rc |= register_trace_napi_poll(trace_napi_poll_hit, NULL);
>  		break;
> +
>  	case TRACE_OFF:
>  		rc |= unregister_trace_kfree_skb(trace_kfree_skb_hit, NULL);
>  		rc |= unregister_trace_napi_poll(trace_napi_poll_hit, NULL);
> @@ -241,6 +248,9 @@ static int set_all_monitor_traces(int state)
>  				kfree_rcu(new_stat, rcu);
>  			}
>  		}
> +
> +		module_put(THIS_MODULE);
> +
>  		break;
>  	default:
>  		rc = 1;
> @@ -383,4 +393,38 @@ out:
>  	return rc;
>  }
>  
> -late_initcall(init_net_drop_monitor);
> +static void exit_net_drop_monitor(void)
> +{
> +	struct per_cpu_dm_data *data;
> +	int cpu;
> +
> +	if (unregister_netdevice_notifier(&dropmon_net_notifier))
> +		printk(KERN_WARNING "Unable to unregiser dropmon notifer\n");

Currently this will only fail if you didn't actually register the
notifier, which would be a bug.  If there is ever any other reason this
could fail, continuing with the notifier still registered would be
disastrous.  Therefore I think this should be:

	rc = unregister_netdevice_notifier(&dropmon_net_notifier);
	BUG_ON(rc);

> +	/*
> +	 * Because of the module_get/put we do in the trace state change path
> +	 * we are guarnateed not to have any current users when we get here
> +	 * all we need to do is make sure that we don't have any running timers
> +	 * or pending schedule calls
> +	 */

Surely you need to call set_all_monitor_traces(TRACE_OFF) first...

> +	for_each_present_cpu(cpu) {
> +		data = &per_cpu(dm_cpu_data, cpu);
> +		del_timer(&data->send_timer);
> +		cancel_work_sync(&data->dm_alert_work);
> +		/*
> +		 * At this point, we should have exclusive access
> +		 * to this struct and can free the skb inside it
> +		 */
> +		kfree_skb(data->skb);
> +	}
> +
> +	if (genl_unregister_family(&net_drop_monitor_family))
> +		printk(KERN_WARNING "Unable to unregister drop monitor socket family\n");

Same issue as with unregister_netdevice_notifier().

Ben.

> +}
> +
> +module_init(init_net_drop_monitor);
> +module_exit(exit_net_drop_monitor);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Neil Horman <nhorman@tuxdriver.com>");

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] drop_monitor: convert to modular building

[Neil Horman]

On Wed, May 16, 2012 at 04:01:45PM +0100, Ben Hutchings wrote:
> On Wed, 2012-05-16 at 10:27 -0400, Neil Horman wrote:
> > When I first wrote drop monitor I wrote it to just build monolithically.  There
> > is no reason it can't be built modularly as well, so lets give it that
> > flexibiity.
> 
> Yes, please.
> 
> [...]
> > --- a/net/core/drop_monitor.c
> > +++ b/net/core/drop_monitor.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/timer.h>
> >  #include <linux/bitops.h>
> >  #include <linux/slab.h>
> > +#include <linux/module.h>
> >  #include <net/genetlink.h>
> >  #include <net/netevent.h>
> >  
> > @@ -223,9 +224,15 @@ static int set_all_monitor_traces(int state)
> >  
> >  	switch (state) {
> >  	case TRACE_ON:
> > +		if (!try_module_get(THIS_MODULE)) {
> > +			rc = -EINVAL;
> 
> Minor issue, but this isn't the right error code - there is nothing
> invalid about the request, it just came at the wrong time.  Perhaps
> ENODEV or ECANCELED?
> 
Yeah, ok, ENODEV seems reasonable.

> > +			break;
> > +		}
> > +
> >  		rc |= register_trace_kfree_skb(trace_kfree_skb_hit, NULL);
> >  		rc |= register_trace_napi_poll(trace_napi_poll_hit, NULL);
> >  		break;
> > +
> >  	case TRACE_OFF:
> >  		rc |= unregister_trace_kfree_skb(trace_kfree_skb_hit, NULL);
> >  		rc |= unregister_trace_napi_poll(trace_napi_poll_hit, NULL);
> > @@ -241,6 +248,9 @@ static int set_all_monitor_traces(int state)
> >  				kfree_rcu(new_stat, rcu);
> >  			}
> >  		}
> > +
> > +		module_put(THIS_MODULE);
> > +
> >  		break;
> >  	default:
> >  		rc = 1;
> > @@ -383,4 +393,38 @@ out:
> >  	return rc;
> >  }
> >  
> > -late_initcall(init_net_drop_monitor);
> > +static void exit_net_drop_monitor(void)
> > +{
> > +	struct per_cpu_dm_data *data;
> > +	int cpu;
> > +
> > +	if (unregister_netdevice_notifier(&dropmon_net_notifier))
> > +		printk(KERN_WARNING "Unable to unregiser dropmon notifer\n");
> 
> Currently this will only fail if you didn't actually register the
> notifier, which would be a bug.  If there is ever any other reason this
> could fail, continuing with the notifier still registered would be
> disastrous.  Therefore I think this should be:
> 
> 	rc = unregister_netdevice_notifier(&dropmon_net_notifier);
> 	BUG_ON(rc);
> 
Ok, seems reasonable.

> > +	/*
> > +	 * Because of the module_get/put we do in the trace state change path
> > +	 * we are guarnateed not to have any current users when we get here
> > +	 * all we need to do is make sure that we don't have any running timers
> > +	 * or pending schedule calls
> > +	 */
> 
> Surely you need to call set_all_monitor_traces(TRACE_OFF) first...
> 
Nope, If you'll note the code higher up in the patch, I use try_module_get and
module_put to prevent the module unload code from getting here while anyone is
actually using the protocol.  Once we are in the module remove routine here, we
are guaranateed that there are no users of this protocol, and that the
tracepoints are all unregistered.

> > +	for_each_present_cpu(cpu) {
> > +		data = &per_cpu(dm_cpu_data, cpu);
> > +		del_timer(&data->send_timer);
> > +		cancel_work_sync(&data->dm_alert_work);
> > +		/*
> > +		 * At this point, we should have exclusive access
> > +		 * to this struct and can free the skb inside it
> > +		 */
> > +		kfree_skb(data->skb);
> > +	}
> > +
> > +	if (genl_unregister_family(&net_drop_monitor_family))
> > +		printk(KERN_WARNING "Unable to unregister drop monitor socket family\n");
> 
> Same issue as with unregister_netdevice_notifier().
> 
ack, I'll update that to a BUG_ON
Neil

Re: [PATCH] drop_monitor: convert to modular building

[Ben Hutchings]

On Wed, 2012-05-16 at 11:19 -0400, Neil Horman wrote:
> On Wed, May 16, 2012 at 04:01:45PM +0100, Ben Hutchings wrote:
> > On Wed, 2012-05-16 at 10:27 -0400, Neil Horman wrote:
> > > When I first wrote drop monitor I wrote it to just build monolithically.  There
> > > is no reason it can't be built modularly as well, so lets give it that
> > > flexibiity.
[...]
> > > +	/*
> > > +	 * Because of the module_get/put we do in the trace state change path
> > > +	 * we are guarnateed not to have any current users when we get here
> > > +	 * all we need to do is make sure that we don't have any running timers
> > > +	 * or pending schedule calls
> > > +	 */
> > 
> > Surely you need to call set_all_monitor_traces(TRACE_OFF) first...
> > 
> Nope, If you'll note the code higher up in the patch, I use try_module_get and
> module_put to prevent the module unload code from getting here while anyone is
> actually using the protocol.  Once we are in the module remove routine here, we
> are guaranateed that there are no users of this protocol, and that the
> tracepoints are all unregistered.
[...]

Yes, of course.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH v2] drop_monitor: convert to modular building

[Neil Horman]

When I first wrote drop monitor I wrote it to just build monolithically.  There
is no reason it can't be built modularly as well, so lets give it that
flexibiity.

I've tested this by building it as both a module and monolithically, and it
seems to work quite well

Change notes:

v2)
* fixed for_each_present_cpu loops to be more correct as per Eric D.
* Converted exit path failures to BUG_ON as per Ben H.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Ben Hutchings <bhutchings@solarflare.com>
---
 net/Kconfig             |    2 +-
 net/core/drop_monitor.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/net/Kconfig b/net/Kconfig
index e07272d..76ad6fa 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -295,7 +295,7 @@ config NET_TCPPROBE
 	module will be called tcp_probe.
 
 config NET_DROP_MONITOR
-	boolean "Network packet drop alerting service"
+	tristate "Network packet drop alerting service"
 	depends on INET && EXPERIMENTAL && TRACEPOINTS
 	---help---
 	This feature provides an alerting service to userspace in the
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index cfeeef8..b6760a6 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -24,6 +24,7 @@
 #include <linux/timer.h>
 #include <linux/bitops.h>
 #include <linux/slab.h>
+#include <linux/module.h>
 #include <net/genetlink.h>
 #include <net/netevent.h>
 
@@ -225,9 +226,15 @@ static int set_all_monitor_traces(int state)
 
 	switch (state) {
 	case TRACE_ON:
+		if (!try_module_get(THIS_MODULE)) {
+			rc = -ENODEV;
+			break;
+		}
+
 		rc |= register_trace_kfree_skb(trace_kfree_skb_hit, NULL);
 		rc |= register_trace_napi_poll(trace_napi_poll_hit, NULL);
 		break;
+
 	case TRACE_OFF:
 		rc |= unregister_trace_kfree_skb(trace_kfree_skb_hit, NULL);
 		rc |= unregister_trace_napi_poll(trace_napi_poll_hit, NULL);
@@ -243,6 +250,9 @@ static int set_all_monitor_traces(int state)
 				kfree_rcu(new_stat, rcu);
 			}
 		}
+
+		module_put(THIS_MODULE);
+
 		break;
 	default:
 		rc = 1;
@@ -368,7 +378,7 @@ static int __init init_net_drop_monitor(void)
 
 	rc = 0;
 
-	for_each_present_cpu(cpu) {
+	for_each_possible_cpu(cpu) {
 		data = &per_cpu(dm_cpu_data, cpu);
 		reset_per_cpu_data(data);
 		INIT_WORK(&data->dm_alert_work, send_dm_alert);
@@ -385,4 +395,36 @@ out:
 	return rc;
 }
 
-late_initcall(init_net_drop_monitor);
+static void exit_net_drop_monitor(void)
+{
+	struct per_cpu_dm_data *data;
+	int cpu;
+
+	BUG_ON(unregister_netdevice_notifier(&dropmon_net_notifier));
+
+	/*
+	 * Because of the module_get/put we do in the trace state change path
+	 * we are guarnateed not to have any current users when we get here
+	 * all we need to do is make sure that we don't have any running timers
+	 * or pending schedule calls
+	 */
+
+	for_each_possible_cpu(cpu) {
+		data = &per_cpu(dm_cpu_data, cpu);
+		del_timer(&data->send_timer);
+		cancel_work_sync(&data->dm_alert_work);
+		/*
+		 * At this point, we should have exclusive access
+		 * to this struct and can free the skb inside it
+		 */
+		kfree_skb(data->skb);
+	}
+
+	BUG_ON(genl_unregister_family(&net_drop_monitor_family));
+}
+
+module_init(init_net_drop_monitor);
+module_exit(exit_net_drop_monitor);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Neil Horman <nhorman@tuxdriver.com>");
-- 
1.7.7.6

Re: [PATCH v2] drop_monitor: convert to modular building

[Ben Hutchings]

On Thu, 2012-05-17 at 13:49 -0400, Neil Horman wrote:
> When I first wrote drop monitor I wrote it to just build monolithically.  There
> is no reason it can't be built modularly as well, so lets give it that
> flexibiity.
> 
> I've tested this by building it as both a module and monolithically, and it
> seems to work quite well
> 
> Change notes:
> 
> v2)
> * fixed for_each_present_cpu loops to be more correct as per Eric D.
> * Converted exit path failures to BUG_ON as per Ben H.

Sorry I didn't pick up on this the first time:

[...]
> -late_initcall(init_net_drop_monitor);
> +static void exit_net_drop_monitor(void)
> +{
> +	struct per_cpu_dm_data *data;
> +	int cpu;
> +
> +	BUG_ON(unregister_netdevice_notifier(&dropmon_net_notifier));
> +
> +	/*
> +	 * Because of the module_get/put we do in the trace state change path
> +	 * we are guarnateed not to have any current users when we get here
> +	 * all we need to do is make sure that we don't have any running timers
> +	 * or pending schedule calls
> +	 */
> +
> +	for_each_possible_cpu(cpu) {
> +		data = &per_cpu(dm_cpu_data, cpu);
> +		del_timer(&data->send_timer);

Doesn't this need to be del_timer_sync()?

Ben.

> +		cancel_work_sync(&data->dm_alert_work);
> +		/*
> +		 * At this point, we should have exclusive access
> +		 * to this struct and can free the skb inside it
> +		 */
> +		kfree_skb(data->skb);
> +	}
> +
> +	BUG_ON(genl_unregister_family(&net_drop_monitor_family));
> +}
> +
> +module_init(init_net_drop_monitor);
> +module_exit(exit_net_drop_monitor);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Neil Horman <nhorman@tuxdriver.com>");

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH v2] drop_monitor: convert to modular building

[Neil Horman]

On Thu, May 17, 2012 at 07:14:05PM +0100, Ben Hutchings wrote:
> On Thu, 2012-05-17 at 13:49 -0400, Neil Horman wrote:
> > When I first wrote drop monitor I wrote it to just build monolithically.  There
> > is no reason it can't be built modularly as well, so lets give it that
> > flexibiity.
> > 
> > I've tested this by building it as both a module and monolithically, and it
> > seems to work quite well
> > 
> > Change notes:
> > 
> > v2)
> > * fixed for_each_present_cpu loops to be more correct as per Eric D.
> > * Converted exit path failures to BUG_ON as per Ben H.
> 
> Sorry I didn't pick up on this the first time:
> 
> [...]
> > -late_initcall(init_net_drop_monitor);
> > +static void exit_net_drop_monitor(void)
> > +{
> > +	struct per_cpu_dm_data *data;
> > +	int cpu;
> > +
> > +	BUG_ON(unregister_netdevice_notifier(&dropmon_net_notifier));
> > +
> > +	/*
> > +	 * Because of the module_get/put we do in the trace state change path
> > +	 * we are guarnateed not to have any current users when we get here
> > +	 * all we need to do is make sure that we don't have any running timers
> > +	 * or pending schedule calls
> > +	 */
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		data = &per_cpu(dm_cpu_data, cpu);
> > +		del_timer(&data->send_timer);
> 
> Doesn't this need to be del_timer_sync()?
> 
Yeah, good catch.  I was thinking it didn't need to be as the timer doesn't
re-arm itself and the cancel_work_sync would undo anything that a running timer
did, but thinking about it, its possible that a timer could fire on cpu A, and
cpu B could execute and complete the cancel_work_sync prior to cpu A scheduling
it, so there is a race window there.  I'll fix that up.
Neil
 

[PATCH v3] drop_monitor: convert to modular building

[Neil Horman]

When I first wrote drop monitor I wrote it to just build monolithically.  There
is no reason it can't be built modularly as well, so lets give it that
flexibiity.

I've tested this by building it as both a module and monolithically, and it
seems to work quite well

Change notes:

v2)
* fixed for_each_present_cpu loops to be more correct as per Eric D.
* Converted exit path failures to BUG_ON as per Ben H.

v3)
* Converted del_timer to del_timer_sync to close race noted by Ben H.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Ben Hutchings <bhutchings@solarflare.com>
---
 net/Kconfig             |    2 +-
 net/core/drop_monitor.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/net/Kconfig b/net/Kconfig
index e07272d..76ad6fa 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -295,7 +295,7 @@ config NET_TCPPROBE
 	module will be called tcp_probe.
 
 config NET_DROP_MONITOR
-	boolean "Network packet drop alerting service"
+	tristate "Network packet drop alerting service"
 	depends on INET && EXPERIMENTAL && TRACEPOINTS
 	---help---
 	This feature provides an alerting service to userspace in the
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index cfeeef8..f93f985 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -24,6 +24,7 @@
 #include <linux/timer.h>
 #include <linux/bitops.h>
 #include <linux/slab.h>
+#include <linux/module.h>
 #include <net/genetlink.h>
 #include <net/netevent.h>
 
@@ -225,9 +226,15 @@ static int set_all_monitor_traces(int state)
 
 	switch (state) {
 	case TRACE_ON:
+		if (!try_module_get(THIS_MODULE)) {
+			rc = -ENODEV;
+			break;
+		}
+
 		rc |= register_trace_kfree_skb(trace_kfree_skb_hit, NULL);
 		rc |= register_trace_napi_poll(trace_napi_poll_hit, NULL);
 		break;
+
 	case TRACE_OFF:
 		rc |= unregister_trace_kfree_skb(trace_kfree_skb_hit, NULL);
 		rc |= unregister_trace_napi_poll(trace_napi_poll_hit, NULL);
@@ -243,6 +250,9 @@ static int set_all_monitor_traces(int state)
 				kfree_rcu(new_stat, rcu);
 			}
 		}
+
+		module_put(THIS_MODULE);
+
 		break;
 	default:
 		rc = 1;
@@ -368,7 +378,7 @@ static int __init init_net_drop_monitor(void)
 
 	rc = 0;
 
-	for_each_present_cpu(cpu) {
+	for_each_possible_cpu(cpu) {
 		data = &per_cpu(dm_cpu_data, cpu);
 		reset_per_cpu_data(data);
 		INIT_WORK(&data->dm_alert_work, send_dm_alert);
@@ -385,4 +395,36 @@ out:
 	return rc;
 }
 
-late_initcall(init_net_drop_monitor);
+static void exit_net_drop_monitor(void)
+{
+	struct per_cpu_dm_data *data;
+	int cpu;
+
+	BUG_ON(unregister_netdevice_notifier(&dropmon_net_notifier));
+
+	/*
+	 * Because of the module_get/put we do in the trace state change path
+	 * we are guarnateed not to have any current users when we get here
+	 * all we need to do is make sure that we don't have any running timers
+	 * or pending schedule calls
+	 */
+
+	for_each_possible_cpu(cpu) {
+		data = &per_cpu(dm_cpu_data, cpu);
+		del_timer_sync(&data->send_timer);
+		cancel_work_sync(&data->dm_alert_work);
+		/*
+		 * At this point, we should have exclusive access
+		 * to this struct and can free the skb inside it
+		 */
+		kfree_skb(data->skb);
+	}
+
+	BUG_ON(genl_unregister_family(&net_drop_monitor_family));
+}
+
+module_init(init_net_drop_monitor);
+module_exit(exit_net_drop_monitor);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Neil Horman <nhorman@tuxdriver.com>");
-- 
1.7.7.6

Re: [PATCH v3] drop_monitor: convert to modular building

[Ben Hutchings]

On Thu, 2012-05-17 at 16:04 -0400, Neil Horman wrote:
> When I first wrote drop monitor I wrote it to just build monolithically.  There
> is no reason it can't be built modularly as well, so lets give it that
> flexibiity.
> 
> I've tested this by building it as both a module and monolithically, and it
> seems to work quite well
> 
> Change notes:
> 
> v2)
> * fixed for_each_present_cpu loops to be more correct as per Eric D.
> * Converted exit path failures to BUG_ON as per Ben H.
> 
> v3)
> * Converted del_timer to del_timer_sync to close race noted by Ben H.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Ben Hutchings <bhutchings@solarflare.com>
[...]

Reviewed-by: Ben Hutchings <bhutchings@solarflare.com>

Thanks,
Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH v3] drop_monitor: convert to modular building

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Thu, 17 May 2012 16:04:00 -0400

> When I first wrote drop monitor I wrote it to just build monolithically.  There
> is no reason it can't be built modularly as well, so lets give it that
> flexibiity.
> 
> I've tested this by building it as both a module and monolithically, and it
> seems to work quite well
> 
> Change notes:
> 
> v2)
> * fixed for_each_present_cpu loops to be more correct as per Eric D.
> * Converted exit path failures to BUG_ON as per Ben H.
> 
> v3)
> * Converted del_timer to del_timer_sync to close race noted by Ben H.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Applied, althrough it didn't apply cleanly to net-next.

Re: [PATCH v3] drop_monitor: convert to modular building

[Neil Horman]

On Thu, May 17, 2012 at 04:09:37PM -0400, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Thu, 17 May 2012 16:04:00 -0400
> 
> > When I first wrote drop monitor I wrote it to just build monolithically.  There
> > is no reason it can't be built modularly as well, so lets give it that
> > flexibiity.
> > 
> > I've tested this by building it as both a module and monolithically, and it
> > seems to work quite well
> > 
> > Change notes:
> > 
> > v2)
> > * fixed for_each_present_cpu loops to be more correct as per Eric D.
> > * Converted exit path failures to BUG_ON as per Ben H.
> > 
> > v3)
> > * Converted del_timer to del_timer_sync to close race noted by Ben H.
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> Applied, althrough it didn't apply cleanly to net-next.
> 

Apologies Dave, should have told you that I was carrying Joe P.'s cleanup patch
in my net-next tree as well:
http://marc.info/?l=linux-netdev&m=133727344816140&w=2

Since you noted that you had applied it, I applied it myself here.
Neil

Re: [PATCH v3] drop_monitor: convert to modular building

[Eric Dumazet]

On Thu, 2012-05-17 at 16:21 -0400, Neil Horman wrote:
> On Thu, May 17, 2012 at 04:09:37PM -0400, David Miller wrote:

> > 
> > Applied, althrough it didn't apply cleanly to net-next.
> > 
> 
> Apologies Dave, should have told you that I was carrying Joe P.'s cleanup patch
> in my net-next tree as well:
> http://marc.info/?l=linux-netdev&m=133727344816140&w=2
> 
> Since you noted that you had applied it, I applied it myself here.
> Neil
> 

Any plan to autoload drop_monitor module from dropwatch,
or issuing some advice ?

# dropwatch -l kas
Unable to find NET_DM family, dropwatch can't work
Cleanuing up on socket creation error

Thanks

Re: [PATCH v3] drop_monitor: convert to modular building

[Neil Horman]

On Tue, May 22, 2012 at 03:05:19PM +0200, Eric Dumazet wrote:
> On Thu, 2012-05-17 at 16:21 -0400, Neil Horman wrote:
> > On Thu, May 17, 2012 at 04:09:37PM -0400, David Miller wrote:
> 
> > > 
> > > Applied, althrough it didn't apply cleanly to net-next.
> > > 
> > 
> > Apologies Dave, should have told you that I was carrying Joe P.'s cleanup patch
> > in my net-next tree as well:
> > http://marc.info/?l=linux-netdev&m=133727344816140&w=2
> > 
> > Since you noted that you had applied it, I applied it myself here.
> > Neil
> > 
> 
> Any plan to autoload drop_monitor module from dropwatch,
> or issuing some advice ?
> 
> # dropwatch -l kas
> Unable to find NET_DM family, dropwatch can't work
> Cleanuing up on socket creation error
> 
> Thanks
> 
I'm looking into that currently, although I was starting to wonder if its
possible to do with a generic netlink socket.  I can't seem to find any
examples, and I can't use the net-pf-* module alias mechanism that formal
protocols implement, since I don't have a defined address family.  I suppose I
could augment that format to support a net-pf-16-<name> alias, where name is the
name of the genl family that gets registered by the module you're looking for.

Does that seem like a reasonable idea?
Neil

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

Re: [PATCH v3] drop_monitor: convert to modular building

[Neil Horman]

On Tue, May 22, 2012 at 03:05:19PM +0200, Eric Dumazet wrote:
> On Thu, 2012-05-17 at 16:21 -0400, Neil Horman wrote:
> > On Thu, May 17, 2012 at 04:09:37PM -0400, David Miller wrote:
> 
> > > 
> > > Applied, althrough it didn't apply cleanly to net-next.
> > > 
> > 
> > Apologies Dave, should have told you that I was carrying Joe P.'s cleanup patch
> > in my net-next tree as well:
> > http://marc.info/?l=linux-netdev&m=133727344816140&w=2
> > 
> > Since you noted that you had applied it, I applied it myself here.
> > Neil
> > 
> 
> Any plan to autoload drop_monitor module from dropwatch,
> or issuing some advice ?
> 
> # dropwatch -l kas
> Unable to find NET_DM family, dropwatch can't work
> Cleanuing up on socket creation error
> 
> Thanks
> 
> 
> 

Eric,
	Just FYI, I sent a series upstream to implement autoloading of generic
netlink families.  Please be awarem, that I've tested these with a hacked
version of dropwatch, and it works great, but with the normal version of
dropwatch, the drop_monitor module still doesn't autoload.  This is due to libnl
not explicitly requesting a family when genl_ctrl_family_resolve is called.
Instead of trying to load the module, it dumps the existing registered families
via a NLM_F_DUMP message.  I'm working on updating libnl to correct this
currently and will cc you on the patch.
Neil

Re: [PATCH v3] drop_monitor: convert to modular building

[Eric Dumazet]

On Tue, 2012-05-29 at 15:33 -0400, Neil Horman wrote:

> Eric,
> 	Just FYI, I sent a series upstream to implement autoloading of generic
> netlink families.  Please be awarem, that I've tested these with a hacked
> version of dropwatch, and it works great, but with the normal version of
> dropwatch, the drop_monitor module still doesn't autoload.  This is due to libnl
> not explicitly requesting a family when genl_ctrl_family_resolve is called.
> Instead of trying to load the module, it dumps the existing registered families
> via a NLM_F_DUMP message.  I'm working on updating libnl to correct this
> currently and will cc you on the patch.

Excellent, thanks Neil