Re: [PATCH 1/2] Make the module 'usage' lists be two-way

[Linus Torvalds <torvalds@linux-foundation.org>]

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