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 - 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/
This archive was generated by hypermail 2b29 : Mon Jul 15 2002 - 22:00:21 EST