Messages in this thread | | | From | Nick Piggin <> | Subject | Re: [PATCH 1/5] lib: introduce call_once() | Date | Tue, 11 Mar 2008 23:41:37 +1100 |
| |
On Tuesday 11 March 2008 01:57, Akinobu Mita wrote:
> +static inline int call_once(struct once_control *once_control, > + int (*init_rouine)(void)) > +{ > + return likely(once_control->done) ? 0 > + : call_once_slow(once_control, init_rouine); > +} > + > +#endif /* __LINUX_ONCE_H */ > Index: 2.6-rc/lib/once.c > =================================================================== > --- /dev/null > +++ 2.6-rc/lib/once.c > @@ -0,0 +1,18 @@ > +#include <linux/module.h> > +#include <linux/once.h> > + > +int call_once_slow(struct once_control *once_control, int > (*init_rouine)(void)) +{ > + int err = 0; > + > + mutex_lock(&once_control->lock); > + if (!once_control->done) { > + err = init_rouine(); > + if (!err) > + once_control->done = 1; > + } > + mutex_unlock(&once_control->lock); > + > + return err; > +} > +EXPORT_SYMBOL_GPL(call_once_slow);
The store "once_control->done = 1" can become visible before init_routine has finished. The code after calling call_once may also speculatively load some memory before the load of once_control->done completes, so you can likewise have a data race that way too.
To fix this, you need smp_wmb after init_rouine(), and probably smp_mb() in the fastpath after the check but before returning.
Basically any time you have this situation where you're touching a shared variable without using locks, then you're vastly increasing the complexity of the code, and so you must have a good reason for it.
So acquiring the mutex unconditionally would be the best way to go, unless you're calling this a lot in fastpaths (in which case I would say you should probably rework your code)
Thanks, Nick
| |