On Thu 2022-02-10 12:03 +0000, Christophe Leroy wrote: > > 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 */ > Commenting an #else / #endif is only usefull when the block is more than > one screen or when there are nested #ifdef inside the block. For me, this is a personal style preference. That being said, fair enough. > > +#else /* !CONFIG_MODULES_TREE_LOOKUP */ > > +static unsigned long module_addr_min = -1UL, module_addr_max; > > This is wrong to put that in a .h. > I understand. This was an oversight. I'll move this to kernel/module/main.c in preparation for your work. > > +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 Agreed. This too was an oversight. I'll use the 'inline' keyword here. > > 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. > > +__always_inline unsigned long __mod_tree_size(struct latch_tree_node *n) > > Should be static. > > +__always_inline bool > > +mod_tree_less(struct latch_tree_node *a, struct latch_tree_node *b) > > Should be static. > > > > +__always_inline int > > +mod_tree_comp(void *key, struct latch_tree_node *n) > > Should be static. > > > +const struct latch_tree_ops mod_tree_ops = { > > + .less = mod_tree_less, > > + .comp = mod_tree_comp, > > +}; > > Should be static. Agreed. Only used in kernel/module/tree_lookup.c. -- Aaron Tomlin