Messages in this thread | | | From | Rusty Russell <> | Subject | Re: [RFC][PATCH] tracing/module: Move tracepoint out of module.h | Date | Thu, 29 Mar 2012 15:54:34 +1030 |
| |
On Thu, 29 Mar 2012 06:22:44 +0200, Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le mercredi 01 février 2012 à 17:18 +1030, Rusty Russell a écrit : > > > > > +void __module_get(struct module *module) > > +{ > > + if (module) { > > + preempt_disable(); > > + __this_cpu_inc(module->refptr->incs); > > + trace_module_get(module, _RET_IP_); > > + preempt_enable(); > > + } > > +} > > +EXPORT_SYMBOL(__module_get); > > + > > Hi Rusty > > I am wondering why preempt_disable()/preempt_enable() is needed in this > code. > > this_cpu_inc(module->refptr->incs) is preempt safe...
I agree, it's overkill here:
> @@ -907,10 +907,8 @@ static struct module_attribute modinfo_refcnt = > void __module_get(struct module *module) > { > if (module) { > - preempt_disable(); > - __this_cpu_inc(module->refptr->incs); > + this_cpu_inc(module->refptr->incs); > trace_module_get(module, _RET_IP_); > - preempt_enable(); > } > } > EXPORT_SYMBOL(__module_get);
But this one is required:
> @@ -920,15 +918,11 @@ bool try_module_get(struct module *module) > bool ret = true; > > if (module) { > - preempt_disable(); > - > if (likely(module_is_live(module))) { > - __this_cpu_inc(module->refptr->incs); > + this_cpu_inc(module->refptr->incs); > trace_module_get(module, _RET_IP_); > } else > ret = false; > - > - preempt_enable(); > } > return ret; > }
This is to protect against module removal's stop_machine.
> @@ -937,15 +931,13 @@ EXPORT_SYMBOL(try_module_get); > void module_put(struct module *module) > { > if (module) { > - preempt_disable(); > smp_wmb(); /* see comment in module_refcount */ > - __this_cpu_inc(module->refptr->decs); > + this_cpu_inc(module->refptr->decs); > > trace_module_put(module, _RET_IP_); > /* Maybe they're waiting for us to drop reference? */ > if (unlikely(!module_is_live(module))) > wake_up_process(module->waiter); > - preempt_enable(); > } > } > EXPORT_SYMBOL(module_put);
Without the preempt disable, module can vanish immediately after that increment.
Cheers, Rusty. -- How could I marry someone with more hair than me? http://baldalex.org -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |