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

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

[Chen, Dennis (SRDC SW)]

1. Narrow down the granularity of mutex_lock/ mutex_unlock
2. Replace some unnecessary mutex_lock/mutex_unlock pairs with RCU
3. Refine the consistent calling style of RCU functioan 

--- module.ori.c        2012-03-07 19:59:07.890900147 +0800
+++ module.c    2012-03-07 22:12:44.982738729 +0800
@@ -419,11 +419,14 @@
 struct module *find_module(const char *name)
 {
        struct module *mod;
-
-       list_for_each_entry(mod, &modules, list) {
-               if (strcmp(mod->name, name) == 0)
+       rcu_read_lock(); 
+       list_for_each_entry_rcu(mod, &modules, list) {
+               if (strcmp(mod->name, name) == 0) {
+                       rcu_read_unlock();
                        return mod;
+                }
        }
+       rcu_read_unlock();
        return NULL;
 }
 EXPORT_SYMBOL_GPL(find_module);
@@ -488,7 +491,7 @@
        struct module *mod;
        unsigned int cpu;
 
-       preempt_disable();
+       rcu_read_lock();
 
        list_for_each_entry_rcu(mod, &modules, list) {
                if (!mod->percpu_size)
@@ -498,13 +501,13 @@
 
                        if ((void *)addr >= start &&
                            (void *)addr < start + mod->percpu_size) {
-                               preempt_enable();
+                               rcu_read_unlock();
                                return true;
                        }
                }
        }
 
-       preempt_enable();
+       rcu_read_unlock();
        return false;
 }
 
@@ -876,11 +879,11 @@
 {
        struct module *owner;
 
-       preempt_disable();
+       rcu_read_lock();
        if (!find_symbol(symbol, &owner, NULL, true, false))
                BUG();
        module_put(owner);
-       preempt_enable();
+       rcu_read_unlock();
 }
 EXPORT_SYMBOL(__symbol_put);
 
@@ -1598,8 +1601,11 @@
 static int __unlink_module(void *_mod)
 {
        struct module *mod = _mod;
-       list_del(&mod->list);
-       module_bug_cleanup(mod);
+       mutex_lock(&module_mutex);
+       list_del_rcu(&mod->list);
+       module_bug_cleanup(mod);
+       mutex_unlock(&module_mutex);
+
        return 0;
 }
 
@@ -1672,7 +1678,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,
@@ -1685,7 +1691,7 @@
                                                set_memory_rw);
                }
        }
-       mutex_unlock(&module_mutex);
+       rcu_read_unlock();
 }
 
 /* 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();
 }
 #else
 static inline void set_section_ro_nx(void *base, unsigned long text_size, unsigned long ro_size, unsigned long total_size) { }
@@ -1729,9 +1735,7 @@
        trace_module_free(mod);
 
        /* Delete from various lists */
-       mutex_lock(&module_mutex);
        stop_machine(__unlink_module, mod, NULL);
-       mutex_unlock(&module_mutex);
        mod_sysfs_teardown(mod);
 
        /* Remove dynamic debug info */
@@ -1769,11 +1773,11 @@
        struct module *owner;
        const struct kernel_symbol *sym;
 
-       preempt_disable();
+       rcu_read_lock();
        sym = find_symbol(symbol, &owner, NULL, true, true);
        if (sym && strong_try_module_get(owner))
                sym = NULL;
-       preempt_enable();
+       rcu_read_unlock();
 
        return sym ? (void *)sym->value : NULL;
 }
@@ -2866,20 +2870,13 @@
 
        /* Mark state as coming so strong_try_module_get() ignores us. */
        mod->state = MODULE_STATE_COMING;
-
-       /* Now sew it into the lists so we can get lockdep and oops
-        * info during argument parsing.  No one should access us, since
-        * strong_try_module_get() will fail.
-        * lockdep/oops can run asynchronous, so use the RCU list insertion
-        * function to insert in a way safe to concurrent readers.
-        * The mutex protects against concurrent writers.
-        */
-       mutex_lock(&module_mutex);
+        
+        /* Concurrent writers for the global modules list are protected by RCU*/
        if (find_module(mod->name)) {
                err = -EEXIST;
                goto unlock;
        }
-
+        
        /* This has to be done once we're sure module name is unique. */
        dynamic_debug_setup(info.debug, info.num_debug);
 
@@ -2889,7 +2886,9 @@
                goto ddebug;
 
        module_bug_finalize(info.hdr, info.sechdrs, mod);
-       list_add_rcu(&mod->list, &modules);
+
+       mutex_lock(&module_mutex);
+       list_add_rcu(&mod->list, &modules);
        mutex_unlock(&module_mutex);
 
        /* Module is ready to execute: parsing args may do that. */
