diff mbox series

[07/54] mm/slub: use stackdepot to save stack trace in objects

Message ID 20210708010747.zIP9yxsci%akpm@linux-foundation.org (mailing list archive)
State New
Headers show
Series [01/54] lib/test: fix spelling mistakes | expand

Commit Message

Andrew Morton July 8, 2021, 1:07 a.m. UTC 
From: Oliver Glitta <glittao@gmail.com>
Subject: mm/slub: use stackdepot to save stack trace in objects

Many stack traces are similar so there are many similar arrays. 
Stackdepot saves each unique stack only once.

Replace field addrs in struct track with depot_stack_handle_t handle.  Use
stackdepot to save stack trace.

The benefits are smaller memory overhead and possibility to aggregate
per-cache statistics in the future using the stackdepot handle instead of
matching stacks manually.

[rdunlap@infradead.org: rename save_stack_trace()]
  Link: https://lkml.kernel.org/r/20210513051920.29320-1-rdunlap@infradead.org
[vbabka@suse.cz: fix lockdep splat]
  Link: https://lkml.kernel.org/r/20210516195150.26740-1-vbabka@suse.czLink: https://lkml.kernel.org/r/20210414163434.4376-1-glittao@gmail.com
Signed-off-by: Oliver Glitta <glittao@gmail.com>
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: David Rientjes <rientjes@google.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 init/Kconfig |    1 
 mm/slub.c    |   79 ++++++++++++++++++++++++++++++-------------------
 2 files changed, 50 insertions(+), 30 deletions(-)

Comments

Christoph Hellwig July 16, 2021, 7:39 a.m. UTC | #1 
This somewhat unexpectedly causes a crash when running the xfs/433 test
in xfstests for me.  Reverting the commit fixes the problem:

