lkml.org 
[lkml]   [2003]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: Patch/resubmit linux-2.5.63-bk4 try_module_get simplification
    Date
    On Sun, 2 Mar 2003 15:12:37 +0100 (CET)
    Roman Zippel <zippel@linux-m68k.org> wrote:

    > Hi,
    >
    > On Fri, 28 Feb 2003, Adam J. Richter wrote:
    >
    > > The following patch shrinks changes the implementation of
    > > try_module_get() and friends to eliminate the special stopping of all
    > > CPU's when a module is unloaded. Instead, it uses a read/write
    > > semaphore in a perhaps slightly non-intuitive way.
    >
    > Hmm, I was waiting a bit for Rusty's comment, but there isn't any...

    (Sorry, on holiday, skimming mail)

    I like it *very* much! There's a small race, but it's easily fixed by
    turning the local_dec into a module_put (or variant) (otherwise an
    rmmod --wait can sleep forever, waking up and seeing a non-zero
    refcount, and going back to sleep).

    Three other requests, if I may.

    It'd be nice to have a comment for the two smp_mb() eg. /* Must increment
    before checking state */ and vice-versa above the one in module.c. Secondly
    probably nicer to just rename the modlist_lock to module_lock and use that,
    and thirdly merge with the racefix patch below if Linus hasn't already
    taken it.

    BTW, I'm away from the office for a couple of weeks, and even if I weren't,
    my test box doesn't stay up for more than minutes under any recent kernel 8(
    Can you stress test this and send to Linus?

    > BTW making the module ref functions not inline saves about 5KB with the
    > standard config.

    Interesting. The "if (!module)" could theortically be dropped, but I
    just looked at the sizes of try_module_get as a standalone function
    for various config options (x86, gcc 3.2.3):

    UNLOAD=y
    SMP=y
    PREEMPT=y
    try_module_get() 78 bytes

    UNLOAD=y
    SMP=y
    PREEMPT=n
    try_module_get() 48 bytes

    UNLOAD=y
    SMP=n
    PREEMPT=n
    try_module_get() 28 bytes

    UNLOAD=n
    try_module_get() 21 bytes

    These numbers are a bit exaggerated due to function prologue and
    epilogues, but on x86 PREEMPT=y case at least I definitely think that
    it's worth out-of-lining it (a straight function call costs about 11
    bytes). Don't know about other archs.

    Thanks for the patch!
    Rusty.
    --
    there are those who do and those who hang on and you don't see too
    many doers quoting their contemporaries. -- Larry McVoy

    Name: Fix two module races
    Author: Bob Miller, Rusty Russell
    Status: Trivial

    D: Bob Miller points out that the try_module_get in use_module() can,
    D: of course, fail. Secondly, there is a race between setting the module
    D: live, and a simultaneous removal of it.

    diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.62-bk6/kernel/module.c working-2.5.62-bk6-modraces/kernel/module.c
    --- linux-2.5.62-bk6/kernel/module.c 2003-02-18 11:18:57.000000000 +1100
    +++ working-2.5.62-bk6-modraces/kernel/module.c 2003-02-24 13:42:44.000000000 +1100
    @@ -173,16 +173,19 @@ static int use_module(struct module *a,
    struct module_use *use;
    if (b == NULL || already_uses(a, b)) return 1;

    + if (!strong_try_module_get(b))
    + return 0;
    +
    DEBUGP("Allocating new usage for %s.\n", a->name);
    use = kmalloc(sizeof(*use), GFP_ATOMIC);
    if (!use) {
    printk("%s: out of memory loading\n", a->name);
    + module_put(b);
    return 0;
    }

    use->module_which_uses = a;
    list_add(&use->list, &b->modules_which_use_me);
    - try_module_get(b); /* Can't fail */
    return 1;
    }

    @@ -1456,10 +1459,12 @@ sys_init_module(void *umod,
    }

    /* Now it's a first class citizen! */
    + down(&module_mutex);
    mod->state = MODULE_STATE_LIVE;
    module_free(mod, mod->module_init);
    mod->module_init = NULL;
    mod->init_size = 0;
    + up(&module_mutex);

    return 0;
    }
    -
    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:33    [W:5.589 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site