@@ -2915,11 +2914,10 @@
        /* Unlink carefully: kallsyms could be walking list. */
        list_del_rcu(&mod->list);
        module_bug_cleanup(mod);
-
+       mutex_unlock(&module_mutex);
  ddebug:
        dynamic_debug_remove(info.debug);
  unlock:
-       mutex_unlock(&module_mutex);
        synchronize_sched();
        kfree(mod->args);
  free_arch_cleanup:
@@ -3102,7 +3100,7 @@
        struct module *mod;
        const char *ret = NULL;
 
-       preempt_disable();
+       rcu_read_lock();
        list_for_each_entry_rcu(mod, &modules, list) {
                if (within_module_init(addr, mod) ||
                    within_module_core(addr, mod)) {
@@ -3117,7 +3115,7 @@
                strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
                ret = namebuf;
        }
-       preempt_enable();
+       rcu_read_unlock();
        return ret;
 }
 
@@ -3125,7 +3123,7 @@
 {
        struct module *mod;
 
-       preempt_disable();
+       rcu_read_lock();
        list_for_each_entry_rcu(mod, &modules, list) {
                if (within_module_init(addr, mod) ||
                    within_module_core(addr, mod)) {
@@ -3135,12 +3133,12 @@
                        if (!sym)
                                goto out;
                        strlcpy(symname, sym, KSYM_NAME_LEN);
-                       preempt_enable();
+                       rcu_read_unlock();
                        return 0;
                }
        }
 out:
-       preempt_enable();
+       rcu_read_unlock();
        return -ERANGE;
 }
 
@@ -3149,7 +3147,7 @@
 {
        struct module *mod;
 
-       preempt_disable();
+       rcu_read_lock();
        list_for_each_entry_rcu(mod, &modules, list) {
                if (within_module_init(addr, mod) ||
                    within_module_core(addr, mod)) {
@@ -3162,12 +3160,12 @@
                                strlcpy(modname, mod->name, MODULE_NAME_LEN);
                        if (name)
                                strlcpy(name, sym, KSYM_NAME_LEN);
-                       preempt_enable();
+                       rcu_read_unlock();
                        return 0;
                }
        }
 out:
-       preempt_enable();
+       rcu_read_unlock();
        return -ERANGE;
 }
 
@@ -3176,7 +3174,7 @@
 {
        struct module *mod;
 
-       preempt_disable();
+       rcu_read_lock();
        list_for_each_entry_rcu(mod, &modules, list) {
                if (symnum < mod->num_symtab) {
                        *value = mod->symtab[symnum].st_value;
@@ -3185,12 +3183,12 @@
                                KSYM_NAME_LEN);
                        strlcpy(module_name, mod->name, MODULE_NAME_LEN);
                        *exported = is_exported(name, *value, mod);
-                       preempt_enable();
+                       rcu_read_unlock();
                        return 0;
                }
                symnum -= mod->num_symtab;
        }
-       preempt_enable();
+       rcu_read_unlock();
        return -ERANGE;
 }
 
@@ -3212,19 +3210,18 @@
        char *colon;
        unsigned long ret = 0;
 
-       /* Don't lock: we're in enough trouble already. */
-       preempt_disable();
        if ((colon = strchr(name, ':')) != NULL) {
                *colon = '\0';
                if ((mod = find_module(name)) != NULL)
                        ret = mod_find_symname(mod, colon+1);
                *colon = ':';
        } else {
+               rcu_read_lock();
                list_for_each_entry_rcu(mod, &modules, list)
                        if ((ret = mod_find_symname(mod, name)) != 0)
                                break;
+               rcu_read_unlock();
        }
-       preempt_enable();
        return ret;
 }
 
@@ -3236,14 +3233,18 @@
        unsigned int i;
        int ret;
 
-       list_for_each_entry(mod, &modules, list) {
+       rcu_read_lock();
+       list_for_each_entry_rcu(mod, &modules, list) {
                for (i = 0; i < mod->num_symtab; i++) {
                        ret = fn(data, mod->strtab + mod->symtab[i].st_name,
                                 mod, mod->symtab[i].st_value);
-                       if (ret != 0)
+                       if (ret != 0) {
+                               rcu_read_unlock();
                                return ret;
+                        }
                }
        }
+       rcu_read_unlock();
        return 0;
 }
 #endif /* CONFIG_KALLSYMS */
@@ -3364,7 +3365,7 @@
        const struct exception_table_entry *e = NULL;
        struct module *mod;
 