xfs/433 files ... [  138.422742] run fstests xfs/433 at 2021-07-16 07:30:42
[  140.128145] XFS (vdb): Mounting V5 Filesystem
[  140.160450] XFS (vdb): Ending clean mount
[  140.171782] xfs filesystem being mounted at /mnt/test supports timestamps un)
[  140.966560] XFS (vdc): Mounting V5 Filesystem
[  140.987911] XFS (vdc): Ending clean mount
[  141.000104] xfs filesystem being mounted at /mnt/scratch supports timestamps)
[  145.130156] XFS (vdc): Unmounting Filesystem
[  145.365230] XFS (vdc): Mounting V5 Filesystem
[  145.394542] XFS (vdc): Ending clean mount
[  145.409232] xfs filesystem being mounted at /mnt/scratch supports timestamps)
[  145.471384] XFS (vdc): Injecting error (false) at file fs/xfs/xfs_buf.c, lin"
[  145.478561] XFS (vdc): Injecting error (false) at file fs/xfs/xfs_buf.c, lin"
[  145.486070] XFS (vdc): Injecting error (false) at file fs/xfs/xfs_buf.c, lin"
[  145.492248] XFS (vdc): Injecting error (false) at file fs/xfs/xfs_buf.c, lin"
[  145.599964] XFS (vdb): Unmounting Filesystem
[  145.958340] BUG: kernel NULL pointer dereference, address: 0000000000000020
[  145.961760] #PF: supervisor read access in kernel mode
[  145.964278] #PF: error_code(0x0000) - not-present page
[  145.966758] PGD 0 P4D 0 
[  145.968041] Oops: 0000 [#1] PREEMPT SMP PTI
[  145.970077] CPU: 3 PID: 14172 Comm: xfs_scrub Not tainted 5.13.0+ #601
[  145.973243] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.144
[  145.977312] RIP: 0010:xfs_inode_hasattr+0x19/0x30
[  145.979626] Code: 83 c6 05 b2 55 75 02 01 e8 39 40 e4 00 eb b6 66 90 31 c0 80
[  145.989446] RSP: 0018:ffffc900070eba08 EFLAGS: 00010206
[  145.992280] RAX: ffffffff00ff0000 RBX: 0000000000000000 RCX: 0000000000000001
[  145.995970] RDX: 0000000000000000 RSI: ffffffff82fdd33f RDI: ffff88810dbe16c0
[  145.999945] RBP: ffff88810dbe16c0 R08: ffff888110e14348 R09: ffff888110e14348
[  146.003932] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
[  146.007854] R13: ffff888110d99000 R14: ffff888110d99000 R15: ffffffff834acd60
[  146.011765] FS:  00007f2bf29d7700(0000) GS:ffff88813bd80000(0000) knlGS:00000
[  146.016127] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  146.019297] CR2: 0000000000000020 CR3: 0000000110c96000 CR4: 00000000000006e0
[  146.023315] Call Trace:
[  146.024726]  xfs_attr_inactive+0x152/0x350
[  146.027059]  xfs_inactive+0x18a/0x240
[  146.029141]  xfs_fs_destroy_inode+0xcc/0x2d0
[  146.031311]  destroy_inode+0x36/0x70
[  146.033130]  xfs_bulkstat_one_int+0x243/0x340
[  146.035342]  xfs_bulkstat_iwalk+0x19/0x30
[  146.037562]  xfs_iwalk_ag_recs+0xef/0x1e0
[  146.039845]  xfs_iwalk_run_callbacks+0x9f/0x140
[  146.042550]  xfs_iwalk_ag+0x230/0x2f0
[  146.044601]  xfs_iwalk+0x139/0x200
[  146.046505]  ? xfs_bulkstat_one_int+0x340/0x340
[  146.049151]  xfs_bulkstat+0xc4/0x130
[  146.050771]  ? xfs_flags2diflags+0xe0/0xe0
[  146.052309]  xfs_ioc_bulkstat.constprop.0.isra.0+0xbf/0x120
[  146.054200]  xfs_file_ioctl+0xb6/0xef0
[  146.055474]  ? lock_is_held_type+0xd5/0x130
[  146.056867]  ? find_held_lock+0x2b/0x80
[  146.058241]  ? lock_release+0x13c/0x2e0
[  146.059385]  ? lock_is_held_type+0xd5/0x130
[  146.060435]  ? __fget_files+0xce/0x1d0
[  146.061385]  __x64_sys_ioctl+0x7e/0xb0
[  146.062333]  do_syscall_64+0x3b/0x90
[  146.063284]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  146.064572] RIP: 0033:0x7f2bf2df5427
[  146.065600] Code: 00 00 90 48 8b 05 69 aa 0c 00 64 c7 00 26 00 00 00 48 c7 c8
[  146.070244] RSP: 002b:00007f2bf29d6bd8 EFLAGS: 00000246 ORIG_RAX: 00000000000
[  146.072015] RAX: ffffffffffffffda RBX: 00007fffe44b8010 RCX: 00007f2bf2df5427
[  146.073692] RDX: 00007f2be4000b20 RSI: 000000008040587f RDI: 0000000000000003
[  146.075322] RBP: 00007f2be4000b20 R08: 00007f2be4003b70 R09: 0000000000000077
[  146.076962] R10: 0000000000000001 R11: 0000000000000246 R12: 00007f2be4003b70
[  146.078480] R13: 00007fffe44b8010 R14: 00007f2be4000b60 R15: 0000000000000018
[  146.079803] Modules linked in:
[  146.080379] CR2: 0000000000000020
[  146.081196] ---[ end trace 80a6ea90b0ea2a03 ]---
[  146.082130] RIP: 0010:xfs_inode_hasattr+0x19/0x30
[  146.083144] Code: 83 c6 05 b2 55 75 02 01 e8 39 40 e4 00 eb b6 66 90 31 c0 80
[  146.086831] RSP: 0018:ffffc900070eba08 EFLAGS: 00010206
[  146.087816] RAX: ffffffff00ff0000 RBX: 0000000000000000 RCX: 0000000000000001
[  146.089122] RDX: 0000000000000000 RSI: ffffffff82fdd33f RDI: ffff88810dbe16c0
[  146.090477] RBP: ffff88810dbe16c0 R08: ffff888110e14348 R09: ffff888110e14348
[  146.091794] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
[  146.093096] R13: ffff888110d99000 R14: ffff888110d99000 R15: ffffffff834acd60
[  146.094429] FS:  00007f2bf29d7700(0000) GS:ffff88813bd80000(0000) knlGS:00000
[  146.096002] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  146.097079] CR2: 0000000000000020 CR3: 0000000110c96000 CR4: 00000000000006e0
[  146.098479] Kernel panic - not syncing: Fatal exception
[  146.099677] Kernel Offset: disabled
[  146.100397] ---[ end Kernel panic - not syncing: Fatal exception ]---
Vlastimil Babka July 16, 2021, 8:57 a.m. UTC | #2 
On 7/16/21 9:39 AM, Christoph Hellwig wrote:
> This somewhat unexpectedly causes a crash when running the xfs/433 test
> in xfstests for me.  Reverting the commit fixes the problem:

