Re: [PATCH v3 4/5] maple_tree: refine mas_store_root() on storing NULL

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Liam R. Howlett <Liam.Howlett@oracle.com> [241018 14:00]:
> * Liam R. Howlett <Liam.Howlett@oracle.com> [241018 13:57]:
> > * Wei Yang <richard.weiyang@gmail.com> [241017 22:40]:
> > > Currently, when storing NULL on mas_store_root(), the behavior could be
> > > improved.
> > > 
> > > For example possible cases are:
> > > 
> > >   * store NULL at any range result a new node
> > >   * store NULL at range [m, n] where m > 0 to a single entry tree result
> > >     a new node with range [m, n] set to NULL
> > >   * store NULL at range [m, n] where m > 0 to an empty tree result
> > >     consecutive NULL slot
> > > 
> > > This patch tries to improve in:
> > > 
> > >   * memory efficient by setting to empty tree instead of using a node
> > 
> > >   * remove the possibility of consecutive NULL slot which will prohibit
> > >     extended null in later operation
> > 
> > I don't understand this.  Do we actually store consecutive NULLs now?
> > 
> > This is a very odd change log for fixing an optimisation.  Maybe start
> > by explaining how we end up with a node with a single value now, then
> > state how this code changes that?
> > 
> > > 
> > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> > > CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
> > > CC: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> > > CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > > 
> > > ---
> > > v3: move change into mas_store_root()
> > > ---
> > >  lib/maple_tree.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> > > index db8b89487c98..03fbee9880eb 100644
> > > --- a/lib/maple_tree.c
> > > +++ b/lib/maple_tree.c
> > > @@ -3439,7 +3439,11 @@ static inline void mas_root_expand(struct ma_state *mas, void *entry)
> > >  
> > >  static inline void mas_store_root(struct ma_state *mas, void *entry)
> > >  {
> > > -	if (likely((mas->last != 0) || (mas->index != 0)))
> > > +	if (!entry) {
> > > +		void *contents = mas_root_locked(mas);
> > > +
> > > +		if (!mas->index && contents)
> > > +			rcu_assign_pointer(mas->tree->ma_root, NULL);
> > 
> > You are changing what used to handle any range that wasn't 0 to handle
> > storing NULL.
> > 
> > This seems really broken.

I understand now.  You don't need to get the contents though

if (!mas->index && mas_is_ptr(mas)) will work

But it's probably faster to just assign the NULL and not check anything.

> > 
> > > +	} else if (likely((mas->last != 0) || (mas->index != 0)))
> > 
> > Isn't this exactly what you have above in the if statement?
> 
> Oh, I see.  It's the same as the line you deleted above.
> 
> > 
> > >  		mas_root_expand(mas, entry);
> > >  	else if (((unsigned long) (entry) & 3) == 2)
> > >  		mas_root_expand(mas, entry);
> > > -- 
> > > 2.34.1
> > >