lkml.org 
[lkml]   [2010]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] module: fix bne2 "gave up waiting for init of module libcrc32c"


On Mon, 31 May 2010, Andrew Morton wrote:
>
> Who's returning -EBUSY? request_module()? If so, are you requiring
> that all code which might call request_module() be correctly
> propagating error codes back? Please spell this all out?

The problem roughly as follows:

task 1 task 2
------ ------

request_module("crc32c")
gets module_mutex
...
drops module_mutex in otder to run "init"
request_module("bne2");
gets module_mutex
wants to link to crc32:
use_module("bne2", "crc32c")
..
strong_try_module_get() returns -EBUSY
because it's not initialized yet
calls libcrc32c_mod_init
...
request_module(optimized crc32c)
waits for module_mutex
that is held by the bne2 loading

.. gives up .. BOOM ..
releases module_mutex
returns error
finishes successfully


because the module locking is pure and utter crap. It uses one hug lock
that it tries to hold for a long time, rather than protecting just the
parts it needs.

Rusty's fix is to just drop the lock around use_module(), and it seems to
work. It's may be right for 'use_module()', but totally wrong from a
conceptual locking standpoint, though - dropping the lock in the middle of
module loading may well "work", but who the hell knows what it really
results in?

IOW, it's one of those "this works, but it's very wrong" things. It makes
the whole module_mutex pretty much a random thing with even less semantics
than it has now. Right now it has some clear area that it protects - the
area may be too _big_, but at least it makes some amount of sense.

The proper fix would appear to be to actually fix locking, which probably
implies turning most of "module_mutex" into a spinlock that protects just
the _real_ critical sections. I don't think there are any real blocking
things except for that "wait for another module to load" case, which is
obviously exactly where we cannot hold the lock in the first place.

So rather than having one large area that gets protected but then dropping
the lock in random places, we should probably just have lots of small
areas that are clearly defined and protected.

And a _lot_ of the module loading doesn't need any locks at all. Much of
the real work is probably totally private, ie loading the actual module
and setting things up before we really expose it. We hold that big lock
over a ridiculously large area right now (basically _all_ of module
loading except the actual init sequence for a module).

> Also, I bet there are drivers which return -EBUSY from their
> module_init() functions if the hardware's in an unexpected state. What
> happens?

Nothing. See above: this is a special case, and it's really just about
strong_try_module_get() returning EBUSY for one special reason.

It's entirely possible that an interim fix (if we can't just fix the
locking) is to _not_ use "strong_try_module_get()" at all, but instead
just use "try_module_get()", and then after we've dropped the
module_mutex, but _before_ we call the "init" function for the module, we
wait for all the modules that this module depends on.

IOW, we'd link to other modules _before_ they are necessarily initialized
(their symbol tables will be as initialized as they are going to be), but
then before we call our own initialization routines we make sure that the
modules we linked to have finished theirs.

Doesn't that sound like the logical thing to do? And it wouldn't change
any locking.

Linus


\
 
 \ /
  Last update: 2010-05-31 20:27    [W:0.223 / U:1.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site