Messages in this thread | | | Date | Mon, 31 May 2010 21:00:42 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: [PATCH 1/2] Make the module 'usage' lists be two-way |
| |
On Tue, 1 Jun 2010, Rusty Russell wrote: > > But this is ugly: > > > + use = kmalloc(sizeof(*use), GFP_ATOMIC); > > + if (!use) { > > + printk("%s: out of memory loading\n", a->name); > > + module_put(b); > > + return 0; > > The module_get is in the caller, but the module_put is here on failure. > Don't half split-out a function like this.
I agree. That happened as part of moving the code around mostly mechanically, but you're right, that fixup is better done in the caller that did the get.
Also, looking at it, I don't think that should be GFP_ATOMIC. I wonder why it is. I don't think we should have recursion issues with memory freeing needing new modules due to IO/filesystem accesses, but maybe there are cases like that. But again, that was just moving old code around.
And with the old "use_module()" having done a wait, we can't have had people calling this from atomic contexts. So I wonder where that GFP_ATOMIC comes from. It goes all the way back to the original in-kernel module loader code in 2002 according to git.
Oh. And back then, it was inside a "modlist_lock". And that lock is long gone, but the GFP_ATOMIC remains.
Of course, it's a small data structure, and there aren't many of them, so nobody would ever notice. It's just an oddity right now.
Anyway, modified patch looks fine to me.
Linus
| |