Message-ID: <CAMuHMdW6KvV+HQPY28ASeGJo_xejqRFFatmFD_2ST9R+Gt-d0Q@mail.gmail.com>
Date: Tue, 29 Oct 2024 15:26:41 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Mark Brown <broonie@...nel.org>
Cc: linux-kernel@...r.kernel.org, Liam Howlett <liam.howlett@...cle.com>
Subject: Re: [PATCH] regcache: Store values more directly in maple trees

Hi Mark,

On Thu, Sep 26, 2024 at 12:20 PM Mark Brown <broonie@...nel.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@...nel.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@...ux-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

Powered by blists - more mailing lists