Re: [PATCH v5 10/10] module: Rework module_addr_{min,max}

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Apr 13, 2015 at 06:56:36PM +0200, Ingo Molnar wrote:
> > + * Bounds of module allocation, for speeding up __module_address.
> > + * Protected by module_mutex.
> > + */
> > +static unsigned long module_addr_min = -1UL, module_addr_max = 0;
> 
> I suspect the same .data vs. .bss problem affects the #else branch as 
> well?

Yes, but the linear walk already has a 'problem', other than the linear
walk itself being one, the list_head isn't actually on the same line as
the 'key' entries -- although I suppose I could fix that for the
!CONFIG_MODULES_TREE_LOOKUP case.

> If so then it would make sense IMHO to put the structure definition 
> into generic code so that both variants benefit from the shared 
> cacheline?

Isn't this optimizing hopeless code? I mean, I can make the change;
something like the below. Although I suppose we should use
____cacheline_aligned here and just take the false sharing.

--- a/kernel/module.c
+++ b/kernel/module.c
@@ -230,11 +230,15 @@ static struct module *mod_find(unsigned
 
 #else /* MODULES_TREE_LOOKUP */
 
-/*
- * Bounds of module allocation, for speeding up __module_address.
- * Protected by module_mutex.
- */
-static unsigned long module_addr_min = -1UL, module_addr_max = 0;
+static struct mod_bounds {
+	unsigned long addr_min;
+	unsigned long addr_max;
+} mod_bounds __cacheline_aligned = {
+	.addr_min = -1UL,
+};
+
+#define module_addr_min mod_bounds.addr_min
+#define module_addr_max mod_bounds.addr_max
 
 static void mod_tree_insert(struct module *mod) { }
 static void mod_tree_remove_init(struct module *mod) { }
@@ -254,6 +258,10 @@ static struct module *mod_find(unsigned
 
 #endif /* MODULES_TREE_LOOKUP */
 
+/*
+ * Bounds of module text, for speeding up __module_address.
+ * Protected by module_mutex.
+ */
 static void __mod_update_bounds(void *base, unsigned int size)
 {
 	unsigned long min = (unsigned long)base;
@@ -3363,8 +3371,8 @@ static int add_unformed_module(struct mo
 		err = -EEXIST;
 		goto out;
 	}
-	list_add_rcu(&mod->list, &modules);
 	mod_update_bounds(mod);
+	list_add_rcu(&mod->list, &modules);
 	mod_tree_insert(mod);
 	err = 0;