-       preempt_disable();
+       rcu_read_lock();
        list_for_each_entry_rcu(mod, &modules, list) {
                if (mod->num_exentries == 0)
                        continue;
@@ -3375,7 +3376,7 @@
                if (e)
                        break;
        }
-       preempt_enable();
+       rcu_read_unlock();
 
        /* Now, if we found one, we are running inside it now, hence
           we cannot unload the module, hence no refcnt needed. */
@@ -3393,9 +3394,9 @@
 {
        bool ret;
 
-       preempt_disable();
+       rcu_read_lock();
        ret = __module_address(addr) != NULL;
-       preempt_enable();
+       rcu_read_unlock();
 
        return ret;
 }
@@ -3434,9 +3435,9 @@
 {
        bool ret;
 
-       preempt_disable();
+       rcu_read_lock();
        ret = __module_text_address(addr) != NULL;
-       preempt_enable();
+       rcu_read_unlock();
 
        return ret;
 }
@@ -3469,10 +3470,10 @@
 
        printk(KERN_DEFAULT "Modules linked in:");
        /* Most callers should already have preempt disabled, but make sure */
-       preempt_disable();
+       rcu_read_lock();
        list_for_each_entry_rcu(mod, &modules, list)
                printk(" %s%s", mod->name, module_flags(mod, buf));
-       preempt_enable();
+       rcu_read_unlock();
        if (last_unloaded_module[0])
                printk(" [last unloaded: %s]", last_unloaded_module);
        printk("\n");

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

[Chen, Dennis (SRDC SW)]

1. Add protection code for module_bug_list readers
2. Add a new module_bug_mutex as the mutex of the module_bug_list writers

--- bug.ori.c   2012-03-07 20:42:56.454850872 +0800
+++ bug.c       2012-03-07 20:58:48.562825887 +0800
@@ -42,6 +42,7 @@
 #include <linux/kernel.h>
 #include <linux/bug.h>
 #include <linux/sched.h>
+#include <linux/rcupdate.h>
 
 extern const struct bug_entry __start___bug_table[], __stop___bug_table[];
 
@@ -56,19 +57,24 @@
 
 #ifdef CONFIG_MODULES
 static LIST_HEAD(module_bug_list);
+static DEFINE_MUTEX(module_bug_mutex);
 
 static const struct bug_entry *module_find_bug(unsigned long bugaddr)
 {
        struct module *mod;
 
-       list_for_each_entry(mod, &module_bug_list, bug_list) {
+       rcu_read_lock();
+       list_for_each_entry_rcu(mod, &module_bug_list, bug_list) {
                const struct bug_entry *bug = mod->bug_table;
                unsigned i;
 
                for (i = 0; i < mod->num_bugs; ++i, ++bug)
-                       if (bugaddr == bug_addr(bug))
+                       if (bugaddr == bug_addr(bug)){
+                               rcu_read_unlock();
                                return bug;
+                        }
        }
+       rcu_read_unlock();
        return NULL;
 }
 
@@ -96,7 +102,9 @@
         * traversals, but since we only traverse on BUG()s, a spinlock
         * could potentially lead to deadlock and thus be counter-productive.
         */
-       list_add(&mod->bug_list, &module_bug_list);
+       mutex_lock(&module_bug_mutex);
+       list_add_rcu(&mod->bug_list, &module_bug_list);
+       mutex_unlock(&module_bug_mutex);
 }
 
 void module_bug_cleanup(struct module *mod)

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

[Cong Wang]

Hi,

On 03/07/2012 10:51 PM, Chen, Dennis (SRDC SW) wrote:

Before we are going into tech details...

> 1. Narrow down the granularity of mutex_lock/ mutex_unlock
> 2. Replace some unnecessary mutex_lock/mutex_unlock pairs with RCU
> 3. Refine the consistent calling style of RCU functioan

Please split the patch into three patches.

>
> --- module.ori.c        2012-03-07 19:59:07.890900147 +0800
> +++ module.c    2012-03-07 22:12:44.982738729 +0800

The patch is not correctly generated, check 
Documentation/SubmittingPatches for more info.

Thanks,

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

[Chen, Dennis (SRDC SW)]

-----Original Message-----
From: Cong Wang [mailto:xiyou.wangcong@gmail.com] 
Sent: Wednesday, March 07, 2012 11:37 PM
To: Chen, Dennis (SRDC SW)
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] Refine mutex and rcu method in module.c, kernel<3.2.9>

Hi,

On 03/07/2012 10:51 PM, Chen, Dennis (SRDC SW) wrote:

Before we are going into tech details...

