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

[Aaron Tomlin]

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