That's weird, the backtrace doesn't even include SLUB/stackdepot code.
Is that kernel actually booted with slub_debug option/built with
CONFIG_SLUB_DEBUG_ON or some cache created with SLAB_STORE_USER ?

> 
> xfs/433 files ... [  138.422742] run fstests xfs/433 at 2021-07-16 07:30:42
> [  140.128145] XFS (vdb): Mounting V5 Filesystem
> [  140.160450] XFS (vdb): Ending clean mount
> [  140.171782] xfs filesystem being mounted at /mnt/test supports timestamps un)
> [  140.966560] XFS (vdc): Mounting V5 Filesystem
> [  140.987911] XFS (vdc): Ending clean mount
> [  141.000104] xfs filesystem being mounted at /mnt/scratch supports timestamps)
> [  145.130156] XFS (vdc): Unmounting Filesystem
> [  145.365230] XFS (vdc): Mounting V5 Filesystem
> [  145.394542] XFS (vdc): Ending clean mount
> [  145.409232] xfs filesystem being mounted at /mnt/scratch supports timestamps)
> [  145.471384] XFS (vdc): Injecting error (false) at file fs/xfs/xfs_buf.c, lin"
> [  145.478561] XFS (vdc): Injecting error (false) at file fs/xfs/xfs_buf.c, lin"
> [  145.486070] XFS (vdc): Injecting error (false) at file fs/xfs/xfs_buf.c, lin"
> [  145.492248] XFS (vdc): Injecting error (false) at file fs/xfs/xfs_buf.c, lin"
> [  145.599964] XFS (vdb): Unmounting Filesystem
> [  145.958340] BUG: kernel NULL pointer dereference, address: 0000000000000020
> [  145.961760] #PF: supervisor read access in kernel mode
> [  145.964278] #PF: error_code(0x0000) - not-present page
> [  145.966758] PGD 0 P4D 0 
> [  145.968041] Oops: 0000 [#1] PREEMPT SMP PTI
> [  145.970077] CPU: 3 PID: 14172 Comm: xfs_scrub Not tainted 5.13.0+ #601
> [  145.973243] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.144
> [  145.977312] RIP: 0010:xfs_inode_hasattr+0x19/0x30
> [  145.979626] Code: 83 c6 05 b2 55 75 02 01 e8 39 40 e4 00 eb b6 66 90 31 c0 80
> [  145.989446] RSP: 0018:ffffc900070eba08 EFLAGS: 00010206
> [  145.992280] RAX: ffffffff00ff0000 RBX: 0000000000000000 RCX: 0000000000000001
> [  145.995970] RDX: 0000000000000000 RSI: ffffffff82fdd33f RDI: ffff88810dbe16c0
> [  145.999945] RBP: ffff88810dbe16c0 R08: ffff888110e14348 R09: ffff888110e14348
> [  146.003932] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
> [  146.007854] R13: ffff888110d99000 R14: ffff888110d99000 R15: ffffffff834acd60
> [  146.011765] FS:  00007f2bf29d7700(0000) GS:ffff88813bd80000(0000) knlGS:00000
> [  146.016127] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  146.019297] CR2: 0000000000000020 CR3: 0000000110c96000 CR4: 00000000000006e0
> [  146.023315] Call Trace:
> [  146.024726]  xfs_attr_inactive+0x152/0x350
> [  146.027059]  xfs_inactive+0x18a/0x240
> [  146.029141]  xfs_fs_destroy_inode+0xcc/0x2d0
> [  146.031311]  destroy_inode+0x36/0x70
> [  146.033130]  xfs_bulkstat_one_int+0x243/0x340
> [  146.035342]  xfs_bulkstat_iwalk+0x19/0x30
> [  146.037562]  xfs_iwalk_ag_recs+0xef/0x1e0
> [  146.039845]  xfs_iwalk_run_callbacks+0x9f/0x140
> [  146.042550]  xfs_iwalk_ag+0x230/0x2f0
> [  146.044601]  xfs_iwalk+0x139/0x200
> [  146.046505]  ? xfs_bulkstat_one_int+0x340/0x340
> [  146.049151]  xfs_bulkstat+0xc4/0x130
> [  146.050771]  ? xfs_flags2diflags+0xe0/0xe0
> [  146.052309]  xfs_ioc_bulkstat.constprop.0.isra.0+0xbf/0x120
> [  146.054200]  xfs_file_ioctl+0xb6/0xef0
> [  146.055474]  ? lock_is_held_type+0xd5/0x130
> [  146.056867]  ? find_held_lock+0x2b/0x80
> [  146.058241]  ? lock_release+0x13c/0x2e0
> [  146.059385]  ? lock_is_held_type+0xd5/0x130
> [  146.060435]  ? __fget_files+0xce/0x1d0
> [  146.061385]  __x64_sys_ioctl+0x7e/0xb0
> [  146.062333]  do_syscall_64+0x3b/0x90
> [  146.063284]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> [  146.064572] RIP: 0033:0x7f2bf2df5427
> [  146.065600] Code: 00 00 90 48 8b 05 69 aa 0c 00 64 c7 00 26 00 00 00 48 c7 c8
> [  146.070244] RSP: 002b:00007f2bf29d6bd8 EFLAGS: 00000246 ORIG_RAX: 00000000000
> [  146.072015] RAX: ffffffffffffffda RBX: 00007fffe44b8010 RCX: 00007f2bf2df5427
> [  146.073692] RDX: 00007f2be4000b20 RSI: 000000008040587f RDI: 0000000000000003
> [  146.075322] RBP: 00007f2be4000b20 R08: 00007f2be4003b70 R09: 0000000000000077
> [  146.076962] R10: 0000000000000001 R11: 0000000000000246 R12: 00007f2be4003b70
> [  146.078480] R13: 00007fffe44b8010 R14: 00007f2be4000b60 R15: 0000000000000018
> [  146.079803] Modules linked in:
> [  146.080379] CR2: 0000000000000020
> [  146.081196] ---[ end trace 80a6ea90b0ea2a03 ]---
> [  146.082130] RIP: 0010:xfs_inode_hasattr+0x19/0x30
> [  146.083144] Code: 83 c6 05 b2 55 75 02 01 e8 39 40 e4 00 eb b6 66 90 31 c0 80
> [  146.086831] RSP: 0018:ffffc900070eba08 EFLAGS: 00010206
> [  146.087816] RAX: ffffffff00ff0000 RBX: 0000000000000000 RCX: 0000000000000001
> [  146.089122] RDX: 0000000000000000 RSI: ffffffff82fdd33f RDI: ffff88810dbe16c0
> [  146.090477] RBP: ffff88810dbe16c0 R08: ffff888110e14348 R09: ffff888110e14348
> [  146.091794] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
> [  146.093096] R13: ffff888110d99000 R14: ffff888110d99000 R15: ffffffff834acd60
> [  146.094429] FS:  00007f2bf29d7700(0000) GS:ffff88813bd80000(0000) knlGS:00000
> [  146.096002] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  146.097079] CR2: 0000000000000020 CR3: 0000000110c96000 CR4: 00000000000006e0
> [  146.098479] Kernel panic - not syncing: Fatal exception
> [  146.099677] Kernel Offset: disabled
> [  146.100397] ---[ end Kernel panic - not syncing: Fatal exception ]---
> 
>
Christoph Hellwig July 16, 2021, 9:12 a.m. UTC | #3 
On Fri, Jul 16, 2021 at 10:57:51AM +0200, Vlastimil Babka wrote:
> On 7/16/21 9:39 AM, Christoph Hellwig wrote:
> > This somewhat unexpectedly causes a crash when running the xfs/433 test
> > in xfstests for me.  Reverting the commit fixes the problem:
> 
> That's weird, the backtrace doesn't even include SLUB/stackdepot code.
> Is that kernel actually booted with slub_debug option/built with
> CONFIG_SLUB_DEBUG_ON or some cache created with SLAB_STORE_USER ?