> 1. Narrow down the granularity of mutex_lock/ mutex_unlock
> 2. Replace some unnecessary mutex_lock/mutex_unlock pairs with RCU
> 3. Refine the consistent calling style of RCU functioan

Please split the patch into three patches.

>
> --- module.ori.c        2012-03-07 19:59:07.890900147 +0800
> +++ module.c    2012-03-07 22:12:44.982738729 +0800

The patch is not correctly generated, check 
Documentation/SubmittingPatches for more info.

====================================================

diff -u module.ori.c module.c > module.patch

give more details please?

Thanks,

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

[Chen, Dennis (SRDC SW)]

Adding kernel module maintainer -- Rusty Russell

-----Original Message-----
From: Chen, Dennis (SRDC SW) 
Sent: Wednesday, March 07, 2012 10:51 PM
To: linux-kernel@vger.kernel.org
Cc: Chen, Dennis (SRDC SW)
Subject: [PATCH 1/2] Refine mutex and rcu method in module.c, kernel<3.2.9>

1. Narrow down the granularity of mutex_lock/ mutex_unlock
2. Replace some unnecessary mutex_lock/mutex_unlock pairs with RCU
3. Refine the consistent calling style of RCU functioan 

--- kernel/module.ori.c        2012-03-07 19:59:07.890900147 +0800
+++ kernel/module.c    2012-03-07 22:12:44.982738729 +0800
@@ -419,11 +419,14 @@
 struct module *find_module(const char *name)
 {
        struct module *mod;
-
-       list_for_each_entry(mod, &modules, list) {
-               if (strcmp(mod->name, name) == 0)
+       rcu_read_lock(); 
+       list_for_each_entry_rcu(mod, &modules, list) {
+               if (strcmp(mod->name, name) == 0) {
+                       rcu_read_unlock();
                        return mod;
+                }
        }
+       rcu_read_unlock();
        return NULL;
 }
 EXPORT_SYMBOL_GPL(find_module);
@@ -488,7 +491,7 @@
        struct module *mod;
        unsigned int cpu;
 
-       preempt_disable();
+       rcu_read_lock();
 
        list_for_each_entry_rcu(mod, &modules, list) {
                if (!mod->percpu_size)
@@ -498,13 +501,13 @@
 
                        if ((void *)addr >= start &&
                            (void *)addr < start + mod->percpu_size) {
-                               preempt_enable();
+                               rcu_read_unlock();
                                return true;
                        }
                }
        }
 
-       preempt_enable();
+       rcu_read_unlock();
        return false;
 }
 
@@ -876,11 +879,11 @@
 {
        struct module *owner;
 
-       preempt_disable();
+       rcu_read_lock();
        if (!find_symbol(symbol, &owner, NULL, true, false))
                BUG();
        module_put(owner);
-       preempt_enable();
+       rcu_read_unlock();
 }
 EXPORT_SYMBOL(__symbol_put);
 
@@ -1598,8 +1601,11 @@
 static int __unlink_module(void *_mod)
 {
        struct module *mod = _mod;
-       list_del(&mod->list);
-       module_bug_cleanup(mod);
+       mutex_lock(&module_mutex);
+       list_del_rcu(&mod->list);
+       module_bug_cleanup(mod);
+       mutex_unlock(&module_mutex);
+
        return 0;
 }
 
@@ -1672,7 +1678,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,
@@ -1685,7 +1691,7 @@
                                                set_memory_rw);
                }
        }
-       mutex_unlock(&module_mutex);
+       rcu_read_unlock();
 }
 
 /* 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();
 }
 #else
 static inline void set_section_ro_nx(void *base, unsigned long text_size, unsigned long ro_size, unsigned long total_size) { }
@@ -1729,9 +1735,7 @@
        trace_module_free(mod);
 
        /* Delete from various lists */
-       mutex_lock(&module_mutex);
        stop_machine(__unlink_module, mod, NULL);
-       mutex_unlock(&module_mutex);
        mod_sysfs_teardown(mod);
 
        /* Remove dynamic debug info */
@@ -1769,11 +1773,11 @@
        struct module *owner;
        const struct kernel_symbol *sym;
 
-       preempt_disable();
+       rcu_read_lock();
        sym = find_symbol(symbol, &owner, NULL, true, true);
        if (sym && strong_try_module_get(owner))
                sym = NULL;
-       preempt_enable();
+       rcu_read_unlock();
 
        return sym ? (void *)sym->value : NULL;
 }
@@ -2866,20 +2870,13 @@
 
        /* Mark state as coming so strong_try_module_get() ignores us. */
        mod->state = MODULE_STATE_COMING;
