Re: [PATCH net-next] bpf: Optimize lpm trie delete
From: Daniel Mack <daniel@zonque.org>
Date: 2017-09-20 16:51:55
Hi Craig, Thanks, this looks much cleaner already :) On 09/20/2017 06:22 PM, Craig Gallek wrote:
quoted hunk ↗ jump to hunk
diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c index 9d58a576b2ae..b5a7d70ec8b5 100644 --- a/kernel/bpf/lpm_trie.c +++ b/kernel/bpf/lpm_trie.c@@ -397,7 +397,7 @@ static int trie_delete_elem(struct bpf_map *map, void *_key) struct lpm_trie_node __rcu **trim; struct lpm_trie_node *node; unsigned long irq_flags; - unsigned int next_bit; + unsigned int next_bit = 0;
This default assignment seems wrong, and I guess you only added it to squelch a compiler warning? [...]
+ /* If the node has one child, we may be able to collapse the tree
+ * while removing this node if the node's child is in the same
+ * 'next bit' slot as this node was in its parent or if the node
+ * itself is the root.
+ */
+ if (trim == &trie->root) {
+ next_bit = node->child[0] ? 0 : 1;
+ rcu_assign_pointer(trie->root, node->child[next_bit]);
+ kfree_rcu(node, rcu);I don't think you should treat this 'root' case special. Instead, move the 'next_bit' assignment outside of the condition ...
+ } else if (rcu_access_pointer(node->child[next_bit])) {
+ rcu_assign_pointer(*trim, node->child[next_bit]);
+ kfree_rcu(node, rcu);... and then this branch would handle the case just fine. Correct? Otherwise, looks good to me! Thanks, Daniel