Re: [V2 PATCH 1/3] module: use rcu to protect module list read

[Cong Wang]

On 03/12/2012 01:38 AM, Eric Dumazet wrote:
> Le dimanche 11 mars 2012 Ã 18:54 +0800, Cong Wang a Ãcrit :
> > V2:
> ...
> > @@ -1639,7 +1639,7 @@ static void mod_sysfs_teardown(struct module *mod)
> >   static int __unlink_module(void *_mod)
> >   {
> >   	struct module *mod = _mod;
> > -	list_del(&mod->list);
> > +	list_del_rcu(&mod->list);
> >   	module_bug_cleanup(mod);
> >   	return 0;
> >   }
> > @@
>
> You mix too many different things in a single patch.
>
> For example, lets review this __unlink_module() change...

Oops, sorry I missed this part...

> If this was really needed, it should be a single patch so that it can be
> a stable submission.
>
> As it is not needed (since its called from stop_machine()), this makes
> your whole patch looking suspicious.
>
> I suggest you make a 100% cleanup patch, changing the title as well,
> because module code _already_ uses RCU.
>
> "module: use rcu to protect module list read" makes no sense at all.
>
> To meet current RCU API best pratices, you change preempt_enable() by
> rcu_read_unlock_sched() and preempt_disable() by rcu_read_lock_sched()
>
> Then, if you believe other changes are needed, submit them with an
> explicit changelog explaining the change, not hiding them in a big
> cleanup patch.

Fair enough, Rusty gave me the same suggestion, I will split this patch.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/