Re: [PATCH 2/2] regmap: Add maple tree based register cache

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2955 bytes --]

On Sat, Mar 25, 2023 at 07:41:02PM -0400, Liam R. Howlett wrote:

> Without use of the locking, the maple tree will generate lockdep
> complaints.

> With the mas_* family of functions, you are responsible for locking.  On
> read you can take the rcu_read_lock(), on writes you need to take
> mas_lock(&mas).

> If you have a lock you already use, you can init the tree with the
> external lock flag and set the external lock like we do in the VMA code.

Oh, that's not ideal.  regmap does have an external lock but it's
already taken outside of the cache code, the cache code shouldn't
have to worry about locking.  This does mean the maple tree locks
should never be contended, but there'll still be some overhead.
OTOH we only have to be faster than accessing the underlying bus
so it's merely annoying.  I've added the locking locally.

> > +static int regcache_maple_drop(struct regmap *map, unsigned int min,
> > +			       unsigned int max)
> > +{

> Wait, if this is for destroying the entire tree:
> 	mas_lock(&mas);
> 	mas_for_each(mas, entry, max)
> 		kfree(entry);
> 	__mt_destroy(mt);
> 	mas_unlock(&mas);

It's not just for that - it's also used for selectively
discarding some entries in the cache, we just reuse the code when
freeing the regmap.  It may be worth using the above in the map
free case, though it's not exactly a hot path.

> If it's not for the entire tree, you can free a range then write a NULL
> over the entire range.  But be conscience of the maple state range after
> the iterator ends, you will need to mas_set_range(&mas, min, max) prior
> to the mas_store_gfp().

Will writing NULL leave tree entries in place that get iterated?
Though the code will need to become more complicated anyway if we
might need to split nodes that cover more than one register.

> > +static int regcache_maple_exit(struct regmap *map)
> > +{
> > +	struct maple_tree *mt = map->cache;
> > +
> > +	/* if we've already been called then just return */
> > +	if (!mt)
> > +		return 0;

> There's not a race here with multiple tasks in here at once, right?

No, everything's locked at the caller level.  It's just
simplifying some of the error handling paths (IIRC the actual
issue is freeing a cache that was never allocated).

> > +	mt_init(mt);
> > +

> The maple tree does support bulk loading (in the b-tree sense).  You can
> put the maple tree in this state and pre-allocate nodes in bulk.
> kernel/fork.c currently does this through the vma iterator interface.
> Note that after a bulk-load, you have to ensure you call mas_destroy()
> to free any unused nodes and to potentially cause a rebalance of the
> last node (this is automatic).  But, again, like you said in your
> comment; this is good for the first pass.

Right, I saw the reallocation stuff and was going to save it for
later after I'd arranged to store ranges of registers directly in
the tree rather than having a node per register since there'll be
some overlap there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]