-
-       /* Now sew it into the lists so we can get lockdep and oops
-        * info during argument parsing.  No one should access us, since
-        * strong_try_module_get() will fail.
-        * lockdep/oops can run asynchronous, so use the RCU list insertion
-        * function to insert in a way safe to concurrent readers.
-        * The mutex protects against concurrent writers.
-        */
-       mutex_lock(&module_mutex);
+        
+        /* Concurrent writers for the global modules list are protected by RCU*/
        if (find_module(mod->name)) {
                err = -EEXIST;
                goto unlock;
        }
-
+        
        /* This has to be done once we're sure module name is unique. */
        dynamic_debug_setup(info.debug, info.num_debug);
 
@@ -2889,7 +2886,9 @@
                goto ddebug;
 
        module_bug_finalize(info.hdr, info.sechdrs, mod);
-       list_add_rcu(&mod->list, &modules);
+
+       mutex_lock(&module_mutex);
+       list_add_rcu(&mod->list, &modules);
        mutex_unlock(&module_mutex);
 
        /* Module is ready to execute: parsing args may do that. */
@@ -2915,11 +2914,10 @@
        /* Unlink carefully: kallsyms could be walking list. */
        list_del_rcu(&mod->list);
        module_bug_cleanup(mod);
-
+       mutex_unlock(&module_mutex);
  ddebug:
        dynamic_debug_remove(info.debug);
  unlock:
-       mutex_unlock(&module_mutex);
        synchronize_sched();
        kfree(mod->args);
  free_arch_cleanup:
@@ -3102,7 +3100,7 @@
        struct module *mod;
        const char *ret = NULL;
 
-       preempt_disable();
+       rcu_read_lock();
        list_for_each_entry_rcu(mod, &modules, list) {
                if (within_module_init(addr, mod) ||
                    within_module_core(addr, mod)) {
@@ -3117,7 +3115,7 @@
                strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
                ret = namebuf;
        }
-       preempt_enable();
+       rcu_read_unlock();
        return ret;
 }
 
@@ -3125,7 +3123,7 @@
 {
        struct module *mod;
 
-       preempt_disable();
+       rcu_read_lock();
        list_for_each_entry_rcu(mod, &modules, list) {
                if (within_module_init(addr, mod) ||
                    within_module_core(addr, mod)) {
@@ -3135,12 +3133,12 @@
                        if (!sym)
                                goto out;
                        strlcpy(symname, sym, KSYM_NAME_LEN);
-                       preempt_enable();
+                       rcu_read_unlock();
                        return 0;
                }
        }
 out:
-       preempt_enable();
+       rcu_read_unlock();
        return -ERANGE;
 }
 
@@ -3149,7 +3147,7 @@
 {
        struct module *mod;
 
-       preempt_disable();
+       rcu_read_lock();
        list_for_each_entry_rcu(mod, &modules, list) {
                if (within_module_init(addr, mod) ||
                    within_module_core(addr, mod)) {
@@ -3162,12 +3160,12 @@
                                strlcpy(modname, mod->name, MODULE_NAME_LEN);
                        if (name)
                                strlcpy(name, sym, KSYM_NAME_LEN);
-                       preempt_enable();
+                       rcu_read_unlock();
                        return 0;
                }
        }
 out:
-       preempt_enable();
+       rcu_read_unlock();
        return -ERANGE;
 }
 
@@ -3176,7 +3174,7 @@
 {
        struct module *mod;
 
-       preempt_disable();
+       rcu_read_lock();
        list_for_each_entry_rcu(mod, &modules, list) {
                if (symnum < mod->num_symtab) {
                        *value = mod->symtab[symnum].st_value;
@@ -3185,12 +3183,12 @@
                                KSYM_NAME_LEN);
                        strlcpy(module_name, mod->name, MODULE_NAME_LEN);
                        *exported = is_exported(name, *value, mod);
-                       preempt_enable();
+                       rcu_read_unlock();
                        return 0;
                }
                symnum -= mod->num_symtab;
        }
-       preempt_enable();
+       rcu_read_unlock();
        return -ERANGE;
 }
 
@@ -3212,19 +3210,18 @@
        char *colon;
        unsigned long ret = 0;
 
-       /* Don't lock: we're in enough trouble already. */
-       preempt_disable();
        if ((colon = strchr(name, ':')) != NULL) {
                *colon = '\0';
                if ((mod = find_module(name)) != NULL)
                        ret = mod_find_symname(mod, colon+1);
                *colon = ':';
        } else {
+               rcu_read_lock();
                list_for_each_entry_rcu(mod, &modules, list)
                        if ((ret = mod_find_symname(mod, name)) != 0)
                                break;
+               rcu_read_unlock();
        }
