Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3] timer: implement lockdep deadlock detection | From | Johannes Berg <> | Date | Wed, 28 Jan 2009 19:06:57 +0100 |
| |
On Wed, 2009-01-28 at 11:59 +0100, Johannes Berg wrote:
> > > I actually got "trying to register non-static key" on my powerpc64 > > > machine. Is there a possibility that functions are not static?? > > > > Hmm, weird, afaict static_obj() includes both text and data, for the > > core kernel as well as modules. > > Yeah, I'd think it should. I'll run it by the powerpc list when I get it > again, it only seems to happen very rarely.
It's actually a generic bug.
delayed work structs are initialised like this:
#define __DELAYED_WORK_INITIALIZER(n, f) { \ .work = __WORK_INITIALIZER((n).work, (f)), \ .timer = TIMER_INITIALIZER(NULL, 0, 0), \ }
#define DECLARE_DELAYED_WORK(n, f) \ struct delayed_work n = __DELAYED_WORK_INITIALIZER(n, f)
Note the NULL function, which I used for the key of timers. Thus, the key is NULL, and the name is "NULL".
Now, this means that my code for the run timer:
struct lockdep_map lockdep_map = timer->lockdep_map;
will actually have lockdep_map here with a NULL key. Once it gets into
lock_map_acquire(&lockdep_map);
it'll try to register &lockdep_map as the key.
Interestingly, that doesn't seem to be a problem on x86_64, which would appear to be a bug, the stack certainly isn't a static location.
The patch below fixes it by using the file/lineno of the static definition as both the name and the key -- using it as the name means you have a good chance of finding it if something goes wrong, and using it as the key means we have a good key for it. The patch looks horrible though. Any better ideas? If not, I think we should roll this into the original patch.
johannes --- include/linux/timer.h | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
--- wireless-testing.orig/include/linux/timer.h 2009-01-28 18:55:03.270071595 +0100 +++ wireless-testing/include/linux/timer.h 2009-01-28 19:00:41.326947619 +0100 @@ -32,23 +32,35 @@ extern struct tvec_base boot_tvec_bases; /* * NB: because we have to copy the lockdep_map, setting _key * here is required, otherwise it could get initialised to the - * copy of the lockdep_map! We use the function pointer as the - * lockdep key here. + * copy of the lockdep_map! We use the pointer to and the string + * "<file>:<line>" as the key resp. the name of the lockdep_map. */ -#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name) \ - .lockdep_map = STATIC_LOCKDEP_MAP_INIT(name, fn), +#define __TIMER_LOCKDEP_MAP_INITIALIZER(name, key) \ + .lockdep_map = STATIC_LOCKDEP_MAP_INIT(name, key), #else -#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name) +#define __TIMER_LOCKDEP_MAP_INITIALIZER(name, key) #endif -#define TIMER_INITIALIZER(_function, _expires, _data) { \ +#define ____TIMER_INITIALIZER(_function, _expires, _data, _name, _key) {\ .entry = { .prev = TIMER_ENTRY_STATIC }, \ .function = (_function), \ .expires = (_expires), \ .data = (_data), \ .base = &boot_tvec_bases, \ - __TIMER_LOCKDEP_MAP_INITIALIZER((_function), #_function)\ + __TIMER_LOCKDEP_MAP_INITIALIZER((_name), (_key)) \ } +/* + * All the indirection here is required to build the + * "<file>:<line>" string and the pointer to it. + */ +#define ___TIMER_INITIALIZER(_fn, _exp, _dat, _kn) \ + ____TIMER_INITIALIZER(_fn, _exp, _dat, (_kn), &(_kn)) +#define __TIMER_INITIALIZER(_fn, _exp, _dat, _f, _c, _l) \ + ___TIMER_INITIALIZER(_fn, _exp, _dat, _f # _c # _l) +#define _TIMER_INITIALIZER(_fn, _exp, _dat, _f, _l) \ + __TIMER_INITIALIZER(_fn, _exp, _dat, _f, :, _l) +#define TIMER_INITIALIZER(_function, _expires, _data) \ + _TIMER_INITIALIZER(_function, _expires, _data, __FILE__, __LINE__) #define DEFINE_TIMER(_name, _function, _expires, _data) \ struct timer_list _name = \
| |