lkml.org 
[lkml]   [2002]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    In message <E181zuY-0004Fl-00@starship> you write:
    > On Thursday 17 October 2002 00:48, Rusty Russell wrote:
    > > > On Wednesday 16 October 2002 08:11, Rusty Russell wrote:
    > > > > It needs to be turned off when dealing with any interface which might
    > > > > be used by one of the hard modules. Which is pretty bad.
    > > >
    > > > As far as I can see, preemption only has to be disabled during the
    > > > synchronize_kernel phase of unloading that one module, and this requireme
    nt
    > > > is inherited neither by dependant or depending modules.
    > >
    > > No, someone could already have been preempted before you start
    > > synchronize_kernel().
    >
    > I don't get that. The sequence is:
    >
    > - turn off preemption
    > - unhook call points
    > - synchronize_kernel
    > - ...
    >
    > which doesn't leave any preemption hole that I can see, so I can't comment
    > on a couple of the other points until you clear that one up.

    You mean that "turn off preemption" also wakes up anyone currently
    preempted? Otherwise they're preempted just inside one of those call
    points.

    > > Still a race between the zero check and the can't-increment state
    > > setting.
    >
    > But that one is easy: the zero check just takes the same spinlock as
    > TRY_INC_MOD_COUNT, then sets can't-increment only in the case the count
    > is zero, considerably simpler than:

    The current spinlock is horrible. You could use a brlock, of course,
    but I didn't mainly because of code bloat and speed. My current code
    looks like:

    static inline int try_module_get(struct module *module)
    {
    int ret = 1;

    if (module) {
    unsigned int cpu = get_cpu();
    if (likely(module->ref[cpu].live))
    local_inc(&module->ref[cpu].counter);
    else
    ret = 0;
    put_cpu();
    }
    return ret;
    }

    Which is small enough to be inlined quite nicely, and very fast.
    Adding br_read_lock_irqsave() starts to get big and slow (at that
    point it's more likely we want to move the module case out of line).

    > > This is what my current code does: rmmod itself checks (if
    > > /proc/modules available), then the kernel sets the module to
    > > can't-increment, then checks again. If the non-blocking flag is set,
    > > it then re-animates the module and fails, otherwise it waits.
    >
    > and leaves no window for spurious failure. The still-initializing case is
    > also easy, e.g., a filesystem module simply doesn't call register_filesystem
    > until it's completely ready to service calls, so nobody is able to do
    > TRY_INC_MOD_COUNT.

    Consider some code which needs to know when cpus go up and down, so
    registers a notifier. If the notifier fires before the init is
    finished, the notifier code will fail to "try_inc_mod_count()" and
    won't call it (it doesn't do try_inc_mod_count at the moment, but
    that's a bug).

    I don't know of any code which does this now, but it is at least a
    theoretical problem.

    > > BTW, current patchset (2.5.43):
    >
    > Thanks, I'll read them all on the 21st ;-) The other thing I need to read
    > closely is Roman's strategy for changing the module format, and the weird
    > linker connections.

    Roman dislikes linking in the kernel. So did I until I wrote it: it's
    really trivial (esp. compared with the code to coordinate with the
    userspace linker properly). And it exists today. The linking takes
    around 200 lines. But, let's say his solution is 500 lines shorter
    than mine.

    For those five hundred lines, the new parameter infrastructure and
    module versioning changes can be done *without* requiring any changes
    in modutils. If you've been following the module changes closely in
    the last couple of years, you'll realize what a pain it has been to
    introduce changes like licensing, etc. This frees up our hand.

    IMHO, the benifits of having it in-kernel outweigh the slight extra
    size.

    > > ...The second is the "die-mother-fucker-die"
    > > version, which taints the kernel and just removes the damn thing. For
    > > most people, this is better than a reboot, and will usually "work".
    >
    > Is there a case where removing a module would actually help? What is
    > the user going to do next, try to reinsert the same module?

    Insert a fixed one, hopefully 8). I was thinking for kernel
    developers, and general robustness (eg. an oops inside a module leaves
    its refcount at 1).

    > > http://www.kernel.org/pub/linux/kernel/people/rusty/patches/Module/force-un
    load.patch.gz
    >
    > ERROR 404: Not Found.

    Damn my fingers. Updated (now applies on top of the others) but I
    haven't tested this version yet (that's what I'm doing now):

    http://www.kernel.org/pub/linux/kernel/people/rusty/patches/Module/forceunload.patch.gz

    Cheers!
    Rusty.
    --
    Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
    -
    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/

    \
     
     \ /
      Last update: 2005-03-22 13:30    [W:0.047 / U:31.508 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site