latched RB-trees and __module_address()
Subsystems (5)
Peter Zijlstra
maintainerv4 cover latched RB-trees and __module_address()
This series is aimed at making __module_address() go fast(er).
The reason for doing so is that most stack unwinders use kernel_text_address() to validate each frame. Perf and ftrace (can) end up doing a lot of stack traces from performance sensitive code.
On the way there it:
- annotates and sanitizes module locking
- introduces the latched RB-tree
- employs it to make __module_address() go fast.
I've build and boot tested this on x86_64 with modules and lockdep enabled. Performance numbers (below) are done with lockdep disabled.
As previously mentioned; the reason for writing the latched RB-tree as generic code is mostly for clarity/documentation purposes; as there are a number of separate and non trivial bits to the complete solution.
As measued on my ivb-ep system with 84 modules loaded; prior to patching the test module (below) reports:
avg +- stdev
Before: 1689 +- 287 [ns] per __module_address() call After: 137 +- 38 [ns] per __module_address() call
Note; I have also tested things like: perf record -a -g modprobe mod_test, to make 'sure' to hit some of the more interesting paths.
Changes since last time:
- depend on CONFIG_PERF_EVENTS || CONFIG_TRACING -- akpm
- use within_module() for jump_labels -- rusty
- minor comment changes -- Compudj, mingo
Rusty, please consider merging this.
Peter Zijlstra
maintainerPATCH v4 1/9 module: Sanitize RCU usage and locking
Currently the RCU usage in module is an inconsistent mess of RCU and RCU-sched, this is broken for CONFIG_PREEMPT where synchronize_rcu() does not imply synchronize_sched().
Most usage sites use preempt_{dis,en}able() which is RCU-sched, but (most of) the modification sites use synchronize_rcu(). With the exception of the module bug list, which actually uses RCU.
Convert everything over to RCU-sched.
Furthermore add lockdep asserts to all sites, because its not at all clear to me the required locking is observed, esp. on exported functions.
--- include/linux/module.h | 12 ++++++++++-- kernel/module.c | 42 ++++++++++++++++++++++++++++++++++-------- lib/bug.c | 7 +++++-- 3 files changed, 49 insertions(+), 12 deletions(-)
include/linux/module.h +10 -2
@@ -415,14 +415,22 @@ struct symsearch {bool unused; }; -/* Search for an exported symbol by name. */ +/* +* Search for an exported symbol by name. +* +* Must be called with module_mutex held or preemption disabled. +*/ const struct kernel_symbol *find_symbol (const char *name,struct module **owner,const unsigned long **crc,bool gplok,bool warn); -/* Walk the exported symbol table */ +/* +* Walk the exported symbol table +* +* Must be called with module_mutex held or preemption disabled. +*/ bool each_symbol_section (bool (*fn)(const struct symsearch *arr,struct module *owner,void *data),void *data);
kernel/module.c +34 -8
91 lines not on this page · raw message
lib/bug.c +5 -2
@@ -66,7 +66,7 @@ static const struct bug_entry *module_fistruct module *mod;const struct bug_entry *bug =NULL ; -rcu_read_lock (); +rcu_read_lock_sched ();list_for_each_entry_rcu (mod, &module_bug_list, bug_list) {unsigned i;
@@ -77,7 +77,7 @@ static const struct bug_entry *module_fi } bug =NULL ; out: -rcu_read_unlock (); +rcu_read_unlock_sched ();return bug; }
@@ -88,6 +88,8 @@ void module_bug_finalize(const Elf_Ehdrchar *secstrings;unsigned int i; +lockdep_assert_held (&module_mutex); + mod->bug_table =NULL ; mod->num_bugs =0 ;
@@ -113,6 +115,7 @@ void module_bug_finalize(const Elf_Ehdrvoid module_bug_cleanup (struct module *mod) { +lockdep_assert_held (&module_mutex);list_del_rcu (&mod->bug_list); }
Re: [PATCH v4 1/9] module: Sanitize RCU usage and locking
kernel/module.c static LIST_HEAD(modules); · line 125
Quoted 12 lines · Peter Zijlstra
kernel/module.c +12
-0+static void module_assert_mutex_or_preempt (void ) +{ +#ifdef CONFIG_LOCKDEP +int rcu_held =rcu_read_lock_sched_held (); +int mutex_held =1 ; + +if (debug_locks) + mutex_held =lockdep_is_held (&module_mutex); + +WARN_ON (!rcu_held && !mutex_held); +#endif +}
Is rcu_lockdep_assert() suitable for it?
(note rcu_lockdep_assert() only works when CONFIG_PROVE_RCU)
Peter Zijlstra
maintainerRe: [PATCH v4 1/9] module: Sanitize RCU usage and locking
Quoted 17 lines · Lai Jiangshan
kernel/module.c static LIST_HEAD(modules); · line 125
Quoted 12 lines · Peter Zijlstra
kernel/module.c +12
-0+static void module_assert_mutex_or_preempt (void ) +{ +#ifdef CONFIG_LOCKDEP +int rcu_held =rcu_read_lock_sched_held (); +int mutex_held =1 ; + +if (debug_locks) + mutex_held =lockdep_is_held (&module_mutex); + +WARN_ON (!rcu_held && !mutex_held); +#endif +}Is
rcu_lockdep_assert()suitable for it?
(notercu_lockdep_assert()only works whenCONFIG_PROVE_RCU)
Nah, this works.
Peter Zijlstra
maintainerPATCH v4 2/9 module: Annotate module version magic
In reply to Peter ZijlstraDue to the new lockdep checks we go:
[ 9.759380] ------------[ cut here ]------------ [ 9.759389] WARNING: CPU: 31 PID: 597 at ../kernel/module.c:216 each_symbol_section+0x121/0x130() [ 9.759391] Modules linked in: [ 9.759393] CPU: 31 PID: 597 Comm: modprobe Not tainted 4.0.0-rc1+ #65 [ 9.759393] Hardware name: Intel Corporation S2600GZ/S2600GZ, BIOS SE5C600.86B.02.02.0002.122320131210 12/23/2013 [ 9.759396] ffffffff817d8676 ffff880424567ca8 ffffffff8157e98b 0000000000000001 [ 9.759398] 0000000000000000 ffff880424567ce8 ffffffff8105fbc7 ffff880424567cd8 [ 9.759400] 0000000000000000 ffffffff810ec160 ffff880424567d40 0000000000000000 [ 9.759400] Call Trace: [ 9.759407] [<ffffffff8157e98b>] dump_stack+0x4f/0x7b [ 9.759410] [<ffffffff8105fbc7>] warn_slowpath_common+0x97/0xe0 [ 9.759412] [<ffffffff810ec160>] ? section_objs+0x60/0x60 [ 9.759414] [<ffffffff8105fc2a>] warn_slowpath_null+0x1a/0x20 [ 9.759415] [<ffffffff810ed9c1>] each_symbol_section+0x121/0x130 [ 9.759417] [<ffffffff810eda01>] find_symbol+0x31/0x70 [ 9.759420] [<ffffffff810ef5bf>] load_module+0x20f/0x2660 [ 9.759422] [<ffffffff8104ef10>] ? __do_page_fault+0x190/0x4e0 [ 9.759426] [<ffffffff815880ec>] ? retint_restore_args+0x13/0x13 [ 9.759427] [<ffffffff815880ec>] ? retint_restore_args+0x13/0x13 [ 9.759433] [<ffffffff810ae73d>] ? trace_hardirqs_on_caller+0x11d/0x1e0 [ 9.759437] [<ffffffff812fcc0e>] ? trace_hardirqs_on_thunk+0x3a/0x3f [ 9.759439] [<ffffffff815880ec>] ? retint_restore_args+0x13/0x13 [ 9.759441] [<ffffffff810f1ade>] SyS_init_module+0xce/0x100 [ 9.759443] [<ffffffff81587429>] system_call_fastpath+0x12/0x17 [ 9.759445] ---[ end trace 9294429076a9c644 ]---
As per the comment this site should be fine, but lets wrap it in preempt_disable() anyhow to placate lockdep.
--- kernel/module.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
kernel/module.c +9 -3
@@ -1191,11 +1191,17 @@ static inline int check_modstruct_versio {const unsigned long *crc; -/* Since this should be found in kernel (which can't be removed), -* no locking is necessary. */ +/* +* Since this should be found in kernel (which can't be removed), no +* locking is necessary -- use preempt_disable() to placate lockdep. +*/ +preempt_disable ();if (!find_symbol (VMLINUX_SYMBOL_STR (module_layout),NULL , - &crc, true, false)) + &crc, true, false)) { +preempt_enable ();BUG (); + } +preempt_enable ();return check_version (sechdrs, versindex,VMLINUX_SYMBOL_STR (module_layout), mod, crc,NULL );
Peter Zijlstra
maintainerPATCH v4 3/9 module, jump_label: Fix module locking
In reply to Peter ZijlstraAs per the module core lockdep annotations:
[ 18.034047] ---[ end trace 9294429076a9c673 ]--- [ 18.047760] Hardware name: Intel Corporation S2600GZ/S2600GZ, BIOS SE5C600.86B.02.02.0002.122320131210 12/23/2013 [ 18.059228] ffffffff817d8676 ffff880036683c38 ffffffff8157e98b 0000000000000001 [ 18.067541] 0000000000000000 ffff880036683c78 ffffffff8105fbc7 ffff880036683c68 [ 18.075851] ffffffffa0046b08 0000000000000000 ffffffffa0046d00 ffffffffa0046cc8 [ 18.084173] Call Trace: [ 18.086906] [<ffffffff8157e98b>] dump_stack+0x4f/0x7b [ 18.092649] [<ffffffff8105fbc7>] warn_slowpath_common+0x97/0xe0 [ 18.099361] [<ffffffff8105fc2a>] warn_slowpath_null+0x1a/0x20 [ 18.105880] [<ffffffff810ee502>] __module_address+0x1d2/0x1e0 [ 18.112400] [<ffffffff81161153>] jump_label_module_notify+0x143/0x1e0 [ 18.119710] [<ffffffff810814bf>] notifier_call_chain+0x4f/0x70 [ 18.126326] [<ffffffff8108160e>] __blocking_notifier_call_chain+0x5e/0x90 [ 18.134009] [<ffffffff81081656>] blocking_notifier_call_chain+0x16/0x20 [ 18.141490] [<ffffffff810f0f00>] load_module+0x1b50/0x2660 [ 18.147720] [<ffffffff810f1ade>] SyS_init_module+0xce/0x100 [ 18.154045] [<ffffffff81587429>] system_call_fastpath+0x12/0x17 [ 18.160748] ---[ end trace 9294429076a9c674 ]---
Jump labels is not doing it right; fix this.
--- kernel/jump_label.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
kernel/jump_label.c +6 -4
@@ -302,7 +302,7 @@ static int jump_label_add_module(structcontinue ; key = iterk; -if (__module_address (iter->key) == mod) { +if (within_module (iter->key, mod)) {/* * Set key->entries to iter, but preserve JUMP_LABEL_TRUE_BRANCH. */
@@ -339,7 +339,7 @@ static void jump_label_del_module(struct key = (struct static_key *)(unsigned long )iter->key; -if (__module_address (iter->key) == mod) +if (within_module (iter->key, mod))continue ; prev = &key->next;
@@ -443,14 +443,16 @@ static void jump_label_update(struct sta {struct jump_entry *stop = __stop___jump_table;struct jump_entry *entry =jump_label_get_entries (key); -#ifdef CONFIG_MODULES -struct module *mod =__module_address ((unsigned long )key); +struct module *mod;__jump_label_mod_update (key, enable); +preempt_disable (); + mod =__module_address ((unsigned long )key);if (mod) stop = mod->jump_entries + mod->num_jump_entries; +preempt_enable ();#endif /* if there are no users, entry can be NULL */ if (entry)
Peter Zijlstra
maintainerPATCH v4 4/9 rbtree: Make lockless searches non-fatal
In reply to Peter ZijlstraChange the insert and erase code such that lockless searches are non-fatal.
In and of itself an rbtree cannot be correctly searched while in-modification, we can however provide weaker guarantees that will allow the rbtree to be used in conjunction with other techniques, such as latches; see 9b0fd802e8c0 ("seqcount: Add raw_write_seqcount_latch()").
For this to work we need the following guarantees from the rbtree code:
1) a lockless reader must not see partial stores, this would allow it to observe nodes that are invalid memory.
2) there must not be (temporary) loops in the tree structure in the modifier's program order, this would cause a lookup which interrupts the modifier to get stuck indefinitely.
For 1) we must use WRITE_ONCE() for all updates to the tree structure; in particular this patch only does rb_{left,right} as those are the only element required for simple searches.
It generates slightly worse code, probably because volatile. But in pointer chasing heavy code a few instructions more should not matter.
For 2) I have carefully audited the code and drawn every intermediate link state and not found a loop.
--- include/linux/rbtree.h | 10 +++++ include/linux/rbtree_augmented.h | 21 +++++++--- lib/rbtree.c | 76 +++++++++++++++++++++++++++------------ 3 files changed, 78 insertions(+), 29 deletions(-)
include/linux/rbtree.h +10 -0
@@ -31,6 +31,7 @@#include <linux/kernel.h> #include <linux/stddef.h> +#include <linux/rcupdate.h> struct rb_node {unsigned long __rb_parent_color;
@@ -85,6 +86,15 @@ static inline void rb_link_node(struct r *rb_link = node; } +static inline void rb_link_node_rcu (struct rb_node * node,struct rb_node * parent, +struct rb_node ** rb_link) +{ + node->__rb_parent_color = (unsigned long )parent; + node->rb_left = node->rb_right =NULL ; + +rcu_assign_pointer (*rb_link, node); +} +#define rb_entry_safe (ptr, type, member) \ ({ typeof(ptr) ____ptr = (ptr); \ ____ptr ? rb_entry(____ptr, type, member) : NULL; \
include/linux/rbtree_augmented.h +14 -7
@@ -123,11 +123,11 @@ __rb_change_child(struct rb_node *old, s {if (parent) {if (parent->rb_left == old) - parent->rb_left = new; +WRITE_ONCE (parent->rb_left, new);else - parent->rb_right = new; +WRITE_ONCE (parent->rb_right, new); }else - root->rb_node = new; +WRITE_ONCE (root->rb_node, new); } extern void __rb_erase_color(struct rb_node *parent, struct rb_root *root,
@@ -137,7 +137,8 @@ static __always_inline struct rb_node * __rb_erase_augmented(struct rb_node *node, struct rb_root *root, const struct rb_augment_callbacks *augment) { - struct rb_node *child = node->rb_right, *tmp = node->rb_left; + struct rb_node *child = node->rb_right; +struct rb_node *tmp = node->rb_left;struct rb_node *parent, *rebalance;unsigned long pc;
@@ -167,6 +168,7 @@ __rb_erase_augmented(struct rb_node *nod tmp = parent; }else {struct rb_node *successor = child, *child2; + tmp = child->rb_left;if (!tmp) {/*
@@ -180,6 +182,7 @@ __rb_erase_augmented(struct rb_node *nod*/ parent = successor; child2 = successor->rb_right; + augment->copy (node, successor); }else {/*
@@ -201,19 +204,23 @@ __rb_erase_augmented(struct rb_node *nod successor = tmp; tmp = tmp->rb_left; }while (tmp); - parent->rb_left = child2 = successor->rb_right; - successor->rb_right = child; + child2 = successor->rb_right; +WRITE_ONCE (parent->rb_left, child2); +WRITE_ONCE (successor->rb_right, child);rb_set_parent (child, successor); + augment->copy (node, successor); augment->propagate (parent, successor); } - successor->rb_left = tmp = node->rb_left; + tmp = node->rb_left; +WRITE_ONCE (successor->rb_left, tmp);rb_set_parent (tmp, successor); pc = node->__rb_parent_color; tmp =__rb_parent (pc);__rb_change_child (node, successor, tmp, root); +if (child2) { successor->__rb_parent_color = pc;rb_set_parent_color (child2, parent,RB_BLACK );
lib/rbtree.c +54 -22
142 lines not on this page · raw message
Peter Zijlstra
maintainerPATCH v4 5/9 seqlock: Better document raw_write_seqcount_latch()
In reply to Peter ZijlstraImprove the documentation of the latch technique as used in the current timekeeping code, such that it can be readily employed elsewhere.
Borrow from the comments in timekeeping and replace those with a reference to this more generic comment.
--- include/linux/seqlock.h | 76 +++++++++++++++++++++++++++++++++++++++++++++- kernel/time/timekeeping.c | 27 ---------------- 2 files changed, 76 insertions(+), 27 deletions(-)
include/linux/seqlock.h +75 -1
@@ -233,9 +233,83 @@ static inline void raw_write_seqcount_en s->sequence++; } -/* +/** * raw_write_seqcount_latch - redirect readers to even/odd copy * @s: pointer to seqcount_t +* +* The latch technique is a multiversion concurrency control method that allows +* queries during non atomic modifications. If you can guarantee queries never +* interrupt the modification -- e.g. the concurrency is strictly between CPUs +* -- you most likely do not need this. +* +* Where the traditional RCU/lockless data structures rely on atomic +* modifications to ensure queries observe either the old or the new state the +* latch allows the same for non atomic updates. The trade-off is doubling the +* cost of storage; we have to maintain two copies of the entire data +* structure. +* +* Very simply put: we first modify one copy and then the other. This ensures +* there is always one copy in a stable state, ready to give us an answer. +* +* The basic form is a data structure like: +* +* struct latch_struct { +* seqcount_t seq; +* struct data_struct data[2]; +* }; +* +* Where a modification, which is assumed to be externally serialized, does the +* following: +* +* void latch_modify(struct latch_struct *latch, ...) +* { +* smp_wmb(); <- Ensure that the last data[1] update is visible +* latch->seq++; +* smp_wmb(); <- Ensure that the seqcount update is visible +* +* modify(latch->data[0], ...); +* +* smp_wmb(); <- Ensure that the data[0] update is visible +* latch->seq++; +* smp_wmb(); <- Ensure that the seqcount update is visible +* +* modify(latch->data[1], ...); +* } +* +* The query will have a form like: +* +* struct entry *latch_query(struct latch_struct *latch, ...) +* { +* struct entry *entry; +* unsigned seq, idx; +* +* do { +* seq = latch->seq; +* smp_rmb(); +* +* idx = seq & 0x01; +* entry = data_query(latch->data[idx], ...); +* +* smp_rmb(); +* } while (seq != latch->seq); +* +* return entry; +* } +* +* So during the modification, queries are first redirected to data[1]. Then we +* modify data[0]. When that is complete, we redirect queries back to data[0] +* and we can modify data[1]. +* +* NOTE: The non-requirement for atomic modifications does _NOT_ include +* the publishing of new entries in the case where data is a dynamic +* data structure. +* +* An iteration might start in data[0] and get suspended long enough +* to miss an entire modification sequence, once it resumes it might +* observe the new entry. +* +* NOTE: When data is a dynamic data structure; one should use regular RCU +* patterns to manage the lifetimes of the objects within. */ static inline void raw_write_seqcount_latch (seqcount_t *s) {
kernel/time/timekeeping.c +1 -26
@@ -339,32 +339,7 @@ static inline s64 timekeeping_get_ns_raw* We want to use this from any context including NMI and tracing / * instrumenting the timekeeping code itself. * -* So we handle this differently than the other timekeeping accessor -* functions which retry when the sequence count has changed. The -* update side does: -* -* smp_wmb(); <- Ensure that the last base[1] update is visible -* tkf->seq++; -* smp_wmb(); <- Ensure that the seqcount update is visible -* update(tkf->base[0], tkr); -* smp_wmb(); <- Ensure that the base[0] update is visible -* tkf->seq++; -* smp_wmb(); <- Ensure that the seqcount update is visible -* update(tkf->base[1], tkr); -* -* The reader side does: -* -* do { -* seq = tkf->seq; -* smp_rmb(); -* idx = seq & 0x01; -* now = now(tkf->base[idx]); -* smp_rmb(); -* } while (seq != tkf->seq) -* -* As long as we update base[0] readers are forced off to -* base[1]. Once base[0] is updated readers are redirected to base[0] -* and the base[1] update takes place. +* Employ the latch technique; see @raw_write_seqcount_latch. * * So if a NMI hits the update of base[0] then it will use base[1] * which is still consistent. In the worst case this can result is a
Peter Zijlstra
maintainerPATCH v4 6/9 rbtree: Implement generic latch_tree
In reply to Peter ZijlstraImplement a latched RB-tree in order to get unconditional RCU/lockless lookups.
--- include/linux/rbtree_latch.h | 223 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 223 insertions(+)
include/linux/rbtree_latch.h new file +223 -0
@@ -0,0 +1,223 @@ +/* +* Latched RB-trees +* +* Copyright (C) 2015 Intel Corp., Peter Zijlstra <[email protected]> +* +* Since RB-trees have non atomic modifications they're not immediately suited +* for RCU/lockless queries. Even though we made RB tree lookups non-fatal for +* lockless lookups; we cannot guarantee they return a correct result. +* +* The simplest solution is a seqlock + rb-tree, this will allow lockless +* lookups; but has the constraint (inherent to the seqlock) that read sides +* cannot nest in write sides. +* +* If we need to allow unconditional lookups (say as required for NMI context +* usage) we need a more complex setup; this data structure provides this by +* employing the latch technique -- see @raw_write_seqcount_latch -- to +* implement a latched RB-tree which does allow for unconditional lookups by +* virtue of always having (at least) one stable copy of the tree. +* +* However, while we have the guarantee that there is at all times one stable +* copy, this does not guarantee an iteration will not observe modifications. +* What might have been a stable copy at the start of the iteration, need not +* remain so for the duration of the iteration. +* +* Therefore, this does require a lockless RB-tree iteration to be non-fatal; +* see the comment in lib/rbtree.c. Note however that we only require the first +* condition -- not seeing partial stores -- because the latch thing isolates +* us from loops. If we were to interrupt a modification the lookup would be +* pointed at the stable tree and complete while the modification was halted. +*/ + +#ifndef RB_TREE_LATCH_H +#define RB_TREE_LATCH_H + +#include <linux/rbtree.h> +#include <linux/seqlock.h> + +struct latch_tree_node { +/* +* Because we have an array of two entries in struct latch_tree_nodes +* it's not possible to use container_of() to get back to the +* encapsulating structure; therefore we have to put in a back pointer. +*/ +void *priv; +struct rb_node node; +}; + +struct latch_tree_nodes { +struct latch_tree_node node[2 ]; +}; + +struct latch_tree_root { +seqcount_t seq; +struct rb_root tree[2 ]; +}; + +/** +* latch_tree_ops - operators to define the tree order +* @less: used for insertion; provides the (partial) order between two elements. +* @comp: used for lookups; provides the order between the search key and an element. +* +* The operators are related like: +* +* comp(a->key,b) < 0 := less(a,b) +* comp(a->key,b) > 0 := less(b,a) +* comp(a->key,b) == 0 := !less(a,b) && !less(b,a) +* +* If these operators define a partial order on the elements we make no +* guarantee on which of the elements matching the key is found. See +* latch_tree_find(). +*/ +struct latch_tree_ops { +bool (*less)(struct latch_tree_node *a,struct latch_tree_node *b); +int (*comp)(void *key,struct latch_tree_node *b); +}; + +static __always_inline void +__lt_insert (struct latch_tree_node *ltn,struct rb_root *root, +bool (*less)(struct latch_tree_node *a,struct latch_tree_node *b)) +{ +struct rb_node **link = &root->rb_node; +struct rb_node *parent =NULL ; +struct latch_tree_node *ltp; + +while (*link) { + parent = *link; + ltp = container_of(parent, struct latch_tree_node, node); + +if (less (ltn, ltp)) + link = &parent->rb_left; +else + link = &parent->rb_right; + } + +rb_link_node_rcu (<n->node, parent, link); +rb_insert_color (<n->node, root); +} + +static __always_inline void +__lt_erase (struct latch_tree_node *ltn,struct rb_root *root) +{ +rb_erase (<n->node, root); +} + +static __always_inline struct latch_tree_node * +__lt_find (void *key,struct rb_root *root, +int (*comp)(void *key,struct latch_tree_node *ltn)) +{ +struct rb_node *n =rcu_dereference_raw (root->rb_node); +struct latch_tree_node *ltn; +int c; + +while (n) { + ltn = container_of(n, struct latch_tree_node, node); + c =comp (key, ltn); + +if (c <0 ) + n =rcu_dereference_raw (n->rb_left); +else if (c >0 ) + n =rcu_dereference_raw (n->rb_right); +else +return ltn; + } + +return NULL ; +} + +/** +* latch_tree_insert() - insert @nodes into the trees @root +* @nodes: nodes to insert +* @root: trees to insert @nodes into +* @priv: pointer to node private data +* @ops: operators defining the node order +* +* Initializes @nodes private pointer with @priv; which typically is a back +* pointer to the containing structure, used by @ops to find the search key. +* +* Then it inserts @nodes into @root in an ordered fashion such that we can +* always observe one complete tree. See the comment for +* raw_write_seqcount_latch(). +* +* The inserts use rcu_assign_pointer() to publish the element such that +* both the @priv pointer values in @nodes as the tree structure is stored +* before we can observe the new @nodes. +* +* All modifications (latch_tree_insert, latch_tree_remove) are assumed to be +* serialized. +*/ +static __always_inline void +latch_tree_insert (struct latch_tree_nodes *nodes, +struct latch_tree_root *root, +void *priv, +const struct latch_tree_ops *ops) +{ + nodes->node[0 ].priv = nodes->node[1 ].priv = priv; + +raw_write_seqcount_latch (&root->seq); +__lt_insert (&nodes->node[0 ], &root->tree[0 ], ops->less); +raw_write_seqcount_latch (&root->seq); +__lt_insert (&nodes->node[1 ], &root->tree[1 ], ops->less); +} + +/** +* latch_tree_erase() - removes @nodes from the trees @root +* @nodes: nodes to remote +* @root: trees to remove @nodes from +* @ops: operators defining the node order +* +* Removes @nodes from the trees @root in an ordered fashion such that we can +* always observe one complete tree. See the comment for +* raw_write_seqcount_latch(). +* +* It is assumed that @nodes will observe one RCU quiescent state before being +* reused of freed. +* +* All modifications (latch_tree_insert, latch_tree_remove) are assumed to be +* serialized. +*/ +static __always_inline void +latch_tree_erase (struct latch_tree_nodes *nodes, +struct latch_tree_root *root, +const struct latch_tree_ops *ops) +{ +raw_write_seqcount_latch (&root->seq); +__lt_erase (&nodes->node[0 ], &root->tree[0 ]); +raw_write_seqcount_latch (&root->seq); +__lt_erase (&nodes->node[1 ], &root->tree[1 ]); +} + +/** +* latch_tree_find() - find the node matching @key in the trees @root +* @key: search key +* @root: trees to search for @key +* @ops: operators defining the node order +* +* Does a lockless lookup in the trees @root for the node matching @key. +* +* It is assumed that this is called while holding the appropriate RCU read +* side lock. +* +* If the operators define a partial order on the elements (there are multiple +* elements which have the same key value) it is undefined which of these +* elements will be found. Nor is it possible to iterate the tree to find +* further elements with the same key value. +* +* Returns: a pointer to the node matching @key or NULL. +*/ +static __always_inline struct latch_tree_node * +latch_tree_find (void *key,struct latch_tree_root *root, +const struct latch_tree_ops *ops) +{ +struct latch_tree_node *node; +unsigned int seq; + +do { + seq =raw_read_seqcount (&root->seq); + node =__lt_find (key, &root->tree[seq &1 ], ops->comp); + }while (read_seqcount_retry (&root->seq, seq)); + +return node; +} + +#endif /* RB_TREE_LATCH_H */
Re: [PATCH v4 6/9] rbtree: Implement generic latch_tree
include/linux/rbtree_latch.h line 46
Quoted 10 lines · Peter Zijlstra
include/linux/rbtree_latch.h new file +10
-0+ +struct latch_tree_node { +/* +* Because we have an array of two entries in struct latch_tree_nodes +* it's not possible to use container_of() to get back to the +* encapsulating structure; therefore we have to put in a back pointer. +*/ +void *priv; +struct rb_node node; +};
I don't think @priv is strictly needed. It is possible to use container_of() to go back. @priv is even not used in this file (except the initialization).
First, we can use container_of() to find encapsulating structure from the struct latch_tree_nodeS.
Second, we can add a @idx to __lt_insert() and __lt_find(), thus we can find the encapsulating latch_tree_nodes from rb_node or latch_tree_node. and struct latch_tree_ops uses latch_tree_nodes instead.
Did I miss anything? If the @priv is possible to be removed, removing it will simplify this file but it may add a little more code in the module.c where the ltn_core<n_init can't share the same ->less() and ->comp() after.
If you do remove @priv, please also use rb_node instead of old latch_tree_node and rename old latch_tree_nodes to latch_tree_node.
Peter Zijlstra
maintainerRe: [PATCH v4 6/9] rbtree: Implement generic latch_tree
Quoted 16 lines · Lai Jiangshan
include/linux/rbtree_latch.h line 46
Quoted 10 lines · Peter Zijlstra
include/linux/rbtree_latch.h new file +10
-0+ +struct latch_tree_node { +/* +* Because we have an array of two entries in struct latch_tree_nodes +* it's not possible to use container_of() to get back to the +* encapsulating structure; therefore we have to put in a back pointer. +*/ +void *priv; +struct rb_node node; +};I don't think @priv is strictly needed. It is possible to use
container_of()to go back. @priv is even not used in this file (except the initialization).
Quoted 4 lines
First, we can use
container_of()to find encapsulating structure from the structlatch_tree_nodeS.Second, we can add a @idx to
__lt_insert()and__lt_find(),
insert yes, find no. Remember that both nodes are in the _same_ tree.
There is no way of knowing if a tree node is an init or core node while iterating.
Re: [PATCH v4 6/9] rbtree: Implement generic latch_tree
Quoted 20 lines · Peter Zijlstra
Quoted 19 lines · Lai Jiangshan
include/linux/rbtree_latch.h line 46
Quoted 10 lines · Peter Zijlstra
include/linux/rbtree_latch.h new file +10
-0+ +struct latch_tree_node { +/* +* Because we have an array of two entries in struct latch_tree_nodes +* it's not possible to use container_of() to get back to the +* encapsulating structure; therefore we have to put in a back pointer. +*/ +void *priv; +struct rb_node node; +};I don't think @priv is strictly needed. It is possible to use
container_of()to go back. @priv is even not used in this file (except the initialization).First, we can use
container_of()to find encapsulating structure from the structlatch_tree_nodeS.
Quoted 8 lines
Quoted 1 line
Second, we can add a @idx to
__lt_insert()and__lt_find(),insert yes, find no. Remember that both nodes are in the _same_ tree.
There is no way of knowing if a tree node is an init or core node while iterating.
.
This sentence is talking about module.c not latch_tree.h. So I guess it is user(module.c)'s problem, not latch_tree.h's problem.
The user(module.c) can wrap the struct latch_tree_nodes and add @priv.
Peter Zijlstra
maintainerRe: [PATCH v4 6/9] rbtree: Implement generic latch_tree
Quoted 4 lines · Lai Jiangshan
This sentence is talking about
module.cnotlatch_tree.h. So I guess it is user(module.c)'s problem, not latch_tree.h's problem.The user(module.c) can wrap the struct
latch_tree_nodesand add @priv.
OK, took me a while to figure out exactly what and how. You mean something like this, right?
(compile tested only)
--- include/linux/module.h | 11 ++++- include/linux/rbtree_latch.h | 93 ++++++++++++++++++------------------------- kernel/module.c | 33 +++++++++------ 3 files changed, 70 insertions(+), 67 deletions(-)
include/linux/module.h +9 -2
@@ -211,6 +211,13 @@ enum module_state { MODULE_STATE_UNFORMED, /* Still setting it up. */ }; +struct module ; + +struct mod_tree_node { +struct latch_tree_node node; +struct module *mod; +}; +struct module {enum module_state state;
@@ -294,8 +301,8 @@ struct module {* core.node[0] to be in the same cacheline as the above entries, * we also assume ltn_init has a higher address than ltn_core. */ -struct latch_tree_nodes ltn_core; -struct latch_tree_nodes ltn_init; +struct mod_tree_node mtn_core; +struct mod_tree_node mtn_init;/* Size of RO sections of the module (text+rodata) */ unsigned int init_ro_size, core_ro_size;
include/linux/rbtree_latch.h +41 -52
179 lines not on this page · raw message
kernel/module.c +20 -13
@@ -119,22 +119,24 @@ static LIST_HEAD(modules);static __always_inline unsigned long __mod_tree_val (struct latch_tree_node *n) { -struct module *mod = n->priv; + struct mod_tree_node *mtn = container_of(n, struct mod_tree_node, node); +struct module *mod = mtn->mod; -if (n >= mod->ltn_init.node) +if (unlikely (mtn == &mod->mtn_init))return (unsigned long )mod->module_init; -else -return (unsigned long )mod->module_core; + +return (unsigned long )mod->module_core; }static __always_inline unsigned long __mod_tree_size (struct latch_tree_node *n) { -struct module *mod = n->priv; + struct mod_tree_node *mtn = container_of(n, struct mod_tree_node, node); +struct module *mod = mtn->mod; -if (n >= mod->ltn_init.node) +if (unlikely (mtn == &mod->mtn_init))return (unsigned long )mod->init_size; -else -return (unsigned long )mod->core_size; + +return (unsigned long )mod->core_size; }static __always_inline bool
@@ -174,20 +176,23 @@ static struct latch_tree_root mod_tree _*/ static void mod_tree_insert (struct module *mod) { -latch_tree_insert (&mod->ltn_core, &mod_tree, mod, &mod_tree_ops); + mod->mtn_core.mod = mod; + mod->mtn_init.mod = mod; + +latch_tree_insert (&mod->mtn_core.node, &mod_tree, &mod_tree_ops);if (mod->init_size) -latch_tree_insert (&mod->ltn_init, &mod_tree, mod, &mod_tree_ops); +latch_tree_insert (&mod->mtn_init.node, &mod_tree, &mod_tree_ops); }static void mod_tree_remove_init (struct module *mod) {if (mod->init_size) -latch_tree_erase (&mod->ltn_init, &mod_tree, &mod_tree_ops); +latch_tree_erase (&mod->mtn_init.node, &mod_tree, &mod_tree_ops); }static void mod_tree_remove (struct module *mod) { -latch_tree_erase (&mod->ltn_core, &mod_tree, &mod_tree_ops); +latch_tree_erase (&mod->mtn_core.node, &mod_tree, &mod_tree_ops);mod_tree_remove_init (mod); }
@@ -196,8 +201,10 @@ static struct module *mod_find(unsignedstruct latch_tree_node *ltn; ltn =latch_tree_find ((void *)addr, &mod_tree, &mod_tree_ops); +if (!ltn) +return NULL ; - return ltn ? ltn->priv : NULL; + return container_of(ltn, struct mod_tree_node, node)->mod; }#else /* !(PERF_EVENTS || TRACING) */
Peter Zijlstra
maintainerRe: [PATCH v4 6/9] rbtree: Implement generic latch_tree
include/linux/module.h enum module_state { · line 219
Quoted 6 lines · Peter Zijlstra
include/linux/module.h +6
-0+struct module ; + +struct mod_tree_node { +struct latch_tree_node node; +struct module *mod; +};
The module pointer should be first in this structure.
Re: [PATCH v4 6/9] rbtree: Implement generic latch_tree
In reply to Peter Zijlstrainclude/linux/rbtree_latch.h struct latch_tree_ops { · line 71
Quoted 10 lines · Peter Zijlstra
struct
latch_tree_node{include/linux/rbtree_latch.h +6
-0+struct rb_node node[2 ]; }; +static __always_inline struct latch_tree_node * +__lt_from_rb (struct rb_node *node,int idx) +{ + return container_of(node, struct latch_tree_node, node[idx]); +}
Ugh. That syntax of offset_of() worries me a bit, but some grepping shows that we already use this form of offset_of() in parts of the kernel, so I guess it's fine.
Even with that small "Ugh", I do have to admit to preferring this to having the back-pointer.
Linus
Peter Zijlstra
maintainerRe: [PATCH v4 6/9] rbtree: Implement generic latch_tree
Quoted 6 lines · Linus Torvalds
Quoted 5 lines · Peter Zijlstra
struct
latch_tree_node{
+ structrb_nodenode[2];
};
Quoted 9 lines
include/linux/rbtree_latch.h struct latch_tree_ops { · line 71
Quoted 5 lines
include/linux/rbtree_latch.h +5
-0+static __always_inline struct latch_tree_node * +__lt_from_rb (struct rb_node *node,int idx) +{ + return container_of(node, struct latch_tree_node, node[idx]); +}Ugh. That syntax of
offset_of()worries me a bit, but some grepping shows that we already use this form ofoffset_of()in parts of the kernel, so I guess it's fine.
I was a little surprised myself it worked, but its a constant after all so it 'should'.
Quoted 2 lines
Even with that small "Ugh", I do have to admit to preferring this to having the back-pointer.
Yeah me too, I'll respin the patches proper after I've given it some actual runtime too.
Re: [PATCH v4 6/9] rbtree: Implement generic latch_tree
Quoted 3 lines · Peter Zijlstra
I was a little surprised myself it worked, but its a constant after all so it 'should'.
Well, it should actually work for non-constants too, even when that 'idx' isn't inlined to one of the fixed constants. So even if this will ever hit the "seq & 1" case for lookup, it should all work. It's just an expression, after all.
All that is really required is that you can do '&(x->y)' on it, where 'x' is the type, and 'y' is the "member".
It's just unusual, which I think makes it slightly harder to read for a *human* just because it breaks the normal pattern of those things. But there's nothing technically wrong with it.
Linus
Re: [PATCH v4 6/9] rbtree: Implement generic latch_tree
Quoted 4 lines · Linus Torvalds
It's just unusual, which I think makes it slightly harder to read for a *human* just because it breaks the normal pattern of those things. But there's nothing technically wrong with it.
You could have written it as
static __always_inline struct latch_tree_node *
__lt_from_rb(struct rb_node *node, int idx)
{
return (void *)(node-idx);
}which is actually shorter, but even if that container_of() use looks a bit unusual, I think the container_of() shows more clearly what you actually want. Plus it would be more reliable if you ever end up adding any other fields to the struct latch_tree_node.
So I wouldn't actually suggest using that "(node-idx)" format. I think it should result in the exact same code, though.
Linus
Peter Zijlstra
maintainerPATCH v4 7/9 module: Optimize __module_address() using a latched RB-tree
In reply to Peter ZijlstraCurrently __module_address() is using a linear search through all modules in order to find the module corresponding to the provided address. With a lot of modules this can take a lot of time.
One of the users of this is kernel_text_address() which is employed in many stack unwinders; which in turn are used by perf-callchain and ftrace (possibly from NMI context).
So by optimizing __module_address() we optimize many stack unwinders which are used by both perf and tracing in performance sensitive code.
--- include/linux/module.h | 22 ++++++++-- kernel/module.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 121 insertions(+), 8 deletions(-)
include/linux/module.h +19 -3
@@ -17,6 +17,7 @@#include <linux/moduleparam.h> #include <linux/jump_label.h> #include <linux/export.h> +#include <linux/rbtree_latch.h> #include <linux/percpu.h> #include <asm/module.h>
@@ -269,8 +270,15 @@ struct module {/* Startup function. */ int (*init)(void ); -/* If this is non-NULL, vfree after init() returns */ -void *module_init; +/* +* If this is non-NULL, vfree after init() returns. +* +* Cacheline align here, such that: +* module_init, module_core, init_size, core_size, +* init_text_size, core_text_size and ltn_core.node[0] +* are on the same cacheline. +*/ +void *module_init____cacheline_aligned ;/* Here is the actual code + data, vfree'd on unload. */ void *module_core;
@@ -281,6 +289,14 @@ struct module {/* The size of the executable code in each section. */ unsigned int init_text_size, core_text_size; +/* +* We rely on the order of these two entries; not only do we want +* core.node[0] to be in the same cacheline as the above entries, +* we also assume ltn_init has a higher address than ltn_core. +*/ +struct latch_tree_nodes ltn_core; +struct latch_tree_nodes ltn_init; +/* Size of RO sections of the module (text+rodata) */ unsigned int init_ro_size, core_ro_size;
@@ -361,7 +377,7 @@ struct module {ctor_fn_t *ctors;unsigned int num_ctors;#endif -}; +}____cacheline_aligned ;#ifndef MODULE_ARCH_INIT #define MODULE_ARCH_INIT {}#endif
kernel/module.c +102 -5
145 lines not on this page · raw message
Peter Zijlstra
maintainerPATCH v4 8/9 module: Make the mod_tree stuff conditional on PERF_EVENTS || TRACING
In reply to Peter ZijlstraAndrew worried about the overhead on small systems; only use the fancy code when either perf or tracing is enabled.
--- kernel/module.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
kernel/module.c +28 -2
@@ -102,6 +102,8 @@ DEFINE_MUTEX(module_mutex);EXPORT_SYMBOL_GPL (module_mutex);static LIST_HEAD (modules); +#if defined(CONFIG_PERF_EVENTS ) || defined(CONFIG_TRACING ) +/* * Use a latched RB-tree for __module_address(); this allows us to use * RCU-sched lookups of the address from any context.
@@ -109,6 +111,10 @@ static LIST_HEAD(modules);* Because modules have two address ranges: init and core, we need two * latch_tree_nodes entries. We use the order they appear in struct module to * determine if we need to use the init or core values for the comparisons. +* +* 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)
@@ -185,7 +191,7 @@ static void mod_tree_remove(struct modulmod_tree_remove_init (mod); } -static struct module *mod_tree_find (unsigned long addr) +static struct module *mod_find (unsigned long addr) {struct latch_tree_node *ltn;
@@ -194,6 +200,26 @@ static struct module *mod_tree_find(unsi return ltn ? ltn->priv : NULL; } +#else /* !(PERF_EVENTS || TRACING) */ + +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) { +if (within_module (addr, mod)) +return mod; + } + +return NULL ; +} + +#endif /* !(PERF_EVENTS || TRACING) */ +#ifdef CONFIG_KGDB_KDB struct list_head *kdb_modules = &modules;/* kdb needs the list of modules */ #endif /* CONFIG_KGDB_KDB */
@@ -3939,7 +3965,7 @@ struct module *__module_address(unsignedmodule_assert_mutex_or_preempt (); - mod =mod_tree_find (addr); + mod =mod_find (addr);if (mod) {BUG_ON (!within_module (addr, mod));if (mod->state ==MODULE_STATE_UNFORMED )
Peter Zijlstra
maintainerPATCH v4 9/9 module: Use __module_address() for module_address_lookup()
In reply to Peter ZijlstraUse the generic __module_address() addr to struct module lookup instead of open coding it once more.
--- kernel/module.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
kernel/module.c +7 -10
@@ -3515,19 +3515,15 @@ const char *module_address_lookup(unsignchar **modname, char *namebuf) { - struct module *mod; const char *ret = NULL; +struct module *mod;preempt_disable (); -list_for_each_entry_rcu (mod, &modules, list) { -if (mod->state ==MODULE_STATE_UNFORMED ) -continue ; -if (within_module (addr, mod)) { -if (modname) - *modname = mod->name; - ret =get_ksymbol (mod, addr, size, offset); -break ; - } + mod =__module_address (addr); +if (mod) { +if (modname) + *modname = mod->name; + ret =get_ksymbol (mod, addr, size, offset); }/* Make a copy in here where it's safe */ if (ret) {
@@ -3535,6 +3531,7 @@ const char *module_address_lookup(unsign ret = namebuf; }preempt_enable (); +return ret; }
For the record I still think it's better to just look it up in the kernel page tables, which already maintain that information. This is a much simpler, much less risky patch. It works on all architectures that use page tables for the kernel, and have a X bit in them. The rest are likely obscure and won't have a lot of modules.
My patch implementing this for x86 is here:
http://lkml.iu.edu/hypermail/linux/kernel/1410.1/01879.html
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only.