-       preempt_enable();
        return ret;
 }
 
@@ -3236,14 +3233,18 @@
        unsigned int i;
        int ret;
 
-       list_for_each_entry(mod, &modules, list) {
+       rcu_read_lock();
+       list_for_each_entry_rcu(mod, &modules, list) {
                for (i = 0; i < mod->num_symtab; i++) {
                        ret = fn(data, mod->strtab + mod->symtab[i].st_name,
                                 mod, mod->symtab[i].st_value);
-                       if (ret != 0)
+                       if (ret != 0) {
+                               rcu_read_unlock();
                                return ret;
+                        }
                }
        }
+       rcu_read_unlock();
        return 0;
 }
 #endif /* CONFIG_KALLSYMS */
@@ -3364,7 +3365,7 @@
        const struct exception_table_entry *e = NULL;
        struct module *mod;
 
-       preempt_disable();
+       rcu_read_lock();
        list_for_each_entry_rcu(mod, &modules, list) {
                if (mod->num_exentries == 0)
                        continue;
@@ -3375,7 +3376,7 @@
                if (e)
                        break;
        }
-       preempt_enable();
+       rcu_read_unlock();
 
        /* Now, if we found one, we are running inside it now, hence
           we cannot unload the module, hence no refcnt needed. */
@@ -3393,9 +3394,9 @@
 {
        bool ret;
 
-       preempt_disable();
+       rcu_read_lock();
        ret = __module_address(addr) != NULL;
-       preempt_enable();
+       rcu_read_unlock();
 
        return ret;
 }
@@ -3434,9 +3435,9 @@
 {
        bool ret;
 
-       preempt_disable();
+       rcu_read_lock();
        ret = __module_text_address(addr) != NULL;
-       preempt_enable();
+       rcu_read_unlock();
 
        return ret;
 }
@@ -3469,10 +3470,10 @@
 
        printk(KERN_DEFAULT "Modules linked in:");
        /* Most callers should already have preempt disabled, but make sure */
-       preempt_disable();
+       rcu_read_lock();
        list_for_each_entry_rcu(mod, &modules, list)
                printk(" %s%s", mod->name, module_flags(mod, buf));
-       preempt_enable();
+       rcu_read_unlock();
        if (last_unloaded_module[0])
                printk(" [last unloaded: %s]", last_unloaded_module);
        printk("\n");

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

[Chen, Dennis (SRDC SW)]

Adding dynamic debug maintainer -- Jason Baron...

-----Original Message-----
From: Chen, Dennis (SRDC SW) 
Sent: Wednesday, March 07, 2012 10:57 PM
To: linux-kernel@vger.kernel.org
Cc: Chen, Dennis (SRDC SW)
Subject: [PATCH 2/2] Refine mutex and rcu method in module.c, kernel<3.2.9>

1. Add protection code for module_bug_list readers
2. Add a new module_bug_mutex as the mutex of the module_bug_list writers

--- lib/bug.ori.c   2012-03-07 20:42:56.454850872 +0800
+++ lib/bug.c       2012-03-07 20:58:48.562825887 +0800
@@ -42,6 +42,7 @@
 #include <linux/kernel.h>
 #include <linux/bug.h>
 #include <linux/sched.h>
+#include <linux/rcupdate.h>
 
 extern const struct bug_entry __start___bug_table[], __stop___bug_table[];
 
@@ -56,19 +57,24 @@
 
 #ifdef CONFIG_MODULES
 static LIST_HEAD(module_bug_list);
+static DEFINE_MUTEX(module_bug_mutex);
 
 static const struct bug_entry *module_find_bug(unsigned long bugaddr)
 {
        struct module *mod;
 
-       list_for_each_entry(mod, &module_bug_list, bug_list) {
+       rcu_read_lock();
+       list_for_each_entry_rcu(mod, &module_bug_list, bug_list) {
                const struct bug_entry *bug = mod->bug_table;
                unsigned i;
 
                for (i = 0; i < mod->num_bugs; ++i, ++bug)
-                       if (bugaddr == bug_addr(bug))
+                       if (bugaddr == bug_addr(bug)){
+                               rcu_read_unlock();
                                return bug;
+                        }
        }
+       rcu_read_unlock();
        return NULL;
 }
 
@@ -96,7 +102,9 @@
         * traversals, but since we only traverse on BUG()s, a spinlock
         * could potentially lead to deadlock and thus be counter-productive.
         */
-       list_add(&mod->bug_list, &module_bug_list);
+       mutex_lock(&module_bug_mutex);
+       list_add_rcu(&mod->bug_list, &module_bug_list);
+       mutex_unlock(&module_bug_mutex);
 }
 
 void module_bug_cleanup(struct module *mod)

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

[Rusty Russell]

On Wed, 7 Mar 2012 14:51:06 +0000, "Chen, Dennis (SRDC SW)" <Dennis1.Chen@amd.com> wrote:
> 1. Narrow down the granularity of mutex_lock/ mutex_unlock
> 2. Replace some unnecessary mutex_lock/mutex_unlock pairs with RCU
> 3. Refine the consistent calling style of RCU functioan 

Hi Dennis,

        This follows a logical evolution, where we wean off the mutex,
but AFAICT this is lost in the noise.  Taking the mutex might be naive,
but adding or removing a module is the slow path.  Or am I missing
something?

> -
> -       /* Now sew it into the lists so we can get lockdep and oops
> -        * info during argument parsing.  No one should access us, since
> -        * strong_try_module_get() will fail.
> -        * lockdep/oops can run asynchronous, so use the RCU list insertion
> -        * function to insert in a way safe to concurrent readers.
> -        * The mutex protects against concurrent writers.
> -        */
> -       mutex_lock(&module_mutex);
> +        
> +        /* Concurrent writers for the global modules list are protected by RCU*/
>         if (find_module(mod->name)) {
>                 err = -EEXIST;
>                 goto unlock;
>         }

RCU does not protect concurrent writers:

> -
> +        
>         /* This has to be done once we're sure module name is unique. */
>         dynamic_debug_setup(info.debug, info.num_debug);

Now this is racy...

Cheers,
Rusty.
-- 
  How could I marry someone with more hair than me?  http://baldalex.org

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

[Chen, Dennis (SRDC SW)]

From: Rusty Russell [mailto:rusty@ozlabs.org] 
Sent: Thursday, March 08, 2012 5:18 PM
To: Chen, Dennis (SRDC SW); linux-kernel@vger.kernel.org
Cc: Chen, Dennis (SRDC SW)
Subject: Re: [PATCH 1/2] Refine mutex and rcu method in module.c, kernel<3.2.9>

On Wed, 7 Mar 2012 14:51:06 +0000, "Chen, Dennis (SRDC SW)" <Dennis1.Chen@amd.com> wrote:
> 1. Narrow down the granularity of mutex_lock/ mutex_unlock
> 2. Replace some unnecessary mutex_lock/mutex_unlock pairs with RCU
> 3. Refine the consistent calling style of RCU functioan 

Hi Dennis,

        This follows a logical evolution, where we wean off the mutex,
but AFAICT this is lost in the noise.  Taking the mutex might be naive,
but adding or removing a module is the slow path.  Or am I missing
something?

> -
> -       /* Now sew it into the lists so we can get lockdep and oops
> -        * info during argument parsing.  No one should access us, since
> -        * strong_try_module_get() will fail.
> -        * lockdep/oops can run asynchronous, so use the RCU list insertion
> -        * function to insert in a way safe to concurrent readers.
> -        * The mutex protects against concurrent writers.
> -        */
> -       mutex_lock(&module_mutex);
> +        
> +        /* Concurrent writers for the global modules list are protected by RCU*/
>         if (find_module(mod->name)) {
>                 err = -EEXIST;
>                 goto unlock;
>         }

RCU does not protect concurrent writers:

> -
> +        
>         /* This has to be done once we're sure module name is unique. */
>         dynamic_debug_setup(info.debug, info.num_debug);

Now this is racy...

======================================================================================

Hi Rusty,

I known RCU doesn't protect concurrent writers, so all the writers for the global modules list has been 
protected by the original module_mutex in the 2-patch, I just make the scope of module_mutex become smaller 
as it can, for example, dynamic_debug_setup doesn't touch modules, so it should not be in the protection of
module_mutex.

I am a person like to see a perfect world, especially in the kernel space:) Maybe I can write a test case
to trigger something you don't expect to see while the original codes will...let's think about it

BRs,
Dennis 

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

[Chen, Dennis (SRDC SW)]

From: Chen, Dennis (SRDC SW) 
Sent: Thursday, March 08, 2012 8:18 PM
To: Rusty Russell; linux-kernel@vger.kernel.org
Cc: Chen, Dennis (SRDC SW)
Subject: RE: [PATCH 1/2] Refine mutex and rcu method in module.c, kernel<3.2.9>

From: Rusty Russell [mailto:rusty@ozlabs.org] 
Sent: Thursday, March 08, 2012 5:18 PM
To: Chen, Dennis (SRDC SW); linux-kernel@vger.kernel.org
Cc: Chen, Dennis (SRDC SW)
Subject: Re: [PATCH 1/2] Refine mutex and rcu method in module.c, kernel<3.2.9>

