Re: [PATCH 3/4] module: use a structure to encapsulate layout.

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Nov 09, 2015 at 02:53:56PM +1030, Rusty Russell wrote:

> diff --git a/include/linux/module.h b/include/linux/module.h
> index 3a19c79918e0..6e68e8cf4d0d 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -302,6 +302,28 @@ struct mod_tree_node {
>  	struct latch_tree_node node;
>  };
>  
> +struct module_layout {
> +	/* The actual code + data. */
> +	void *base;
> +	/* Total size. */
> +	unsigned int size;
> +	/* The size of the executable code.  */
> +	unsigned int text_size;
> +	/* Size of RO section of the module (text+rodata) */
> +	unsigned int ro_size;

There's a 4 byte hole here, but I suppose that's OK, this arrangement
does simplify things.

> +
> +#ifdef CONFIG_MODULES_TREE_LOOKUP
> +	struct mod_tree_node mtn;
> +#endif
> +};
> +
> +#ifdef CONFIG_MODULES_TREE_LOOKUP
> +/* Only touch one cacheline for common rbtree-for-core-layout case. */
> +#define __module_layout_align ____cacheline_aligned
> +#else
> +#define __module_layout_align
> +#endif
> +
>  struct module {
>  	enum module_state state;
>  

> diff --git a/kernel/module.c b/kernel/module.c
> index 14b224967e7b..a0a3d6d9d5e8 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -108,13 +108,6 @@ static LIST_HEAD(modules);
>   * Use a latched RB-tree for __module_address(); this allows us to use
>   * RCU-sched lookups of the address from any context.
>   *
> - * Because modules have two address ranges: init and core, we need two
> - * latch_tree_nodes entries. Therefore we need the back-pointer from
> - * mod_tree_node.

We still have the back-pointers, so removing all of that seems a little
excessive.

> - *
> - * Because init ranges are short lived we mark them unlikely and have placed
> - * them outside the critical cacheline in struct module.

This information also isn't preserved.

>   * This is conditional on PERF_EVENTS || TRACING because those can really hit
>   * __module_address() hard by doing a lot of stack unwinding; potentially from
>   * NMI context.
> @@ -122,24 +115,16 @@ static LIST_HEAD(modules);
>  
>  static __always_inline unsigned long __mod_tree_val(struct latch_tree_node *n)
>  {
> -	struct mod_tree_node *mtn = container_of(n, struct mod_tree_node, node);
> -	struct module *mod = mtn->mod;
> +	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
>  
> -	if (unlikely(mtn == &mod->mtn_init))
> -		return (unsigned long)mod->module_init;
> -
> -	return (unsigned long)mod->module_core;
> +	return (unsigned long)layout->base;
>  }
>  
>  static __always_inline unsigned long __mod_tree_size(struct latch_tree_node *n)
>  {
> -	struct mod_tree_node *mtn = container_of(n, struct mod_tree_node, node);
> -	struct module *mod = mtn->mod;
> -
> -	if (unlikely(mtn == &mod->mtn_init))
> -		return (unsigned long)mod->init_size;
> +	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
>  
> -	return (unsigned long)mod->core_size;
> +	return (unsigned long)layout->size;
>  }

Nice!

> @@ -197,23 +182,23 @@ static void __mod_tree_remove(struct mod_tree_node *node)
>   */
>  static void mod_tree_insert(struct module *mod)
>  {
> -	mod->mtn_core.mod = mod;
> -	mod->mtn_init.mod = mod;
> +	mod->core_layout.mtn.mod = mod;
> +	mod->init_layout.mtn.mod = mod;

 ^ back-pointers :-)

> -	__mod_tree_insert(&mod->mtn_core);
> -	if (mod->init_size)
> -		__mod_tree_insert(&mod->mtn_init);
> +	__mod_tree_insert(&mod->core_layout.mtn);
> +	if (mod->init_layout.size)
> +		__mod_tree_insert(&mod->init_layout.mtn);
>  }

Aside from these minor nits,

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>