Re: [PATCH v5 05/13] module: Move latched RB-tree support to a separate file

[Christophe Leroy]

Le 09/02/2022 à 18:03, Aaron Tomlin a écrit :
> No functional change.
> 
> This patch migrates module latched RB-tree support
> (e.g. see __module_address()) from core module code
> into kernel/module/tree_lookup.c.
> 
> Signed-off-by: Aaron Tomlin <atomlin@xxxxxxxxxx>
> ---
>   include/linux/module.h      |   4 +-
>   kernel/module/Makefile      |   1 +
>   kernel/module/internal.h    |  34 ++++++++++
>   kernel/module/main.c        | 129 +-----------------------------------
>   kernel/module/tree_lookup.c | 109 ++++++++++++++++++++++++++++++
>   5 files changed, 148 insertions(+), 129 deletions(-)
>   create mode 100644 kernel/module/tree_lookup.c
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 680b31ff57fa..fd6161d78127 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -342,9 +342,9 @@ struct module_layout {
>   #ifdef CONFIG_MODULES_TREE_LOOKUP
>   /* Only touch one cacheline for common rbtree-for-core-layout case. */
>   #define __module_layout_align ____cacheline_aligned
> -#else
> +#else /* !CONFIG_MODULES_TREE_LOOKUP */
>   #define __module_layout_align
> -#endif
> +#endif /* CONFIG_MODULES_TREE_LOOKUP */

What's the added value of those two changes ? That's a five lines #ifdef 
block without any other nested #ifdef.

Commenting an #else / #endif is only usefull when the block is more than 
one screen or when there are nested #ifdef inside the block.

Please keep changes at the minimum.

>   
>   struct mod_kallsyms {
>   	Elf_Sym *symtab;
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index ee20d864ad19..fc6d7a053a62 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -9,4 +9,5 @@ obj-$(CONFIG_MODULE_SIG) += signing.o
>   obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
>   ifdef CONFIG_MODULES
>   obj-$(CONFIG_LIVEPATCH) += livepatch.o
> +obj-$(CONFIG_MODULES_TREE_LOOKUP) += tree_lookup.o
>   endif
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index d252e0af1c54..08b6be037b72 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -9,6 +9,7 @@
>   #include <linux/compiler.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
> +#include <linux/rculist.h>
>   
>   #ifndef ARCH_SHF_SMALL
>   #define ARCH_SHF_SMALL 0
> @@ -90,3 +91,36 @@ static inline void module_decompress_cleanup(struct load_info *info)
>   {
>   }
>   #endif
> +
> +#ifdef CONFIG_MODULES_TREE_LOOKUP
> +struct mod_tree_root {
> +	struct latch_tree_root root;
> +	unsigned long addr_min;
> +	unsigned long addr_max;
> +};
> +
> +extern struct mod_tree_root mod_tree;
> +
> +void mod_tree_insert(struct module *mod);
> +void mod_tree_remove_init(struct module *mod);
> +void mod_tree_remove(struct module *mod);
> +struct module *mod_find(unsigned long addr);
> +#else /* !CONFIG_MODULES_TREE_LOOKUP */
> +static unsigned long module_addr_min = -1UL, module_addr_max;

This is wrong to put that in a .h.

By chance module_addr_min re used only in main.c but if they were used 
in another file you would get two independant versions of it.

So leave it in main.c, anyway it's going away with my series.

> +
> +static void mod_tree_insert(struct module *mod) { }
> +static void mod_tree_remove_init(struct module *mod) { }
> +static void mod_tree_remove(struct module *mod) { }
> +static struct module *mod_find(unsigned long addr)

Also keep mod_find() in main.c, or make it a 'static inline'. Otherwise 
it will be duplicated in every file including internal.h

> +{
> +	struct module *mod;
> +
> +	list_for_each_entry_rcu(mod, &modules, list,
> +				lockdep_is_held(&module_mutex)) {
> +		if (within_module(addr, mod))
> +			return mod;
> +	}
> +
> +	return NULL;
> +}
> +#endif /* CONFIG_MODULES_TREE_LOOKUP */
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 5f5bd7152b55..f733a719c65d 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -90,138 +90,13 @@ static DECLARE_WORK(init_free_wq, do_free_init);
>   static LLIST_HEAD(init_free_list);
>   
>   #ifdef CONFIG_MODULES_TREE_LOOKUP
> -
> -/*
> - * Use a latched RB-tree for __module_address(); this allows us to use
> - * RCU-sched lookups of the address from any context.
> - *
> - * 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.
> - */
> -
> -static __always_inline unsigned long __mod_tree_val(struct latch_tree_node *n)
> -{
> -	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
> -
> -	return (unsigned long)layout->base;
> -}
> -
> -static __always_inline unsigned long __mod_tree_size(struct latch_tree_node *n)
> -{
> -	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
> -
> -	return (unsigned long)layout->size;
> -}
> -
> -static __always_inline bool
> -mod_tree_less(struct latch_tree_node *a, struct latch_tree_node *b)
> -{
> -	return __mod_tree_val(a) < __mod_tree_val(b);
> -}
> -
> -static __always_inline int
> -mod_tree_comp(void *key, struct latch_tree_node *n)
> -{
> -	unsigned long val = (unsigned long)key;
> -	unsigned long start, end;
> -
> -	start = __mod_tree_val(n);
> -	if (val < start)
> -		return -1;
> -
> -	end = start + __mod_tree_size(n);
> -	if (val >= end)
> -		return 1;
> -
> -	return 0;
> -}
> -
> -static const struct latch_tree_ops mod_tree_ops = {
> -	.less = mod_tree_less,
> -	.comp = mod_tree_comp,
> -};
> -
> -static struct mod_tree_root {
> -	struct latch_tree_root root;
> -	unsigned long addr_min;
> -	unsigned long addr_max;
> -} mod_tree __cacheline_aligned = {
> +struct mod_tree_root mod_tree __cacheline_aligned = {
>   	.addr_min = -1UL,
>   };
>   
>   #define module_addr_min mod_tree.addr_min
>   #define module_addr_max mod_tree.addr_max
> -
> -static noinline void __mod_tree_insert(struct mod_tree_node *node)
> -{
> -	latch_tree_insert(&node->node, &mod_tree.root, &mod_tree_ops);
> -}
> -
> -static void __mod_tree_remove(struct mod_tree_node *node)
> -{
> -	latch_tree_erase(&node->node, &mod_tree.root, &mod_tree_ops);
> -}
> -
> -/*
> - * These modifications: insert, remove_init and remove; are serialized by the
> - * module_mutex.
> - */
> -static void mod_tree_insert(struct module *mod)
> -{
> -	mod->core_layout.mtn.mod = mod;
> -	mod->init_layout.mtn.mod = mod;
> -
> -	__mod_tree_insert(&mod->core_layout.mtn);
> -	if (mod->init_layout.size)
> -		__mod_tree_insert(&mod->init_layout.mtn);
> -}
> -
> -static void mod_tree_remove_init(struct module *mod)
> -{
> -	if (mod->init_layout.size)
> -		__mod_tree_remove(&mod->init_layout.mtn);
> -}
> -
> -static void mod_tree_remove(struct module *mod)
> -{
> -	__mod_tree_remove(&mod->core_layout.mtn);
> -	mod_tree_remove_init(mod);
> -}
> -
> -static struct module *mod_find(unsigned long addr)
> -{
> -	struct latch_tree_node *ltn;
> -
> -	ltn = latch_tree_find((void *)addr, &mod_tree.root, &mod_tree_ops);
> -	if (!ltn)
> -		return NULL;
> -
> -	return container_of(ltn, struct mod_tree_node, node)->mod;
> -}
> -
> -#else /* MODULES_TREE_LOOKUP */
> -
> -static unsigned long module_addr_min = -1UL, module_addr_max = 0;
> -
> -static void mod_tree_insert(struct module *mod) { }
> -static void mod_tree_remove_init(struct module *mod) { }
> -static void mod_tree_remove(struct module *mod) { }
> -
> -static struct module *mod_find(unsigned long addr)
> -{
> -	struct module *mod;
> -
> -	list_for_each_entry_rcu(mod, &modules, list,
> -				lockdep_is_held(&module_mutex)) {
> -		if (within_module(addr, mod))
> -			return mod;
> -	}
> -
> -	return NULL;
> -}
> -
> -#endif /* MODULES_TREE_LOOKUP */
> +#endif
>   
>   /*
>    * Bounds of module text, for speeding up __module_address.
> diff --git a/kernel/module/tree_lookup.c b/kernel/module/tree_lookup.c
> new file mode 100644
> index 000000000000..037d6eb2f56f
> --- /dev/null
> +++ b/kernel/module/tree_lookup.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Modules tree lookup
> + *
> + * Copyright (C) 2015 Peter Zijlstra
> + * Copyright (C) 2015 Rusty Russell
> + */
> +
> +#include <linux/module.h>
> +#include <linux/rbtree_latch.h>
> +#include "internal.h"
> +
> +/*
> + * Use a latched RB-tree for __module_address(); this allows us to use
> + * RCU-sched lookups of the address from any context.
> + *
> + * 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.
> + */
> +
> +__always_inline unsigned long __mod_tree_val(struct latch_tree_node *n)

Should be static.


> +{
> +	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
> +
> +	return (unsigned long)layout->base;
> +}
> +
> +__always_inline unsigned long __mod_tree_size(struct latch_tree_node *n)

Should be static.


> +{
> +	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
> +
> +	return (unsigned long)layout->size;
> +}
> +
> +__always_inline bool
> +mod_tree_less(struct latch_tree_node *a, struct latch_tree_node *b)

Should be static.


> +{
> +	return __mod_tree_val(a) < __mod_tree_val(b);
> +}
> +
> +__always_inline int
> +mod_tree_comp(void *key, struct latch_tree_node *n)

Should be static.

> +{
> +	unsigned long val = (unsigned long)key;
> +	unsigned long start, end;
> +
> +	start = __mod_tree_val(n);
> +	if (val < start)
> +		return -1;
> +
> +	end = start + __mod_tree_size(n);
> +	if (val >= end)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +const struct latch_tree_ops mod_tree_ops = {
> +	.less = mod_tree_less,
> +	.comp = mod_tree_comp,
> +};

Should be static.


> +
> +static noinline void __mod_tree_insert(struct mod_tree_node *node)
> +{
> +	latch_tree_insert(&node->node, &mod_tree.root, &mod_tree_ops);
> +}
> +
> +static void __mod_tree_remove(struct mod_tree_node *node)
> +{
> +	latch_tree_erase(&node->node, &mod_tree.root, &mod_tree_ops);
> +}
> +
> +/*
> + * These modifications: insert, remove_init and remove; are serialized by the
> + * module_mutex.
> + */
> +void mod_tree_insert(struct module *mod)
> +{
> +	mod->core_layout.mtn.mod = mod;
> +	mod->init_layout.mtn.mod = mod;
> +
> +	__mod_tree_insert(&mod->core_layout.mtn);
> +	if (mod->init_layout.size)
> +		__mod_tree_insert(&mod->init_layout.mtn);
> +}
> +
> +void mod_tree_remove_init(struct module *mod)
> +{
> +	if (mod->init_layout.size)
> +		__mod_tree_remove(&mod->init_layout.mtn);
> +}
> +
> +void mod_tree_remove(struct module *mod)
> +{
> +	__mod_tree_remove(&mod->core_layout.mtn);
> +	mod_tree_remove_init(mod);
> +}
> +
> +struct module *mod_find(unsigned long addr)
> +{
> +	struct latch_tree_node *ltn;
> +
> +	ltn = latch_tree_find((void *)addr, &mod_tree.root, &mod_tree_ops);
> +	if (!ltn)
> +		return NULL;
> +
> +	return container_of(ltn, struct mod_tree_node, node)->mod;
> +}