On Wed, 7 Mar 2012 14:51:06 +0000, "Chen, Dennis (SRDC SW)" <Dennis1.Chen@amd.com> wrote:
> 1. Narrow down the granularity of mutex_lock/ mutex_unlock
> 2. Replace some unnecessary mutex_lock/mutex_unlock pairs with RCU
> 3. Refine the consistent calling style of RCU functioan 

Hi Dennis,

        This follows a logical evolution, where we wean off the mutex,
but AFAICT this is lost in the noise.  Taking the mutex might be naive,
but adding or removing a module is the slow path.  Or am I missing
something?

> -
> -       /* Now sew it into the lists so we can get lockdep and oops
> -        * info during argument parsing.  No one should access us, since
> -        * strong_try_module_get() will fail.
> -        * lockdep/oops can run asynchronous, so use the RCU list insertion
> -        * function to insert in a way safe to concurrent readers.
> -        * The mutex protects against concurrent writers.
> -        */
> -       mutex_lock(&module_mutex);
> +        
> +        /* Concurrent writers for the global modules list are protected by RCU*/
>         if (find_module(mod->name)) {
>                 err = -EEXIST;
>                 goto unlock;
>         }

RCU does not protect concurrent writers:

> -
> +        
>         /* This has to be done once we're sure module name is unique. */
>         dynamic_debug_setup(info.debug, info.num_debug);

Now this is racy...

======================================================================================

Hi Rusty,

I known RCU doesn't protect concurrent writers, so all the writers for the global modules list has been 
protected by the original module_mutex in the 2-patch, I just make the scope of module_mutex become smaller 
as it can, for example, dynamic_debug_setup doesn't touch modules, so it should not be in the protection of
module_mutex.

I am a person like to see a perfect world, especially in the kernel space:) Maybe I can write a test case
to trigger something you don't expect to see while the original codes will...let's think about it

BRs,
Dennis 

+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

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.

BRs,
Dennis 

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

[Cong Wang]

On 03/07/2012 11:52 PM, Chen, Dennis (SRDC SW) wrote:
>
> diff -u module.ori.c module.c>  module.patch
>
> give more details please?

Let me copy-and-paste it for you as you still don't read it. :)

Documentation/SubmittingPatches

Use "diff -up" or "diff -uprN" to create patches.

All changes to the Linux kernel occur in the form of patches, as
generated by diff(1).  When creating your patch, make sure to create it
in "unified diff" format, as supplied by the '-u' argument to diff(1).
Also, please use the '-p' argument which shows which C function each
change is in - that makes the resultant diff a lot easier to read.
Patches should be based in the root kernel source directory,
not in any lower subdirectory.

To create a patch for a single file, it is often sufficient to do:

         SRCTREE= linux-2.6
         MYFILE=  drivers/net/mydriver.c

         cd $SRCTREE
         cp $MYFILE $MYFILE.orig
         vi $MYFILE      # make your change
         cd ..
         diff -up $SRCTREE/$MYFILE{.orig,} > /tmp/patch

To create a patch for multiple files, you should unpack a "vanilla",
or unmodified kernel source tree, and generate a diff against your
own source tree.  For example:

         MYSRC= /devel/linux-2.6

         tar xvfz linux-2.6.12.tar.gz
         mv linux-2.6.12 linux-2.6.12-vanilla
         diff -uprN -X linux-2.6.12-vanilla/Documentation/dontdiff \
                 linux-2.6.12-vanilla $MYSRC > /tmp/patch

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

[Cong Wang]

On 03/08/2012 12:48 AM, Chen, Dennis (SRDC SW) wrote:
> Adding dynamic debug maintainer -- Jason Baron...
>
> -----Original Message-----
> From: Chen, Dennis (SRDC SW)
> Sent: Wednesday, March 07, 2012 10:57 PM
> To: linux-kernel@vger.kernel.org
> Cc: Chen, Dennis (SRDC SW)
> Subject: [PATCH 2/2] Refine mutex and rcu method in module.c, kernel<3.2.9>
>
> 1. Add protection code for module_bug_list readers
> 2. Add a new module_bug_mutex as the mutex of the module_bug_list writers

Take a look at the comments in the code:

         /*
          * Strictly speaking this should have a spinlock to protect against
          * traversals, but since we only traverse on BUG()s, a spinlock
          * could potentially lead to deadlock and thus be 
counter-productive.
          */


module_find_bug() is called in a dying path...

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

[Cong Wang]

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.

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

[Paul E. McKenney]

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