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

[Cong Wang <xiyou.wangcong@gmail.com>]

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!