CONFIG_SLUB_DEBUG_ON is enabled, yes.  Full .config attached.
Linus Torvalds July 16, 2021, 8:12 p.m. UTC | #4 
On Fri, Jul 16, 2021 at 12:39 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> This somewhat unexpectedly causes a crash when running the xfs/433 test
> in xfstests for me.  Reverting the commit fixes the problem:

I don't see why that would be the case, but I'm inclined to revert
that commit for another reason: the code doesn't seem to match the
description of the commit.

It used to be that CONFIG_SLUB_DEBUG was a config option that was
harmless and that defaulted to 'y' because there was little downside.
In fact, it's not just "default y", it doesn't even *ask* the user
unless CONFIG_EXPERT is on. Because it was fairly harmless. And then
SLOB_DEBUG_ON was that "do you actually want this code _enabled_".

But now it basically force-enables that STACKDEPOT support too, and
then instead of having an _optional_ CONFIG_STACKTRACE, you basically
have that as being forced on you whether you want active debugging or
not.

Maybe that

        select STACKDEPOT if STACKTRACE_SUPPORT

should have been

        select STACKDEPOT if STACKTRACE

because i\t used to be that CONFIG_STACKTRACE was somewhat unusual,
and only enabled for special debug cases (admittedly "CONFIG_TRACING"
likely meant that it was fairly widely enabled).

In contrast, STACKTRACE_SUPPORT is basically "this architecture supports it".

So now it seems STACKDEPOT is enabled basically unconditionally.

So I really don't see why it would cause that xfs issue, but I think
there are multiple reasons to just go "Hmm" on that commit.

Comments?

                Linus
Vlastimil Babka July 16, 2021, 10:37 p.m. UTC | #5 
On 7/16/21 10:12 PM, Linus Torvalds wrote:
> On Fri, Jul 16, 2021 at 12:39 AM Christoph Hellwig <hch@infradead.org> wrote:
>>
>> This somewhat unexpectedly causes a crash when running the xfs/433 test
>> in xfstests for me.  Reverting the commit fixes the problem:
> 
> I don't see why that would be the case, but I'm inclined to revert
> that commit for another reason: the code doesn't seem to match the
> description of the commit.
> 
> It used to be that CONFIG_SLUB_DEBUG was a config option that was
> harmless and that defaulted to 'y' because there was little downside.
> In fact, it's not just "default y", it doesn't even *ask* the user
> unless CONFIG_EXPERT is on. Because it was fairly harmless. And then
> SLOB_DEBUG_ON was that "do you actually want this code _enabled_".
> 
> But now it basically force-enables that STACKDEPOT support too, and
> then instead of having an _optional_ CONFIG_STACKTRACE, you basically
> have that as being forced on you whether you want active debugging or
> not.
> 
> Maybe that
> 
>         select STACKDEPOT if STACKTRACE_SUPPORT
> 
> should have been
> 
>         select STACKDEPOT if STACKTRACE

I recall we tried that and run into KConfig recursive dependency hell as
"config STACKDEPOT" does "select STACKTRACE", and after some attempts
ended up with the above.

> because i\t used to be that CONFIG_STACKTRACE was somewhat unusual,
> and only enabled for special debug cases (admittedly "CONFIG_TRACING"
> likely meant that it was fairly widely enabled).
> 
> In contrast, STACKTRACE_SUPPORT is basically "this architecture supports it".
> 
> So now it seems STACKDEPOT is enabled basically unconditionally.

It seemed rather harmless as it was just a bit of extra code. But it's
true Geert reports [1] unexpected memory usage which I would have only
expected if actual stacks started to be collected. So I guess we'll have
to look into that.

[1]
https://lore.kernel.org/lkml/CAMuHMdW=eoVzM1Re5FVoEN87nKfiLmM2+Ah7eNu2KXEhCvbZyA@mail.gmail.com/

> So I really don't see why it would cause that xfs issue, but I think
> there are multiple reasons to just go "Hmm" on that commit.
> 
> Comments?
> 
>                 Linus
>
Randy Dunlap July 17, 2021, 5:34 p.m. UTC | #6 
On 7/16/21 3:37 PM, Vlastimil Babka wrote:
> On 7/16/21 10:12 PM, Linus Torvalds wrote:
>> On Fri, Jul 16, 2021 at 12:39 AM Christoph Hellwig <hch@infradead.org> wrote:
>>>
>>> This somewhat unexpectedly causes a crash when running the xfs/433 test
>>> in xfstests for me.  Reverting the commit fixes the problem:
>>
>> I don't see why that would be the case, but I'm inclined to revert
>> that commit for another reason: the code doesn't seem to match the
>> description of the commit.
>>
>> It used to be that CONFIG_SLUB_DEBUG was a config option that was
>> harmless and that defaulted to 'y' because there was little downside.
>> In fact, it's not just "default y", it doesn't even *ask* the user
>> unless CONFIG_EXPERT is on. Because it was fairly harmless. And then
>> SLOB_DEBUG_ON was that "do you actually want this code _enabled_".
>>
>> But now it basically force-enables that STACKDEPOT support too, and
>> then instead of having an _optional_ CONFIG_STACKTRACE, you basically
>> have that as being forced on you whether you want active debugging or
>> not.
>>
>> Maybe that
>>
>>         select STACKDEPOT if STACKTRACE_SUPPORT
>>
>> should have been
>>
>>         select STACKDEPOT if STACKTRACE
> 
> I recall we tried that and run into KConfig recursive dependency hell as
> "config STACKDEPOT" does "select STACKTRACE", and after some attempts
> ended up with the above.
> 
>> because i\t used to be that CONFIG_STACKTRACE was somewhat unusual,
>> and only enabled for special debug cases (admittedly "CONFIG_TRACING"
>> likely meant that it was fairly widely enabled).
>>
>> In contrast, STACKTRACE_SUPPORT is basically "this architecture supports it".
>>
>> So now it seems STACKDEPOT is enabled basically unconditionally.
> 
> It seemed rather harmless as it was just a bit of extra code. But it's
> true Geert reports [1] unexpected memory usage which I would have only
> expected if actual stacks started to be collected. So I guess we'll have
> to look into that.
> 
> [1]
> https://lore.kernel.org/lkml/CAMuHMdW=eoVzM1Re5FVoEN87nKfiLmM2+Ah7eNu2KXEhCvbZyA@mail.gmail.com/
> 
>> So I really don't see why it would cause that xfs issue, but I think
>> there are multiple reasons to just go "Hmm" on that commit.
>>
>> Comments?
>>
>>                 Linus
>>
> 

There is also the matter of lib/stackdepot.c build errors on ARCH=arc:

https://lore.kernel.org/lkml/202107150600.LkGNb4Vb-lkp@intel.com/
Vlastimil Babka July 18, 2021, 7:29 a.m. UTC | #7 
On 7/17/21 7:34 PM, Randy Dunlap wrote:
>>> because i\t used to be that CONFIG_STACKTRACE was somewhat unusual,
>>> and only enabled for special debug cases (admittedly "CONFIG_TRACING"
>>> likely meant that it was fairly widely enabled).
>>>
>>> In contrast, STACKTRACE_SUPPORT is basically "this architecture supports it".
>>>
>>> So now it seems STACKDEPOT is enabled basically unconditionally.
>>
>> It seemed rather harmless as it was just a bit of extra code. But it's
>> true Geert reports [1] unexpected memory usage which I would have only
>> expected if actual stacks started to be collected. So I guess we'll have
>> to look into that.
>>
>> [1]
>> https://lore.kernel.org/lkml/CAMuHMdW=eoVzM1Re5FVoEN87nKfiLmM2+Ah7eNu2KXEhCvbZyA@mail.gmail.com/
>>
>>> So I really don't see why it would cause that xfs issue, but I think
>>> there are multiple reasons to just go "Hmm" on that commit.
>>>
>>> Comments?
>>>
>>>                 Linus
>>>
>>
> 
> There is also the matter of lib/stackdepot.c build errors on ARCH=arc:
> 
> https://lore.kernel.org/lkml/202107150600.LkGNb4Vb-lkp@intel.com/

That's being fixed AFAIK?

https://lore.kernel.org/lkml/20210710145033.2804047-1-linux@roeck-us.net/

I'll try to come up with some KConfig flag set that will make it depend
on STRACKTRACE again without recursion issues.
Randy Dunlap July 18, 2021, 2:17 p.m. UTC | #8 
On 7/18/21 12:29 AM, Vlastimil Babka wrote:
> On 7/17/21 7:34 PM, Randy Dunlap wrote:
>>>> because i\t used to be that CONFIG_STACKTRACE was somewhat unusual,
>>>> and only enabled for special debug cases (admittedly "CONFIG_TRACING"
>>>> likely meant that it was fairly widely enabled).
>>>>
>>>> In contrast, STACKTRACE_SUPPORT is basically "this architecture supports it".
>>>>
>>>> So now it seems STACKDEPOT is enabled basically unconditionally.
>>>
>>> It seemed rather harmless as it was just a bit of extra code. But it's
>>> true Geert reports [1] unexpected memory usage which I would have only
>>> expected if actual stacks started to be collected. So I guess we'll have
>>> to look into that.
>>>
>>> [1]
>>> https://lore.kernel.org/lkml/CAMuHMdW=eoVzM1Re5FVoEN87nKfiLmM2+Ah7eNu2KXEhCvbZyA@mail.gmail.com/
>>>
>>>> So I really don't see why it would cause that xfs issue, but I think
>>>> there are multiple reasons to just go "Hmm" on that commit.
>>>>
>>>> Comments?
>>>>
>>>>                 Linus
>>>>
>>>
>>
>> There is also the matter of lib/stackdepot.c build errors on ARCH=arc:
>>
>> https://lore.kernel.org/lkml/202107150600.LkGNb4Vb-lkp@intel.com/
> 
> That's being fixed AFAIK?
> 
> https://lore.kernel.org/lkml/20210710145033.2804047-1-linux@roeck-us.net/

Ah, thanks.

> I'll try to come up with some KConfig flag set that will make it depend
> on STRACKTRACE again without recursion issues.
>
diff mbox series

Patch

--- a/init/Kconfig~mm-slub-use-stackdepot-to-save-stack-trace-in-objects
+++ a/init/Kconfig
@@ -1847,6 +1847,7 @@  config SLUB_DEBUG
 	default y
 	bool "Enable SLUB debugging support" if EXPERT
 	depends on SLUB && SYSFS
+	select STACKDEPOT if STACKTRACE_SUPPORT
 	help
 	  SLUB has extensive debug support features. Disabling these can
 	  result in significant savings in code size. This also disables
--- a/mm/slub.c~mm-slub-use-stackdepot-to-save-stack-trace-in-objects
+++ a/mm/slub.c
@@ -26,6 +26,7 @@ 
 #include <linux/cpuset.h>
 #include <linux/mempolicy.h>
 #include <linux/ctype.h>
+#include <linux/stackdepot.h>
 #include <linux/debugobjects.h>
 #include <linux/kallsyms.h>
 #include <linux/kfence.h>
@@ -220,8 +221,8 @@  static inline bool kmem_cache_has_cpu_pa
 #define TRACK_ADDRS_COUNT 16
 struct track {
 	unsigned long addr;	/* Called from address */
-#ifdef CONFIG_STACKTRACE
-	unsigned long addrs[TRACK_ADDRS_COUNT];	/* Called from address */
+#ifdef CONFIG_STACKDEPOT
+	depot_stack_handle_t handle;
 #endif
 	int cpu;		/* Was running on cpu */
 	int pid;		/* Pid context */
@@ -625,22 +626,27 @@  static struct track *get_track(struct km
 	return kasan_reset_tag(p + alloc);
 }
 
+#ifdef CONFIG_STACKDEPOT
+static depot_stack_handle_t save_stack_depot_trace(gfp_t flags)
+{
+	unsigned long entries[TRACK_ADDRS_COUNT];
+	depot_stack_handle_t handle;
+	unsigned int nr_entries;
+
+	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 4);
+	handle = stack_depot_save(entries, nr_entries, flags);
+	return handle;
+}
+#endif
+
 static void set_track(struct kmem_cache *s, void *object,
 			enum track_item alloc, unsigned long addr)
 {
 	struct track *p = get_track(s, object, alloc);
 
 	if (addr) {
-#ifdef CONFIG_STACKTRACE
-		unsigned int nr_entries;
-
-		metadata_access_enable();
-		nr_entries = stack_trace_save(kasan_reset_tag(p->addrs),
-					      TRACK_ADDRS_COUNT, 3);
-		metadata_access_disable();
-
-		if (nr_entries < TRACK_ADDRS_COUNT)
-			p->addrs[nr_entries] = 0;
+#ifdef CONFIG_STACKDEPOT
+		p->handle = save_stack_depot_trace(GFP_NOWAIT);
 #endif
 		p->addr = addr;
 		p->cpu = smp_processor_id();
@@ -667,14 +673,19 @@  static void print_track(const char *s, s
 
 	pr_err("%s in %pS age=%lu cpu=%u pid=%d\n",
 	       s, (void *)t->addr, pr_time - t->when, t->cpu, t->pid);
-#ifdef CONFIG_STACKTRACE
+#ifdef CONFIG_STACKDEPOT
 	{
-		int i;
-		for (i = 0; i < TRACK_ADDRS_COUNT; i++)
-			if (t->addrs[i])
-				pr_err("\t%pS\n", (void *)t->addrs[i]);
-			else
-				break;
+		depot_stack_handle_t handle;
+		unsigned long *entries;
+		unsigned int nr_entries;
+
+		handle = READ_ONCE(t->handle);
+		if (!handle) {
+			pr_err("object allocation/free stack trace missing\n");
+		} else {
+			nr_entries = stack_depot_fetch(handle, &entries);
+			stack_trace_print(entries, nr_entries, 0);
+		}
 	}
 #endif
 }
@@ -4048,18 +4059,26 @@  void kmem_obj_info(struct kmem_obj_info
 	objp = fixup_red_left(s, objp);
 	trackp = get_track(s, objp, TRACK_ALLOC);
 	kpp->kp_ret = (void *)trackp->addr;
-#ifdef CONFIG_STACKTRACE
-	for (i = 0; i < KS_ADDRS_COUNT && i < TRACK_ADDRS_COUNT; i++) {
-		kpp->kp_stack[i] = (void *)trackp->addrs[i];
-		if (!kpp->kp_stack[i])
-			break;
-	}
+#ifdef CONFIG_STACKDEPOT
+	{
+		depot_stack_handle_t handle;
+		unsigned long *entries;
+		unsigned int nr_entries;
 
-	trackp = get_track(s, objp, TRACK_FREE);
-	for (i = 0; i < KS_ADDRS_COUNT && i < TRACK_ADDRS_COUNT; i++) {
-		kpp->kp_free_stack[i] = (void *)trackp->addrs[i];
-		if (!kpp->kp_free_stack[i])
-			break;
+		handle = READ_ONCE(trackp->handle);
+		if (handle) {
+			nr_entries = stack_depot_fetch(handle, &entries);
+			for (i = 0; i < KS_ADDRS_COUNT && i < nr_entries; i++)
+				kpp->kp_stack[i] = (void *)entries[i];
+		}
+
+		trackp = get_track(s, objp, TRACK_FREE);
+		handle = READ_ONCE(trackp->handle);
+		if (handle) {
+			nr_entries = stack_depot_fetch(handle, &entries);
+			for (i = 0; i < KS_ADDRS_COUNT && i < nr_entries; i++)
+				kpp->kp_free_stack[i] = (void *)entries[i];
+		}
 	}
 #endif
 #endif