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

[Mark Brown]

On Mon, Apr 03, 2023 at 11:45:08AM -0400, Liam R. Howlett wrote:
> * Mark Brown <broonie@xxxxxxxxxx> [230329 20:10]:

> > +	/* Any adjacent entries to extend/merge? */
> > +	mas_set_range(&mas, reg - 1, reg + 1);
> > +	index = reg;
> > +	last = reg;
> > +
> > +	lower = mas_find(&mas, reg - 1);

> If you just want to check the previous, you can use:
> mas_prev(&mas, reg - 1);
> This will try the previous entry without rewalking from the top of the
> tree and you don't need to mas_set_range() call.

I did give this a spin but it doesn't seem to be doing what I'd expect
for indexes 0 and 1, in a register map with index 0 if we attempt to
insert index 1 the mas_prev() doesn't find the existing entry for index
0 so we don't attempt to combine them.  It seems to do the right thing
for non-zero indexes, I'll have a poke at some point.

> > +	/*
> > +	 * This is safe because the regmap lock means the Maple lock
> > +	 * is redundant, but we need to take it due to lockdep asserts
> > +	 * in the maple tree code.
> > +	 */
> > +	mas_lock(&mas);

> > +	mas_set_range(&mas, index, last);
> > +	ret = mas_store_gfp(&mas, entry, GFP_KERNEL);

> You can avoid this walk as well by changing the order of the code
> before:

> mas_walk(&mas, reg);
> if entry... return
> mas_next(&mas, reg + 1);
> ...
> mas_prev(&mas, reg - 1);
> ...

> This should now be pointing at the location mas_store_gfp() expects:
> mas.last = last;
> ret = mas_store_gfp()

This appears to be triggering data corruption for me in the cache drop
test, again I'll have a poke at some point.  We seem to be getting
rubbish for the upper data block, though only in the drop test which
means it's possibly an interaction with how the tree is affected by
dropping the middle of a block.  Might well be something stupid I'm
doing either here or in the drop function.

Attachment: signature.asc
Description: PGP signature