Re: [PATCH] regcache: Store values more directly in maple trees

Geert Uytterhoeven
2024-10-29T14:26:56Z

Hi Mark,

On Thu, Sep 26, 2024 at 12:20 PM Mark Brown <broonie@kernel.org> wrote:
> Currently the regmap usage of the maple tree is a bit non-idomatic, rather
> than storing register values directly we allocate arrays of contiguous
> register values and store pointers to those arrays. This is all quite
> fiddly, especially around adding values (where we may need to combine a new
> allocation with adjacent ones) or dropping values (where we may have to
> keep parts of a contiguous block). One reason for doing this is that the
> maple tree wants to store pointers rather than integers, and in particular
> has some problems when we want to store 0 as a value.
>
> For 64 bit systems we can take advantage of the fact that regmap only
> supports 32 bit values and store values with an extra high bit set in the
> maple tree, avoiding the special cases with 0 and allowing us to save a
> layer of indirection. This approach was suggested by Liam Howlett.
>
> That doesn't help 32 bit systems though since we don't have any non-value bits there. For those we
> can keep the same code structure by switching to do a separate allocation
> for each value. The resulting data structure is not a thing of beauty but
> the code is much less complicated, and should be able to make better use of
> the slab allocator in cases where contiguous blocks of registers are not
> powers of 2.
>
> Let's implement these two approaches, using CONFIG_64BIT to choose between
> direct storage and allocating per-register storage. The end result is much
> simpler, making more direct usage of the maple tree API and the detailed
> optimisation work that goes into it's implementation. One indication of
> the simplifications is that even with having the two different allocation
> strategies we still have an overall negative diffstat.
>
> Signed-off-by: Mark Brown <broonie@kernel.org>

Thanks for your patch, which is now commit 49a85851b14cf663 ("regcache:
Store values more directly in maple trees") in regmap/for-next.

I started seeing intermittent memory corruption reports during s2idle
on Salvator-XS:

BUG: KFENCE: memory corruption in
regcache_maple_sync_block.constprop.0+0xcc/0x1c8

Corrupted memory at 0x00000000ba391541 [ ! . . . ] (in kfence-#12):
regcache_maple_sync_block.constprop.0+0xcc/0x1c8
regcache_maple_sync+0x140/0x1b0
regcache_sync+0xc8/0x208
vc5_resume+0x28/0x54
dpm_run_callback+0x5c/0xc8
device_resume+0xf8/0x1d4
async_resume+0x20/0x34
async_run_entry_fn+0x34/0xec
process_scheduled_works+0x2e8/0x4a8
worker_thread+0x150/0x1dc
kthread+0xe4/0xf4
ret_from_fork+0x10/0x20

kfence-#12: 0x000000003219df44-0x0000000074811d38, size=4, cache=kmalloc-8

allocated by task 1058 on cpu 6 at 694.672934s (0.104605s ago):
regcache_maple_sync_block.constprop.0+0x70/0x1c8
regcache_maple_sync+0x140/0x1b0
regcache_sync+0xc8/0x208
vc5_resume+0x28/0x54
dpm_run_callback+0x5c/0xc8
device_resume+0xf8/0x1d4
async_resume+0x20/0x34
async_run_entry_fn+0x34/0xec
process_scheduled_works+0x2e8/0x4a8
worker_thread+0x150/0x1dc
kthread+0xe4/0xf4
ret_from_fork+0x10/0x20

freed by task 1058 on cpu 6 at 694.673657s (0.164678s ago):
regcache_maple_sync_block.constprop.0+0xcc/0x1c8
regcache_maple_sync+0x140/0x1b0
regcache_sync+0xc8/0x208
vc5_resume+0x28/0x54
dpm_run_callback+0x5c/0xc8
device_resume+0xf8/0x1d4
async_resume+0x20/0x34
async_run_entry_fn+0x34/0xec
process_scheduled_works+0x2e8/0x4a8
worker_thread+0x150/0x1dc
kthread+0xe4/0xf4
ret_from_fork+0x10/0x20

CPU: 6 UID: 0 PID: 1058 Comm: kworker/u33:9 Tainted: G W
6.12.0-rc5-rcar3-05681-g29cac4e36a79 #148
Tainted: [W]=WARN
Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
Workqueue: async async_run_entry_fn

While I did not bisect the issue, this change looks suspicious to me...

> --- a/drivers/base/regmap/regcache-maple.c
> +++ b/drivers/base/regmap/regcache-maple.c
> @@ -13,26 +13,53 @@
>
> #include "internal.h"
>
> -static int regcache_maple_read(struct regmap *map,
> - unsigned int reg, unsigned int *value)
> +#if IS_ENABLED(CONFIG_64BIT)
> +/*
> + * On 64 bit systems uintptr_t will be 64 bit but unsigned long 32 bit
> + * so we can store the register values directly in the maple tree. We

... as this assumption is not true: "unsigned long" is 64-bit on
64-bit platforms (and 32-bit on 32-bit platforms).

> + * need to always set an out of bounds bit due to maple tree's
> + * handling of NULL.
> + */

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds