| Message ID | 20230425140955.3834476-3-Liam.Howlett@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Maple tree mas_{next,prev}_range() and cleanup | expand |
On Tue, Apr 25, 2023 at 10:09:23AM -0400, Liam R. Howlett wrote: >From: "Liam R. Howlett" <Liam.Howlett@Oracle.com> > >mas_parent_enum() is a simple wrapper for mte_parent_enum() which is >only called from that wrapper. Remove the wrapper and inline >mte_parent_enum() into mas_parent_enum(). > >At the same time, clean up the bit masking of the root pointer since it >cannot be set by the time the bit masking occurs. Change the check on >the root bit to a WARN_ON(), and fix the verification code to not >trigger the WARN_ON() before checking if the node is root. > >Reported-by: Wei Yang <richard.weiyang@gmail.com> >Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
在 2023/4/25 22:09, Liam R. Howlett 写道: > From: "Liam R. Howlett" <Liam.Howlett@Oracle.com> > > mas_parent_enum() is a simple wrapper for mte_parent_enum() which is > only called from that wrapper. Remove the wrapper and inline > mte_parent_enum() into mas_parent_enum(). > > At the same time, clean up the bit masking of the root pointer since it > cannot be set by the time the bit masking occurs. Change the check on > the root bit to a WARN_ON(), and fix the verification code to not > trigger the WARN_ON() before checking if the node is root. > > Reported-by: Wei Yang <richard.weiyang@gmail.com> > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com> > --- > lib/maple_tree.c | 28 +++++++++++----------------- > 1 file changed, 11 insertions(+), 17 deletions(-) > > diff --git a/lib/maple_tree.c b/lib/maple_tree.c > index 9cf4fca42310c..ac0245dd88dad 100644 > --- a/lib/maple_tree.c > +++ b/lib/maple_tree.c > @@ -428,25 +428,23 @@ static inline unsigned long mte_parent_slot_mask(unsigned long parent) > * mas_parent_enum() - Return the maple_type of the parent from the stored > * parent type. > * @mas: The maple state > - * @node: The maple_enode to extract the parent's enum > + * @enode: The maple_enode to extract the parent's enum > * Return: The node->parent maple_type > */ > static inline > -enum maple_type mte_parent_enum(struct maple_enode *p_enode, > - struct maple_tree *mt) > +enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode) Do you think it's better to rename this function to mas_parent_type()? The meaning of enum is not obvious and there is already a similar function mte_node_type(). > { > unsigned long p_type; > > - p_type = (unsigned long)p_enode; > - if (p_type & MAPLE_PARENT_ROOT) > - return 0; /* Validated in the caller. */ > + p_type = (unsigned long)mte_to_node(enode)->parent; > + if (WARN_ON(p_type & MAPLE_PARENT_ROOT)) > + return 0; > > p_type &= MAPLE_NODE_MASK; > - p_type = p_type & ~(MAPLE_PARENT_ROOT | mte_parent_slot_mask(p_type)); > - > + p_type &= ~mte_parent_slot_mask(p_type); > switch (p_type) { > case MAPLE_PARENT_RANGE64: /* or MAPLE_PARENT_ARANGE64 */ > - if (mt_is_alloc(mt)) > + if (mt_is_alloc(mas->tree)) > return maple_arange_64; > return maple_range_64; > } > @@ -454,12 +452,6 @@ enum maple_type mte_parent_enum(struct maple_enode *p_enode, > return 0; > } > > -static inline > -enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode) > -{ > - return mte_parent_enum(ma_enode_ptr(mte_to_node(enode)->parent), mas->tree); > -} > - > /* > * mte_set_parent() - Set the parent node and encode the slot > * @enode: The encoded maple node. > @@ -7008,14 +7000,16 @@ static void mas_validate_parent_slot(struct ma_state *mas) > { > struct maple_node *parent; > struct maple_enode *node; > - enum maple_type p_type = mas_parent_enum(mas, mas->node); > - unsigned char p_slot = mte_parent_slot(mas->node); > + enum maple_type p_type; > + unsigned char p_slot; > void __rcu **slots; > int i; > > if (mte_is_root(mas->node)) > return; > > + p_slot = mte_parent_slot(mas->node); > + p_type = mas_parent_enum(mas, mas->node); > parent = mte_parent(mas->node); > slots = ma_slots(parent, p_type); > MT_BUG_ON(mas->tree, mas_mn(mas) == parent);
* Peng Zhang <zhangpeng.00@bytedance.com> [230426 00:15]: > > 在 2023/4/25 22:09, Liam R. Howlett 写道: > > From: "Liam R. Howlett" <Liam.Howlett@Oracle.com> > > > > mas_parent_enum() is a simple wrapper for mte_parent_enum() which is > > only called from that wrapper. Remove the wrapper and inline > > mte_parent_enum() into mas_parent_enum(). > > > > At the same time, clean up the bit masking of the root pointer since it > > cannot be set by the time the bit masking occurs. Change the check on > > the root bit to a WARN_ON(), and fix the verification code to not > > trigger the WARN_ON() before checking if the node is root. > > > > Reported-by: Wei Yang <richard.weiyang@gmail.com> > > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com> > > --- > > lib/maple_tree.c | 28 +++++++++++----------------- > > 1 file changed, 11 insertions(+), 17 deletions(-) > > > > diff --git a/lib/maple_tree.c b/lib/maple_tree.c > > index 9cf4fca42310c..ac0245dd88dad 100644 > > --- a/lib/maple_tree.c > > +++ b/lib/maple_tree.c > > @@ -428,25 +428,23 @@ static inline unsigned long mte_parent_slot_mask(unsigned long parent) > > * mas_parent_enum() - Return the maple_type of the parent from the stored > > * parent type. > > * @mas: The maple state > > - * @node: The maple_enode to extract the parent's enum > > + * @enode: The maple_enode to extract the parent's enum > > * Return: The node->parent maple_type > > */ > > static inline > > -enum maple_type mte_parent_enum(struct maple_enode *p_enode, > > - struct maple_tree *mt) > > +enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode) > > Do you think it's better to rename this function to mas_parent_type()? > The meaning of enum is not obvious and there is already a similar > function mte_node_type(). Yes, thanks. I'll make that change in v2. > > > { > > unsigned long p_type; > > - p_type = (unsigned long)p_enode; > > - if (p_type & MAPLE_PARENT_ROOT) > > - return 0; /* Validated in the caller. */ > > + p_type = (unsigned long)mte_to_node(enode)->parent; > > + if (WARN_ON(p_type & MAPLE_PARENT_ROOT)) > > + return 0; > > p_type &= MAPLE_NODE_MASK; > > - p_type = p_type & ~(MAPLE_PARENT_ROOT | mte_parent_slot_mask(p_type)); > > - > > + p_type &= ~mte_parent_slot_mask(p_type); > > switch (p_type) { > > case MAPLE_PARENT_RANGE64: /* or MAPLE_PARENT_ARANGE64 */ > > - if (mt_is_alloc(mt)) > > + if (mt_is_alloc(mas->tree)) > > return maple_arange_64; > > return maple_range_64; > > } > > @@ -454,12 +452,6 @@ enum maple_type mte_parent_enum(struct maple_enode *p_enode, > > return 0; > > } > > -static inline > > -enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode) > > -{ > > - return mte_parent_enum(ma_enode_ptr(mte_to_node(enode)->parent), mas->tree); > > -} > > - > > /* > > * mte_set_parent() - Set the parent node and encode the slot > > * @enode: The encoded maple node. > > @@ -7008,14 +7000,16 @@ static void mas_validate_parent_slot(struct ma_state *mas) > > { > > struct maple_node *parent; > > struct maple_enode *node; > > - enum maple_type p_type = mas_parent_enum(mas, mas->node); > > - unsigned char p_slot = mte_parent_slot(mas->node); > > + enum maple_type p_type; > > + unsigned char p_slot; > > void __rcu **slots; > > int i; > > if (mte_is_root(mas->node)) > > return; > > + p_slot = mte_parent_slot(mas->node); > > + p_type = mas_parent_enum(mas, mas->node); > > parent = mte_parent(mas->node); > > slots = ma_slots(parent, p_type); > > MT_BUG_ON(mas->tree, mas_mn(mas) == parent);
diff --git a/lib/maple_tree.c b/lib/maple_tree.c index 9cf4fca42310c..ac0245dd88dad 100644 --- a/lib/maple_tree.c +++ b/lib/maple_tree.c @@ -428,25 +428,23 @@ static inline unsigned long mte_parent_slot_mask(unsigned long parent) * mas_parent_enum() - Return the maple_type of the parent from the stored * parent type. * @mas: The maple state - * @node: The maple_enode to extract the parent's enum + * @enode: The maple_enode to extract the parent's enum * Return: The node->parent maple_type */ static inline -enum maple_type mte_parent_enum(struct maple_enode *p_enode, - struct maple_tree *mt) +enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode) { unsigned long p_type; - p_type = (unsigned long)p_enode; - if (p_type & MAPLE_PARENT_ROOT) - return 0; /* Validated in the caller. */ + p_type = (unsigned long)mte_to_node(enode)->parent; + if (WARN_ON(p_type & MAPLE_PARENT_ROOT)) + return 0; p_type &= MAPLE_NODE_MASK; - p_type = p_type & ~(MAPLE_PARENT_ROOT | mte_parent_slot_mask(p_type)); - + p_type &= ~mte_parent_slot_mask(p_type); switch (p_type) { case MAPLE_PARENT_RANGE64: /* or MAPLE_PARENT_ARANGE64 */ - if (mt_is_alloc(mt)) + if (mt_is_alloc(mas->tree)) return maple_arange_64; return maple_range_64; } @@ -454,12 +452,6 @@ enum maple_type mte_parent_enum(struct maple_enode *p_enode, return 0; } -static inline -enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode) -{ - return mte_parent_enum(ma_enode_ptr(mte_to_node(enode)->parent), mas->tree); -} - /* * mte_set_parent() - Set the parent node and encode the slot * @enode: The encoded maple node. @@ -7008,14 +7000,16 @@ static void mas_validate_parent_slot(struct ma_state *mas) { struct maple_node *parent; struct maple_enode *node; - enum maple_type p_type = mas_parent_enum(mas, mas->node); - unsigned char p_slot = mte_parent_slot(mas->node); + enum maple_type p_type; + unsigned char p_slot; void __rcu **slots; int i; if (mte_is_root(mas->node)) return; + p_slot = mte_parent_slot(mas->node); + p_type = mas_parent_enum(mas, mas->node); parent = mte_parent(mas->node); slots = ma_slots(parent, p_type); MT_BUG_ON(mas->tree, mas_mn(mas) == parent);