Date: Sun, 16 Feb 2025 19:08:00 GMT From: Doug Moore <dougm@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: bba883df5e88 - main - pctrie: iter_remove check from panic to KASSERT Message-ID: <202502161908.51GJ806X033111@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by dougm: URL: https://cgit.FreeBSD.org/src/commit/?id=bba883df5e88d0fb1133b23c05db5501dd321ad8 commit bba883df5e88d0fb1133b23c05db5501dd321ad8 Author: Doug Moore <dougm@FreeBSD.org> AuthorDate: 2025-02-16 19:05:18 +0000 Commit: Doug Moore <dougm@FreeBSD.org> CommitDate: 2025-02-16 19:05:18 +0000 pctrie: iter_remove check from panic to KASSERT pctrie_iter_remove checks to see if the thing the iterator points to is actually there, and panics if it is not. This panic would likely indicate the same iterator had been used for removal twice, without advancing the iterator in-between. This test takes a bit of time, and as it indicates a programmer error rather than some external condition, it is better handled as a KASSERT. This means with KASSERTs disabled, a wee bit of time is saved. Reviewed by: alc, markj Differential Revision: https://reviews.freebsd.org/D49015 --- sys/kern/subr_pctrie.c | 41 +++++++++++++++++++---------------------- sys/sys/pctrie.h | 7 ++----- 2 files changed, 21 insertions(+), 27 deletions(-) diff --git a/sys/kern/subr_pctrie.c b/sys/kern/subr_pctrie.c index 16690c3521bf..51be005cccd7 100644 --- a/sys/kern/subr_pctrie.c +++ b/sys/kern/subr_pctrie.c @@ -826,26 +826,22 @@ pctrie_iter_jump_le(struct pctrie_iter *it, int64_t jump) } /* - * If 'child', a leaf and a child of 'parent', is not NULL and has key 'index', - * then remove it from the pctrie and return its value. If doing so produces an - * internal node with only one child, purge it from the pctrie and save it in - * *freenode for later disposal. + * Remove the non-NULL child identified by 'index' from the set of children of + * 'node'. If doing so causes 'node' to have only one child, purge it from the + * pctrie and save it in *freenode for later disposal. */ -static uint64_t * +static void pctrie_remove(struct pctrie *ptree, struct pctrie_node *node, uint64_t index, - struct pctrie_node *child, struct pctrie_node **freenode) + struct pctrie_node **freenode) { - uint64_t *m; + struct pctrie_node *child; int slot; *freenode = NULL; - m = pctrie_match_value(child, index); - if (m == NULL) - return (m); if (node == NULL) { pctrie_node_store(pctrie_root(ptree), PCTRIE_NULL, PCTRIE_LOCKED); - return (m); + return; } slot = pctrie_slot(node, index); KASSERT((node->pn_popmap & (1 << slot)) != 0, @@ -854,7 +850,7 @@ pctrie_remove(struct pctrie *ptree, struct pctrie_node *node, uint64_t index, node->pn_popmap ^= 1 << slot; pctrie_node_store(&node->pn_child[slot], PCTRIE_NULL, PCTRIE_LOCKED); if (!powerof2(node->pn_popmap)) - return (m); + return; KASSERT(node->pn_popmap != 0, ("%s: bad popmap all zeroes", __func__)); slot = ffs(node->pn_popmap) - 1; child = pctrie_node_load(&node->pn_child[slot], NULL, PCTRIE_LOCKED); @@ -866,7 +862,6 @@ pctrie_remove(struct pctrie *ptree, struct pctrie_node *node, uint64_t index, pctrie_setparent(child, node); pctrie_node_store(pctrie_child(ptree, node, index), child, PCTRIE_LOCKED); - return (m); } /* @@ -878,6 +873,7 @@ pctrie_remove_lookup(struct pctrie *ptree, uint64_t index, struct pctrie_node **freenode) { struct pctrie_node *child, *node; + uint64_t *m; int slot; node = NULL; @@ -888,25 +884,26 @@ pctrie_remove_lookup(struct pctrie *ptree, uint64_t index, child = pctrie_node_load(&node->pn_child[slot], NULL, PCTRIE_LOCKED); } - return (pctrie_remove(ptree, node, index, child, freenode)); + if ((m = pctrie_match_value(child, index)) != NULL) + pctrie_remove(ptree, node, index, freenode); + else + *freenode = NULL; + return (m); } /* * Remove from the trie the leaf last chosen by the iterator, and * adjust the path if it's last member is to be freed. */ -uint64_t * +void pctrie_iter_remove(struct pctrie_iter *it, struct pctrie_node **freenode) { - struct pctrie_node *child; - uint64_t *m; - - child = pctrie_node_load(pctrie_child(it->ptree, it->node, it->index), - NULL, PCTRIE_LOCKED); - m = pctrie_remove(it->ptree, it->node, it->index, child, freenode); + KASSERT(NULL != pctrie_match_value(pctrie_node_load(pctrie_child( + it->ptree, it->node, it->index), NULL, PCTRIE_LOCKED), it->index), + ("%s: removing value %lx not at iter", __func__, it->index)); + pctrie_remove(it->ptree, it->node, it->index, freenode); if (*freenode != NULL) it->node = pctrie_parent(it->node); - return (m); } /* diff --git a/sys/sys/pctrie.h b/sys/sys/pctrie.h index 196449e663d3..42473dc8e632 100644 --- a/sys/sys/pctrie.h +++ b/sys/sys/pctrie.h @@ -312,12 +312,9 @@ name##_PCTRIE_REMOVE_BASE(struct pctrie *ptree, \ static __inline __unused void \ name##_PCTRIE_ITER_REMOVE(struct pctrie_iter *it) \ { \ - uint64_t *val; \ struct pctrie_node *freenode; \ \ - val = pctrie_iter_remove(it, &freenode); \ - if (val == NULL) \ - panic("%s: key not found", __func__); \ + pctrie_iter_remove(it, &freenode); \ name##_PCTRIE_REMOVE_BASE(it->ptree, freenode); \ } \ \ @@ -386,7 +383,7 @@ struct pctrie_node *pctrie_reclaim_resume_cb(struct pctrie_node **pnode, pctrie_cb_t callback, int keyoff, void *arg); uint64_t *pctrie_remove_lookup(struct pctrie *ptree, uint64_t index, struct pctrie_node **killnode); -uint64_t *pctrie_iter_remove(struct pctrie_iter *it, +void pctrie_iter_remove(struct pctrie_iter *it, struct pctrie_node **freenode); uint64_t *pctrie_iter_value(struct pctrie_iter *it); uint64_t *pctrie_replace(struct pctrie *ptree, uint64_t *newval);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202502161908.51GJ806X033111>