Re: [PATCH 1/2] Refine mutex and rcu method in module.c, kernel<3.2.9>

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Sat, Mar 10, 2012 at 12:26:30PM +0800, Cong Wang wrote:
> On 03/09/2012 12:13 AM, Chen, Dennis (SRDC SW) wrote:
> >Hi Rusty,
> >
> >Pls notice the following change in the patch (in set_all_modules_text_ro function):
> >
> >/* Iterate through all modules and set each module's text as RO */
> >@@ -1693,7 +1699,7 @@
> >  {
> >         struct module *mod;
> >
> >-       mutex_lock(&module_mutex);
> >+       rcu_read_lock();
> >         list_for_each_entry_rcu(mod,&modules, list) {
> >                 if ((mod->module_core)&&  (mod->core_text_size)) {
> >                         set_page_attributes(mod->module_core,
> >@@ -1706,7 +1712,7 @@
> >                                                 set_memory_ro);
> >                 }
> >         }
> >-       mutex_unlock(&module_mutex);
> >+       rcu_read_unlock();
> >  }
> >
> >This function just needs to iterate the modules list, but now it holds a unnecessary lock when it does that,
> >The other module can't be inserted during this operation, also can you make sure the set_page_attributes will
> >run smoothly all the time, if not it's a risk action to hold a lock.
> >So summary--
> >I think the idea for kernel module protection is simple:
> >Writers to modules, use mutex_lock
> >Readers, use rcu. __ALL__ codes here should be with a unified style! This will make our kernel gracefully.
> >
> >PS: my comments in the patch " /* Concurrent writers for the global modules list are protected by RCU*/" is not right, RCU
> >Should be mutex lock.
> 
> I think your change makes sense, I don't know why preempt_disable()
> was used, git blame told me the related two commits are 4
> years-old...
> 
> cb2a5205 2008-01-14 00:55:03 -0800 3180
> d72b3751 2008-08-30 10:09:00 +0200 3181
> 
> maybe at that time rcu was not what it is today... Cc'ing Paul.

I believe that these use either stop-machine or synchronize_sched()
for updates, so disabling preemption will work for readers.

							Thanx, Paul