lkml.org 
[lkml]   [2010]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] Make the module 'usage' lists be two-way


On Tue, 1 Jun 2010, Américo Wang wrote:
> >+ list_for_each_entry_safe(use, tmp, &mod->target_list, target_list) {
> >+ struct module *i = use->target;
> >+ DEBUGP("%s unusing %s\n", mod->name, i->name);
> >+ module_put(i);
> >+ list_del(&use->source_list);
> >+ list_del(&use->target_list);
> >+ kfree(use);
> >+ sysfs_remove_link(i->holders_dir, mod->name);
>
> I think it's nice to have a remove_module_usage() here, since we already
> have add_module_usage().

I agree that those five lines could easily be a function of their own. It
would make sense and be symmetric. That said, I didn't do it because there
was just a single use place, and it was so simple.

Also, if you do turn it into a function, it's a bit dubious what the
proper calling convention would be. The clean interface would be to just
pass in "use", since you can figure out both the source and target from
there. At the same time, it's a bit sad to re-load "mod" from
"use->source", when we had it explicitly and started from it. But maybe
that doesn't really matter.

One thing I react to now that I look at it again - I think we should also
move the "module_put(i)" to the end. In fact I thought I did it when I
moved the code around, but clearly I hadn't.

It won't really matter as-is (we hold the module_mutex, so "i" isn't going
away), and it's what the old code used to do, but we should probably aim
for more of a ref-counted worldview where you don't put the module until
after it's not used again. And we clearly still use "i" after the
module_put() above in the sysfs_remove_link thing. So that's a bit ugly
too.

I suspect the module code could do with quite a bit of a cleanup. The
primary target of that patch was a different issue, though.

Linus
--
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: 2010-06-01 05:59    [W:0.412 / U:0.836 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site