Re: [PATCH 3/8] xarray: Explicitely set XA_FREE_MARK in __xa_cmpxchg()

[Jan Kara <jack@suse.cz>]

On Wed 05-02-20 14:45:12, Jason Gunthorpe wrote:
> On Tue, Feb 04, 2020 at 03:25:09PM +0100, Jan Kara wrote:
> > __xa_cmpxchg() relies on xas_store() to set XA_FREE_MARK when storing
> > NULL into xarray that has free tracking enabled. Make the setting of
> > XA_FREE_MARK explicit similarly as its clearing currently it.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> >  lib/xarray.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/xarray.c b/lib/xarray.c
> > index ae8b7070e82c..4e32497c51bd 100644
> > +++ b/lib/xarray.c
> > @@ -1477,8 +1477,12 @@ void *__xa_cmpxchg(struct xarray *xa, unsigned long index,
> >  		curr = xas_load(&xas);
> >  		if (curr == old) {
> >  			xas_store(&xas, entry);
> > -			if (xa_track_free(xa) && entry && !curr)
> > -				xas_clear_mark(&xas, XA_FREE_MARK);
> > +			if (xa_track_free(xa)) {
> > +				if (entry && !curr)
> > +					xas_clear_mark(&xas, XA_FREE_MARK);
> > +				else if (!entry && curr)
> > +					xas_set_mark(&xas, XA_FREE_MARK);
> > +			}
> 
> This feels like an optimization that should also happen for
> __xa_store, which has very similar code:
> 
> 		curr = xas_store(&xas, entry);
> 		if (xa_track_free(xa))
> 			xas_clear_mark(&xas, XA_FREE_MARK);
> 
> Something like
> 
>                 if (xa_track_free(xa) && entry && !curr)
> 			xas_clear_mark(&xas, XA_FREE_MARK);
> 
> ?

Yeah, entry != NULL is guaranteed for __xa_store() (see how it transforms
NULL to XA_ZERO_ENTRY a few lines above) but !curr is probably a good
condition to add to save some unnecessary clearing when overwriting
existing values. It is unrelated to this patch though, just a separate
optimization so I'll add that as a separate patch to the series. Thanks for
the idea.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR