Re: Rusty's module talk at the Kernel Summit

[Daniel Phillips <phillips@arcor.de>]

On Friday 12 July 2002 01:37, Alexander Viro wrote:
> As for determining the loading/normal/unloading - we _already_ have that
> state, no need to introduce new fields.  How do you think try_inc_mod_count()
> manages to work?  Exactly - there's a field of struct module that contains
> a bunch of flags.  And no, Daniel's ramblings (from what I've seen quoted)
> are pure BS - there's no need to mess with "oh, but I refuse to be
> unregistered"; proper refcounting is easy for normal cases.

I don't particularly like using the mod count to hold a module in memory.
It's workable but sloppy.  Supposing that the mod count counts the number
of filesystems mounted (it doesn't, it counts the number of mounts, an
even sillier thing to count), and supposing all are unmounted but the
module can't unregister itself for some other reason, say some thread it
owns hasn't exited yet.  Yes, you could say the mod count is the count of
all mounts, plus all the threads the module owns, plus more counts for
other resources the module owns, but why?  Just let the unregister routine
return failure, it's more general and a simpler interface.  Besides,

> It's not needed.  I don't see where this ret-rmmod crap is coming from -
> module uses some interface and decisions about holding it pinned belong
> to that interface.

The ret-rmmod race is what you get when you rely on something in the
module dec'ing the use count, and somebody can come along later to throw
the module out of memory - stepping on still-executing ret code.  This
race isn't obviously gone.

Speaking of crap, this is nothing to be proud of:

637                 spin_lock(&unload_lock);
638                 if (mod->refs == NULL
639                     && (mod->flags & MOD_AUTOCLEAN)
640                     && (mod->flags & MOD_RUNNING)
641                     && !(mod->flags & MOD_DELETED)
642                     && (mod->flags & MOD_USED_ONCE)
643                     && !__MOD_IN_USE(mod)) {
644                         if ((mod->flags & MOD_VISITED)
645                             && !(mod->flags & MOD_JUST_FREED)) {
646                                 spin_unlock(&unload_lock);
647                                 mod->flags &= ~MOD_VISITED;
648                         } else {
649                                 mod->flags |= MOD_DELETED;
650                                 spin_unlock(&unload_lock);
651                                 free_module(mod, 1);
652                                 something_changed = 1;
653                         }
654                 } else {
655                         spin_unlock(&unload_lock);
656                 }

I'm not going to be very easily convinced that the result of this
current effort is going to be the most elegant possible.  Yes, I expect
it to work eventually, but as an shining example of transparent code...
it just isn't.

The rest of the interface seems to run about the same level of
cleanliness.  I suppose I shouldn't be so quick to put away my
dung-shovel.

> Plain, simple and works for all normal drivers.

That we agree on.

-- 
Daniel