linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
@ 2024-11-06 11:19 Mikulas Patocka
  2024-11-06 11:34 ` Vlastimil Babka
  0 siblings, 1 reply; 14+ messages in thread
From: Mikulas Patocka @ 2024-11-06 11:19 UTC (permalink / raw)
  To: Pedro Falcato, Vlastimil Babka, Christoph Lameter, Pekka Enberg,
	David Rientjes, Joonsoo Kim, Andrew Morton
  Cc: zkabelac, Roman Gushchin, Hyeonggon Yoo, linux-mm

Hi

The commit 4c39529663b93165953ecf9b1a9ea817358dcd06 ("slab: Warn on 
duplicate cache names when DEBUG_VM=y") is causing large number of 
warnings about "dm_bufio_buffer", "dm_bufio_buffer-%u" (and other) device 
mapper caches.

I'd like to ask - how to properly fix it?

We create a "dm_bufio_buffer" or "dm_bufio_buffer-%u" cache with every dm 
bufio client. It used to work (and the duplicate caches are merged), but 
now it warns.

Should I append a pointer to the dm_bufio structure to the slab cache name 
to make them different? Or is there any other preferred solution?

Note that it is not possible to pre-create the cache "dm_bufio_buffer-%u" 
in the module's init function, because the size of per-buffer auxiliary 
data is not known at this point.

Mikulas



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-06 11:19 slab warning: kmem_cache of name 'dm_bufio_buffer' already exists Mikulas Patocka
@ 2024-11-06 11:34 ` Vlastimil Babka
  2024-11-06 12:05   ` Mikulas Patocka
  2024-11-08 16:49   ` Yang Shi
  0 siblings, 2 replies; 14+ messages in thread
From: Vlastimil Babka @ 2024-11-06 11:34 UTC (permalink / raw)
  To: Mikulas Patocka, Pedro Falcato, Christoph Lameter, Pekka Enberg,
	David Rientjes, Joonsoo Kim, Andrew Morton
  Cc: zkabelac, Roman Gushchin, Hyeonggon Yoo, linux-mm

On 11/6/24 12:19, Mikulas Patocka wrote:
> Hi

Hi,

> The commit 4c39529663b93165953ecf9b1a9ea817358dcd06 ("slab: Warn on 
> duplicate cache names when DEBUG_VM=y") is causing large number of 
> warnings about "dm_bufio_buffer", "dm_bufio_buffer-%u" (and other) device 
> mapper caches.

Hmm wonder why nobody run into this before. We thought the code that would
cause the warning would be all fixed before introducing it, but we missed
some, sorry.

> I'd like to ask - how to properly fix it?
> 
> We create a "dm_bufio_buffer" or "dm_bufio_buffer-%u" cache with every dm 
> bufio client. It used to work (and the duplicate caches are merged), but 

Note the merging can be disabled so then it's really several caches with
exactly same name in /proc/slabinfo and inability to create their
sysfs/debugfs directories.

> now it warns.
> 
> Should I append a pointer to the dm_bufio structure to the slab cache name 
> to make them different? Or is there any other preferred solution?

Anything that uniquely identifies the client should be ok, but beware e.g.
device names that can have slashes, see commit a360f311f57a36 (also for the
simplest possible fix that is an incremented number).

> Note that it is not possible to pre-create the cache "dm_bufio_buffer-%u" 
> in the module's init function, because the size of per-buffer auxiliary 
> data is not known at this point.

Looks like some chose to solve this the harder way, see 4d784c042d164f

> Mikulas
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-06 11:34 ` Vlastimil Babka
@ 2024-11-06 12:05   ` Mikulas Patocka
  2024-11-06 16:28     ` Vlastimil Babka
  2024-11-08 16:49   ` Yang Shi
  1 sibling, 1 reply; 14+ messages in thread
From: Mikulas Patocka @ 2024-11-06 12:05 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Pedro Falcato, Christoph Lameter, Pekka Enberg, David Rientjes,
	Joonsoo Kim, Andrew Morton, zkabelac, Roman Gushchin,
	Hyeonggon Yoo, linux-mm



On Wed, 6 Nov 2024, Vlastimil Babka wrote:

> On 11/6/24 12:19, Mikulas Patocka wrote:
> > Hi
> 
> Hi,
> 
> > The commit 4c39529663b93165953ecf9b1a9ea817358dcd06 ("slab: Warn on 
> > duplicate cache names when DEBUG_VM=y") is causing large number of 
> > warnings about "dm_bufio_buffer", "dm_bufio_buffer-%u" (and other) device 
> > mapper caches.
> 
> Hmm wonder why nobody run into this before. We thought the code that would
> cause the warning would be all fixed before introducing it, but we missed
> some, sorry.
> 
> > I'd like to ask - how to properly fix it?
> > 
> > We create a "dm_bufio_buffer" or "dm_bufio_buffer-%u" cache with every dm 
> > bufio client. It used to work (and the duplicate caches are merged), but 
> 
> Note the merging can be disabled so then it's really several caches with
> exactly same name in /proc/slabinfo and inability to create their
> sysfs/debugfs directories.

Would it be sensible to allow merging caches with the same name and same 
attributes and only warn if there are caches with the same name and 
different attributes?

> > now it warns.
> > 
> > Should I append a pointer to the dm_bufio structure to the slab cache name 
> > to make them different? Or is there any other preferred solution?
> 
> Anything that uniquely identifies the client should be ok, but beware e.g.
> device names that can have slashes, see commit a360f311f57a36 (also for the
> simplest possible fix that is an incremented number).
> 
> > Note that it is not possible to pre-create the cache "dm_bufio_buffer-%u" 
> > in the module's init function, because the size of per-buffer auxiliary 
> > data is not known at this point.
> 
> Looks like some chose to solve this the harder way, see 4d784c042d164f

This seems too complicated.

The always increasing sequence number seems like a better soltion.

Mikulas



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-06 12:05   ` Mikulas Patocka
@ 2024-11-06 16:28     ` Vlastimil Babka
  2024-11-06 21:22       ` Mikulas Patocka
  0 siblings, 1 reply; 14+ messages in thread
From: Vlastimil Babka @ 2024-11-06 16:28 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Pedro Falcato, Christoph Lameter, Pekka Enberg, David Rientjes,
	Joonsoo Kim, Andrew Morton, zkabelac, Roman Gushchin,
	Hyeonggon Yoo, linux-mm

On 11/6/24 13:05, Mikulas Patocka wrote:
> 
> 
> On Wed, 6 Nov 2024, Vlastimil Babka wrote:
> 
>> On 11/6/24 12:19, Mikulas Patocka wrote:
>> > Hi
>> 
>> Hi,
>> 
>> > The commit 4c39529663b93165953ecf9b1a9ea817358dcd06 ("slab: Warn on 
>> > duplicate cache names when DEBUG_VM=y") is causing large number of 
>> > warnings about "dm_bufio_buffer", "dm_bufio_buffer-%u" (and other) device 
>> > mapper caches.
>> 
>> Hmm wonder why nobody run into this before. We thought the code that would
>> cause the warning would be all fixed before introducing it, but we missed
>> some, sorry.
>> 
>> > I'd like to ask - how to properly fix it?
>> > 
>> > We create a "dm_bufio_buffer" or "dm_bufio_buffer-%u" cache with every dm 
>> > bufio client. It used to work (and the duplicate caches are merged), but 
>> 
>> Note the merging can be disabled so then it's really several caches with
>> exactly same name in /proc/slabinfo and inability to create their
>> sysfs/debugfs directories.
> 
> Would it be sensible to allow merging caches with the same name and same 
> attributes and only warn if there are caches with the same name and 
> different attributes?

We might consider that.

BTW, what benefits do you get from creating own kmem caches instead of using
kmalloc()? If it's just alignment, if you round up the intended size to
power of two, there's implicit kmalloc alignment guarantee. AFAICS there's
some alignment for c->slab_cache in dm_bufio_client_create()

In case the allocations have odd sizes without any such alignment
(the case of c->slab_buffer?) separate size-specific caches can result in
better packing, but that should only matter if you expect many/long-lived
objects to be allocated.

>> > now it warns.
>> > 
>> > Should I append a pointer to the dm_bufio structure to the slab cache name 
>> > to make them different? Or is there any other preferred solution?
>> 
>> Anything that uniquely identifies the client should be ok, but beware e.g.
>> device names that can have slashes, see commit a360f311f57a36 (also for the
>> simplest possible fix that is an incremented number).
>> 
>> > Note that it is not possible to pre-create the cache "dm_bufio_buffer-%u" 
>> > in the module's init function, because the size of per-buffer auxiliary 
>> > data is not known at this point.
>> 
>> Looks like some chose to solve this the harder way, see 4d784c042d164f
> 
> This seems too complicated.
> 
> The always increasing sequence number seems like a better soltion.
> 
> Mikulas
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-06 16:28     ` Vlastimil Babka
@ 2024-11-06 21:22       ` Mikulas Patocka
  2024-11-08  9:56         ` Vlastimil Babka
  2024-11-08 10:08         ` Vlastimil Babka
  0 siblings, 2 replies; 14+ messages in thread
From: Mikulas Patocka @ 2024-11-06 21:22 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Pedro Falcato, Christoph Lameter, Pekka Enberg, David Rientjes,
	Joonsoo Kim, Andrew Morton, zkabelac, Roman Gushchin,
	Hyeonggon Yoo, linux-mm



On Wed, 6 Nov 2024, Vlastimil Babka wrote:

> On 11/6/24 13:05, Mikulas Patocka wrote:
> > 
> > 
> > On Wed, 6 Nov 2024, Vlastimil Babka wrote:
> > 
> >> On 11/6/24 12:19, Mikulas Patocka wrote:
> >> > Hi
> >> 
> >> Hi,
> >> 
> >> > The commit 4c39529663b93165953ecf9b1a9ea817358dcd06 ("slab: Warn on 
> >> > duplicate cache names when DEBUG_VM=y") is causing large number of 
> >> > warnings about "dm_bufio_buffer", "dm_bufio_buffer-%u" (and other) device 
> >> > mapper caches.
> >> 
> >> Hmm wonder why nobody run into this before. We thought the code that would
> >> cause the warning would be all fixed before introducing it, but we missed
> >> some, sorry.
> >> 
> >> > I'd like to ask - how to properly fix it?
> >> > 
> >> > We create a "dm_bufio_buffer" or "dm_bufio_buffer-%u" cache with every dm 
> >> > bufio client. It used to work (and the duplicate caches are merged), but 
> >> 
> >> Note the merging can be disabled so then it's really several caches with
> >> exactly same name in /proc/slabinfo and inability to create their
> >> sysfs/debugfs directories.
> > 
> > Would it be sensible to allow merging caches with the same name and same 
> > attributes and only warn if there are caches with the same name and 
> > different attributes?
> 
> We might consider that.

That would be good - so that users don't have to write their own slab 
cache merge logic.

> BTW, what benefits do you get from creating own kmem caches instead of using
> kmalloc()? If it's just alignment, if you round up the intended size to
> power of two, there's implicit kmalloc alignment guarantee.

See the function xfs_buf_alloc_kmem - it allocates a buffer using kmalloc, 
tests if the buffer crosses a page boundary, and if it does, the code 
falls back to xfs_buf_alloc_pages.

Do you think that it can be simplified to just allocate a buffer and NOT 
check for page crossing?

> AFAICS there's some alignment for c->slab_cache in 
> dm_bufio_client_create()

There are two slab caches - one for the dm_buffer structure and one for 
the buffer data (if the buffer size is less than a page).

> In case the allocations have odd sizes without any such alignment
> (the case of c->slab_buffer?) separate size-specific caches can result in
> better packing, but that should only matter if you expect many/long-lived
> objects to be allocated.

The cache for the dm_buffer structure is there so that it utilizes memory 
better. Yes - there may be a lot of long-lived dm_buffers in memory.

Mikulas



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-06 21:22       ` Mikulas Patocka
@ 2024-11-08  9:56         ` Vlastimil Babka
  2024-11-08 11:13           ` Hyeonggon Yoo
  2024-11-08 13:47           ` Pedro Falcato
  2024-11-08 10:08         ` Vlastimil Babka
  1 sibling, 2 replies; 14+ messages in thread
From: Vlastimil Babka @ 2024-11-08  9:56 UTC (permalink / raw)
  To: Mikulas Patocka
  Cc: Pedro Falcato, Christoph Lameter, Pekka Enberg, David Rientjes,
	Joonsoo Kim, Andrew Morton, zkabelac, Roman Gushchin,
	Hyeonggon Yoo, linux-mm

On 11/6/24 22:22, Mikulas Patocka wrote:
> 
> 
> On Wed, 6 Nov 2024, Vlastimil Babka wrote:
> 
>> On 11/6/24 13:05, Mikulas Patocka wrote:
>> > 
>> > 
>> > On Wed, 6 Nov 2024, Vlastimil Babka wrote:
>> > 
>> >> On 11/6/24 12:19, Mikulas Patocka wrote:
>> >> > Hi
>> >> 
>> >> Hi,
>> >> 
>> >> > The commit 4c39529663b93165953ecf9b1a9ea817358dcd06 ("slab: Warn on 
>> >> > duplicate cache names when DEBUG_VM=y") is causing large number of 
>> >> > warnings about "dm_bufio_buffer", "dm_bufio_buffer-%u" (and other) device 
>> >> > mapper caches.
>> >> 
>> >> Hmm wonder why nobody run into this before. We thought the code that would
>> >> cause the warning would be all fixed before introducing it, but we missed
>> >> some, sorry.
>> >> 
>> >> > I'd like to ask - how to properly fix it?
>> >> > 
>> >> > We create a "dm_bufio_buffer" or "dm_bufio_buffer-%u" cache with every dm 
>> >> > bufio client. It used to work (and the duplicate caches are merged), but 
>> >> 
>> >> Note the merging can be disabled so then it's really several caches with
>> >> exactly same name in /proc/slabinfo and inability to create their
>> >> sysfs/debugfs directories.
>> > 
>> > Would it be sensible to allow merging caches with the same name and same 
>> > attributes and only warn if there are caches with the same name and 
>> > different attributes?
>> 
>> We might consider that.
> 
> That would be good - so that users don't have to write their own slab 
> cache merge logic.

So Pedro, wanna do that? Or Hyeonggon maybe? :)



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-06 21:22       ` Mikulas Patocka
  2024-11-08  9:56         ` Vlastimil Babka
@ 2024-11-08 10:08         ` Vlastimil Babka
  2024-11-08 14:56           ` Christoph Hellwig
  1 sibling, 1 reply; 14+ messages in thread
From: Vlastimil Babka @ 2024-11-08 10:08 UTC (permalink / raw)
  To: Mikulas Patocka, Dave Chinner, Darrick J. Wong, linux-xfs,
	Christoph Hellwig
  Cc: Pedro Falcato, Christoph Lameter, Pekka Enberg, David Rientjes,
	Joonsoo Kim, Andrew Morton, zkabelac, Roman Gushchin,
	Hyeonggon Yoo, linux-mm

On 11/6/24 22:22, Mikulas Patocka wrote:
> 
>> BTW, what benefits do you get from creating own kmem caches instead of using
>> kmalloc()? If it's just alignment, if you round up the intended size to
>> power of two, there's implicit kmalloc alignment guarantee.
> 
> See the function xfs_buf_alloc_kmem - it allocates a buffer using kmalloc, 
> tests if the buffer crosses a page boundary, and if it does, the code 
> falls back to xfs_buf_alloc_pages.
> 
> Do you think that it can be simplified to just allocate a buffer and NOT 
> check for page crossing?

Right, IIRC xfs was one of the usecases that prompted us towards defining
the kmalloc alignment guarantees, which was around 2019.
So today, kmalloc() allocations will not cross a page boundary if the
requested size is lower than page size, and it's a power-of-two value. Even
if SLUB debugging is enabled (before the alignment became guaranteed, it
would happen naturally, and only be violated by either using SLOB, or
enabling SLUB debugging).
xfs_buf_alloc_kmem() could be thus simplified.

>> AFAICS there's some alignment for c->slab_cache in 
>> dm_bufio_client_create()
> 
> There are two slab caches - one for the dm_buffer structure and one for 
> the buffer data (if the buffer size is less than a page).
> 
>> In case the allocations have odd sizes without any such alignment
>> (the case of c->slab_buffer?) separate size-specific caches can result in
>> better packing, but that should only matter if you expect many/long-lived
>> objects to be allocated.
> 
> The cache for the dm_buffer structure is there so that it utilizes memory 
> better. Yes - there may be a lot of long-lived dm_buffers in memory.

OK.

> Mikulas
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-08  9:56         ` Vlastimil Babka
@ 2024-11-08 11:13           ` Hyeonggon Yoo
  2024-11-08 11:21             ` Vlastimil Babka
  2024-11-08 13:47           ` Pedro Falcato
  1 sibling, 1 reply; 14+ messages in thread
From: Hyeonggon Yoo @ 2024-11-08 11:13 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Mikulas Patocka, Pedro Falcato, Christoph Lameter, Pekka Enberg,
	David Rientjes, Joonsoo Kim, Andrew Morton, zkabelac,
	Roman Gushchin, linux-mm

On Fri, Nov 8, 2024 at 6:56 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 11/6/24 22:22, Mikulas Patocka wrote:
> >
> >
> > On Wed, 6 Nov 2024, Vlastimil Babka wrote:
> >
> >> On 11/6/24 13:05, Mikulas Patocka wrote:
> >> >
> >> >
> >> > On Wed, 6 Nov 2024, Vlastimil Babka wrote:
> >> >
> >> >> On 11/6/24 12:19, Mikulas Patocka wrote:
> >> >> > Hi
> >> >>
> >> >> Hi,
> >> >>
> >> >> > The commit 4c39529663b93165953ecf9b1a9ea817358dcd06 ("slab: Warn on
> >> >> > duplicate cache names when DEBUG_VM=y") is causing large number of
> >> >> > warnings about "dm_bufio_buffer", "dm_bufio_buffer-%u" (and other) device
> >> >> > mapper caches.
> >> >>
> >> >> Hmm wonder why nobody run into this before. We thought the code that would
> >> >> cause the warning would be all fixed before introducing it, but we missed
> >> >> some, sorry.
> >> >>
> >> >> > I'd like to ask - how to properly fix it?
> >> >> >
> >> >> > We create a "dm_bufio_buffer" or "dm_bufio_buffer-%u" cache with every dm
> >> >> > bufio client. It used to work (and the duplicate caches are merged), but
> >> >>
> >> >> Note the merging can be disabled so then it's really several caches with
> >> >> exactly same name in /proc/slabinfo and inability to create their
> >> >> sysfs/debugfs directories.
> >> >
> >> > Would it be sensible to allow merging caches with the same name and same
> >> > attributes and only warn if there are caches with the same name and
> >> > different attributes?
> >>
> >> We might consider that.
> >
> > That would be good - so that users don't have to write their own slab
> > cache merge logic.
>
> So Pedro, wanna do that? Or Hyeonggon maybe? :)

IIUC it would be a special slab merging logic if the names and
attributes match, even if slab merging is disabled. Right?


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-08 11:13           ` Hyeonggon Yoo
@ 2024-11-08 11:21             ` Vlastimil Babka
  0 siblings, 0 replies; 14+ messages in thread
From: Vlastimil Babka @ 2024-11-08 11:21 UTC (permalink / raw)
  To: Hyeonggon Yoo
  Cc: Mikulas Patocka, Pedro Falcato, Christoph Lameter, Pekka Enberg,
	David Rientjes, Joonsoo Kim, Andrew Morton, zkabelac,
	Roman Gushchin, linux-mm

On 11/8/24 12:13, Hyeonggon Yoo wrote:
> On Fri, Nov 8, 2024 at 6:56 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>>
>> On 11/6/24 22:22, Mikulas Patocka wrote:
>> >
>> >
>> > On Wed, 6 Nov 2024, Vlastimil Babka wrote:
>> >
>> >> On 11/6/24 13:05, Mikulas Patocka wrote:
>> >> >
>> >> >
>> >> > On Wed, 6 Nov 2024, Vlastimil Babka wrote:
>> >> >
>> >> >> On 11/6/24 12:19, Mikulas Patocka wrote:
>> >> >> > Hi
>> >> >>
>> >> >> Hi,
>> >> >>
>> >> >> > The commit 4c39529663b93165953ecf9b1a9ea817358dcd06 ("slab: Warn on
>> >> >> > duplicate cache names when DEBUG_VM=y") is causing large number of
>> >> >> > warnings about "dm_bufio_buffer", "dm_bufio_buffer-%u" (and other) device
>> >> >> > mapper caches.
>> >> >>
>> >> >> Hmm wonder why nobody run into this before. We thought the code that would
>> >> >> cause the warning would be all fixed before introducing it, but we missed
>> >> >> some, sorry.
>> >> >>
>> >> >> > I'd like to ask - how to properly fix it?
>> >> >> >
>> >> >> > We create a "dm_bufio_buffer" or "dm_bufio_buffer-%u" cache with every dm
>> >> >> > bufio client. It used to work (and the duplicate caches are merged), but
>> >> >>
>> >> >> Note the merging can be disabled so then it's really several caches with
>> >> >> exactly same name in /proc/slabinfo and inability to create their
>> >> >> sysfs/debugfs directories.
>> >> >
>> >> > Would it be sensible to allow merging caches with the same name and same
>> >> > attributes and only warn if there are caches with the same name and
>> >> > different attributes?
>> >>
>> >> We might consider that.
>> >
>> > That would be good - so that users don't have to write their own slab
>> > cache merge logic.
>>
>> So Pedro, wanna do that? Or Hyeonggon maybe? :)
> 
> IIUC it would be a special slab merging logic if the names and
> attributes match, even if slab merging is disabled. Right?

Yes, I'm thinking of a new SLAB_ flag to make this opt-in and not
accidental. It would need to override even the (debugging etc) flags or
cache-specific that prevent merging, in addition to the global setting.
Instead we'd need to check that everything is exactly the same - flags,
ctor, etc...


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-08  9:56         ` Vlastimil Babka
  2024-11-08 11:13           ` Hyeonggon Yoo
@ 2024-11-08 13:47           ` Pedro Falcato
  1 sibling, 0 replies; 14+ messages in thread
From: Pedro Falcato @ 2024-11-08 13:47 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Mikulas Patocka, Christoph Lameter, Pekka Enberg, David Rientjes,
	Joonsoo Kim, Andrew Morton, zkabelac, Roman Gushchin,
	Hyeonggon Yoo, linux-mm

On Fri, Nov 8, 2024 at 9:56 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 11/6/24 22:22, Mikulas Patocka wrote:
> >
> >
> > On Wed, 6 Nov 2024, Vlastimil Babka wrote:
> >
> >> On 11/6/24 13:05, Mikulas Patocka wrote:
> >> >
> >> >
> >> > On Wed, 6 Nov 2024, Vlastimil Babka wrote:
> >> >
> >> >> On 11/6/24 12:19, Mikulas Patocka wrote:
> >> >> > Hi
> >> >>
> >> >> Hi,
> >> >>
> >> >> > The commit 4c39529663b93165953ecf9b1a9ea817358dcd06 ("slab: Warn on
> >> >> > duplicate cache names when DEBUG_VM=y") is causing large number of
> >> >> > warnings about "dm_bufio_buffer", "dm_bufio_buffer-%u" (and other) device
> >> >> > mapper caches.
> >> >>
> >> >> Hmm wonder why nobody run into this before. We thought the code that would
> >> >> cause the warning would be all fixed before introducing it, but we missed
> >> >> some, sorry.
> >> >>
> >> >> > I'd like to ask - how to properly fix it?
> >> >> >
> >> >> > We create a "dm_bufio_buffer" or "dm_bufio_buffer-%u" cache with every dm
> >> >> > bufio client. It used to work (and the duplicate caches are merged), but
> >> >>
> >> >> Note the merging can be disabled so then it's really several caches with
> >> >> exactly same name in /proc/slabinfo and inability to create their
> >> >> sysfs/debugfs directories.
> >> >
> >> > Would it be sensible to allow merging caches with the same name and same
> >> > attributes and only warn if there are caches with the same name and
> >> > different attributes?
> >>
> >> We might consider that.
> >
> > That would be good - so that users don't have to write their own slab
> > cache merge logic.
>
> So Pedro, wanna do that? Or Hyeonggon maybe? :)
>

I'll take a look this weekend, now that our polish friend passed me
the smoking gun ;)

-- 
Pedro


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-08 10:08         ` Vlastimil Babka
@ 2024-11-08 14:56           ` Christoph Hellwig
  0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2024-11-08 14:56 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Mikulas Patocka, Dave Chinner, Darrick J. Wong, linux-xfs,
	Christoph Hellwig, Pedro Falcato, Christoph Lameter,
	Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
	zkabelac, Roman Gushchin, Hyeonggon Yoo, linux-mm

On Fri, Nov 08, 2024 at 11:08:40AM +0100, Vlastimil Babka wrote:
> Right, IIRC xfs was one of the usecases that prompted us towards defining
> the kmalloc alignment guarantees, which was around 2019.
> So today, kmalloc() allocations will not cross a page boundary if the
> requested size is lower than page size, and it's a power-of-two value. Even
> if SLUB debugging is enabled (before the alignment became guaranteed, it
> would happen naturally, and only be violated by either using SLOB, or
> enabling SLUB debugging).
> xfs_buf_alloc_kmem() could be thus simplified.

I'm not sure we'll want to fully trust future allocator changes, but
maybe switching the fallback logic to an assert or WARN_ON might be
wortwhile.



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-06 11:34 ` Vlastimil Babka
  2024-11-06 12:05   ` Mikulas Patocka
@ 2024-11-08 16:49   ` Yang Shi
  2024-11-08 17:00     ` Vlastimil Babka
  1 sibling, 1 reply; 14+ messages in thread
From: Yang Shi @ 2024-11-08 16:49 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Mikulas Patocka, Pedro Falcato, Christoph Lameter, Pekka Enberg,
	David Rientjes, Joonsoo Kim, Andrew Morton, zkabelac,
	Roman Gushchin, Hyeonggon Yoo, linux-mm

On Wed, Nov 6, 2024 at 3:35 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 11/6/24 12:19, Mikulas Patocka wrote:
> > Hi
>
> Hi,
>
> > The commit 4c39529663b93165953ecf9b1a9ea817358dcd06 ("slab: Warn on
> > duplicate cache names when DEBUG_VM=y") is causing large number of
> > warnings about "dm_bufio_buffer", "dm_bufio_buffer-%u" (and other) device
> > mapper caches.
>
> Hmm wonder why nobody run into this before. We thought the code that would
> cause the warning would be all fixed before introducing it, but we missed
> some, sorry.

Another data point, I also saw such warning when booting 6.12-rcX (rc1
and rc6), but from mlx5 driver. I thought it was mlx driver specific
issue, it seems not.

[   63.537874] kmem_cache of name 'mlx5_fs_ftes' already exists
[   63.546399] WARNING: CPU: 0 PID: 9 at mm/slab_common.c:107
__kmem_cache_create_args+0xb4/0x330
[   63.557839] Modules linked in: vfat fat mlx5_core(+) dax_hmem
cxl_acpi ampere_cspmu cxl_port cxl_core mlxfw psample einj arm_spe_pmu
arm_cspmu_module tls pci_hyperv_intf acpi_ipmi acpi_tad ipmi_ssif
ipmi_devintf arm_cmn ipmi_msghandler cppc_cpufreq(+) fuse loop
nfnetlink zram xfs crct10dif_ce polyval_ce polyval_generic ghash_ce
sha3_ce nvme sha512_ce nvme_core sha512_arm64 sbsa_gwdt nvme_auth
xgene_hwmon
[   63.596473] CPU: 0 UID: 0 PID: 9 Comm: kworker/0:1 Tainted: G
 W          6.12.0-rc6 #80
[   63.609156] Tainted: [W]=WARN
[   63.614976] Hardware name: ZOLLNER SUNMOONLAKE/SunMoon Lake, BIOS
00.00. 2024-10-30 13:48:02 11/06/2024
[   63.624359] Workqueue: events work_for_cpu_fn
[   63.631569] pstate: 63400009 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
[   63.638523] pc : __kmem_cache_create_args+0xb4/0x330
[   63.646340] lr : __kmem_cache_create_args+0xb4/0x330
[   63.654158] sp : ffff800080d1bc00
[   63.660330] x29: ffff800080d1bc00 x28: 0000000000000000 x27: 0000000000000000
[   63.670318] x26: 0000000000000000 x25: ffffbf101f234410 x24: 00000000000002d8
[   63.677446] x23: 0000000000000000 x22: ffff800080d1bc58 x21: ffffbf101f2344b8
[   63.687436] x20: ffffbf0ff94870a8 x19: ffff2001053a1700 x18: 0000000000000014
[   63.694563] x17: 00000000a871ae29 x16: ffffbf101c282f18 x15: 000000007773661b
[   63.704552] x14: 0000000000000000 x13: 7374736978652079 x12: 646165726c612027
[   63.714539] x11: 736574665f73665f x10: 35786c6d2720656d x9 : ffffbf101c1722f0
[   63.724531] x8 : ffff800080d1b890 x7 : 0000000000000001 x6 : 0000000000000001
[   63.734518] x5 : ffff001f7d803448 x4 : 0000000000000000 x3 : 0000000000000000
[   63.741646] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000108e3a440
[   63.751634] Call trace:
[   63.754072]  __kmem_cache_create_args+0xb4/0x330
[   63.761542]  mlx5_fs_core_alloc+0x158/0x1a8 [mlx5_core]
[   63.769708]  mlx5_init_once+0x13c/0x520 [mlx5_core]
[   63.777528]  mlx5_init_one_devl_locked+0xa8/0x280 [mlx5_core]
[   63.786127]  probe_one+0xe0/0x200 [mlx5_core]
[   63.790569]  local_pci_probe+0x48/0xc0
[   63.794307]  work_for_cpu_fn+0x24/0x40
[   63.798044]  process_one_work+0x180/0x430
[   63.802042]  worker_thread+0x25c/0x380
[   63.805778]  kthread+0xf4/0x108
[   63.808907]  ret_from_fork+0x10/0x20

>
> > I'd like to ask - how to properly fix it?
> >
> > We create a "dm_bufio_buffer" or "dm_bufio_buffer-%u" cache with every dm
> > bufio client. It used to work (and the duplicate caches are merged), but
>
> Note the merging can be disabled so then it's really several caches with
> exactly same name in /proc/slabinfo and inability to create their
> sysfs/debugfs directories.
>
> > now it warns.
> >
> > Should I append a pointer to the dm_bufio structure to the slab cache name
> > to make them different? Or is there any other preferred solution?
>
> Anything that uniquely identifies the client should be ok, but beware e.g.
> device names that can have slashes, see commit a360f311f57a36 (also for the
> simplest possible fix that is an incremented number).
>
> > Note that it is not possible to pre-create the cache "dm_bufio_buffer-%u"
> > in the module's init function, because the size of per-buffer auxiliary
> > data is not known at this point.
>
> Looks like some chose to solve this the harder way, see 4d784c042d164f
>
> > Mikulas
> >
>
>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-08 16:49   ` Yang Shi
@ 2024-11-08 17:00     ` Vlastimil Babka
  2024-11-08 17:43       ` Yang Shi
  0 siblings, 1 reply; 14+ messages in thread
From: Vlastimil Babka @ 2024-11-08 17:00 UTC (permalink / raw)
  To: Yang Shi
  Cc: Mikulas Patocka, Pedro Falcato, Christoph Lameter, Pekka Enberg,
	David Rientjes, Joonsoo Kim, Andrew Morton, zkabelac,
	Roman Gushchin, Hyeonggon Yoo, linux-mm

On 11/8/24 17:49, Yang Shi wrote:
> On Wed, Nov 6, 2024 at 3:35 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>>
>> On 11/6/24 12:19, Mikulas Patocka wrote:
>> > Hi
>>
>> Hi,
>>
>> > The commit 4c39529663b93165953ecf9b1a9ea817358dcd06 ("slab: Warn on
>> > duplicate cache names when DEBUG_VM=y") is causing large number of
>> > warnings about "dm_bufio_buffer", "dm_bufio_buffer-%u" (and other) device
>> > mapper caches.
>>
>> Hmm wonder why nobody run into this before. We thought the code that would
>> cause the warning would be all fixed before introducing it, but we missed
>> some, sorry.
> 
> Another data point, I also saw such warning when booting 6.12-rcX (rc1
> and rc6), but from mlx5 driver. I thought it was mlx driver specific
> issue, it seems not.

Well this particular issue is mlx driver specific.
The caches it creates (per device?) don't seem to be have specific size or
anything to that device, so it should be relatively simple to fix.

> [   63.537874] kmem_cache of name 'mlx5_fs_ftes' already exists
> [   63.546399] WARNING: CPU: 0 PID: 9 at mm/slab_common.c:107
> __kmem_cache_create_args+0xb4/0x330
> [   63.557839] Modules linked in: vfat fat mlx5_core(+) dax_hmem
> cxl_acpi ampere_cspmu cxl_port cxl_core mlxfw psample einj arm_spe_pmu
> arm_cspmu_module tls pci_hyperv_intf acpi_ipmi acpi_tad ipmi_ssif
> ipmi_devintf arm_cmn ipmi_msghandler cppc_cpufreq(+) fuse loop
> nfnetlink zram xfs crct10dif_ce polyval_ce polyval_generic ghash_ce
> sha3_ce nvme sha512_ce nvme_core sha512_arm64 sbsa_gwdt nvme_auth
> xgene_hwmon
> [   63.596473] CPU: 0 UID: 0 PID: 9 Comm: kworker/0:1 Tainted: G
>  W          6.12.0-rc6 #80
> [   63.609156] Tainted: [W]=WARN
> [   63.614976] Hardware name: ZOLLNER SUNMOONLAKE/SunMoon Lake, BIOS
> 00.00. 2024-10-30 13:48:02 11/06/2024
> [   63.624359] Workqueue: events work_for_cpu_fn
> [   63.631569] pstate: 63400009 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
> [   63.638523] pc : __kmem_cache_create_args+0xb4/0x330
> [   63.646340] lr : __kmem_cache_create_args+0xb4/0x330
> [   63.654158] sp : ffff800080d1bc00
> [   63.660330] x29: ffff800080d1bc00 x28: 0000000000000000 x27: 0000000000000000
> [   63.670318] x26: 0000000000000000 x25: ffffbf101f234410 x24: 00000000000002d8
> [   63.677446] x23: 0000000000000000 x22: ffff800080d1bc58 x21: ffffbf101f2344b8
> [   63.687436] x20: ffffbf0ff94870a8 x19: ffff2001053a1700 x18: 0000000000000014
> [   63.694563] x17: 00000000a871ae29 x16: ffffbf101c282f18 x15: 000000007773661b
> [   63.704552] x14: 0000000000000000 x13: 7374736978652079 x12: 646165726c612027
> [   63.714539] x11: 736574665f73665f x10: 35786c6d2720656d x9 : ffffbf101c1722f0
> [   63.724531] x8 : ffff800080d1b890 x7 : 0000000000000001 x6 : 0000000000000001
> [   63.734518] x5 : ffff001f7d803448 x4 : 0000000000000000 x3 : 0000000000000000
> [   63.741646] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000108e3a440
> [   63.751634] Call trace:
> [   63.754072]  __kmem_cache_create_args+0xb4/0x330
> [   63.761542]  mlx5_fs_core_alloc+0x158/0x1a8 [mlx5_core]
> [   63.769708]  mlx5_init_once+0x13c/0x520 [mlx5_core]
> [   63.777528]  mlx5_init_one_devl_locked+0xa8/0x280 [mlx5_core]
> [   63.786127]  probe_one+0xe0/0x200 [mlx5_core]
> [   63.790569]  local_pci_probe+0x48/0xc0
> [   63.794307]  work_for_cpu_fn+0x24/0x40
> [   63.798044]  process_one_work+0x180/0x430
> [   63.802042]  worker_thread+0x25c/0x380
> [   63.805778]  kthread+0xf4/0x108
> [   63.808907]  ret_from_fork+0x10/0x20
> 
>>
>> > I'd like to ask - how to properly fix it?
>> >
>> > We create a "dm_bufio_buffer" or "dm_bufio_buffer-%u" cache with every dm
>> > bufio client. It used to work (and the duplicate caches are merged), but
>>
>> Note the merging can be disabled so then it's really several caches with
>> exactly same name in /proc/slabinfo and inability to create their
>> sysfs/debugfs directories.
>>
>> > now it warns.
>> >
>> > Should I append a pointer to the dm_bufio structure to the slab cache name
>> > to make them different? Or is there any other preferred solution?
>>
>> Anything that uniquely identifies the client should be ok, but beware e.g.
>> device names that can have slashes, see commit a360f311f57a36 (also for the
>> simplest possible fix that is an incremented number).
>>
>> > Note that it is not possible to pre-create the cache "dm_bufio_buffer-%u"
>> > in the module's init function, because the size of per-buffer auxiliary
>> > data is not known at this point.
>>
>> Looks like some chose to solve this the harder way, see 4d784c042d164f
>>
>> > Mikulas
>> >
>>
>>



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: slab warning: kmem_cache of name 'dm_bufio_buffer' already exists
  2024-11-08 17:00     ` Vlastimil Babka
@ 2024-11-08 17:43       ` Yang Shi
  0 siblings, 0 replies; 14+ messages in thread
From: Yang Shi @ 2024-11-08 17:43 UTC (permalink / raw)
  To: Vlastimil Babka, saeedm, tariqt
  Cc: Mikulas Patocka, Pedro Falcato, Christoph Lameter, Pekka Enberg,
	David Rientjes, Joonsoo Kim, Andrew Morton, zkabelac,
	Roman Gushchin, Hyeonggon Yoo, linux-mm

On Fri, Nov 8, 2024 at 9:00 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 11/8/24 17:49, Yang Shi wrote:
> > On Wed, Nov 6, 2024 at 3:35 AM Vlastimil Babka <vbabka@suse.cz> wrote:
> >>
> >> On 11/6/24 12:19, Mikulas Patocka wrote:
> >> > Hi
> >>
> >> Hi,
> >>
> >> > The commit 4c39529663b93165953ecf9b1a9ea817358dcd06 ("slab: Warn on
> >> > duplicate cache names when DEBUG_VM=y") is causing large number of
> >> > warnings about "dm_bufio_buffer", "dm_bufio_buffer-%u" (and other) device
> >> > mapper caches.
> >>
> >> Hmm wonder why nobody run into this before. We thought the code that would
> >> cause the warning would be all fixed before introducing it, but we missed
> >> some, sorry.
> >
> > Another data point, I also saw such warning when booting 6.12-rcX (rc1
> > and rc6), but from mlx5 driver. I thought it was mlx driver specific
> > issue, it seems not.
>
> Well this particular issue is mlx driver specific.
> The caches it creates (per device?) don't seem to be have specific size or
> anything to that device, so it should be relatively simple to fix.

Aha, thank you. Added mlx driver maintainers.

>
> > [   63.537874] kmem_cache of name 'mlx5_fs_ftes' already exists
> > [   63.546399] WARNING: CPU: 0 PID: 9 at mm/slab_common.c:107
> > __kmem_cache_create_args+0xb4/0x330
> > [   63.557839] Modules linked in: vfat fat mlx5_core(+) dax_hmem
> > cxl_acpi ampere_cspmu cxl_port cxl_core mlxfw psample einj arm_spe_pmu
> > arm_cspmu_module tls pci_hyperv_intf acpi_ipmi acpi_tad ipmi_ssif
> > ipmi_devintf arm_cmn ipmi_msghandler cppc_cpufreq(+) fuse loop
> > nfnetlink zram xfs crct10dif_ce polyval_ce polyval_generic ghash_ce
> > sha3_ce nvme sha512_ce nvme_core sha512_arm64 sbsa_gwdt nvme_auth
> > xgene_hwmon
> > [   63.596473] CPU: 0 UID: 0 PID: 9 Comm: kworker/0:1 Tainted: G
> >  W          6.12.0-rc6 #80
> > [   63.609156] Tainted: [W]=WARN
> > [   63.614976] Hardware name: ZOLLNER SUNMOONLAKE/SunMoon Lake, BIOS
> > 00.00. 2024-10-30 13:48:02 11/06/2024
> > [   63.624359] Workqueue: events work_for_cpu_fn
> > [   63.631569] pstate: 63400009 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
> > [   63.638523] pc : __kmem_cache_create_args+0xb4/0x330
> > [   63.646340] lr : __kmem_cache_create_args+0xb4/0x330
> > [   63.654158] sp : ffff800080d1bc00
> > [   63.660330] x29: ffff800080d1bc00 x28: 0000000000000000 x27: 0000000000000000
> > [   63.670318] x26: 0000000000000000 x25: ffffbf101f234410 x24: 00000000000002d8
> > [   63.677446] x23: 0000000000000000 x22: ffff800080d1bc58 x21: ffffbf101f2344b8
> > [   63.687436] x20: ffffbf0ff94870a8 x19: ffff2001053a1700 x18: 0000000000000014
> > [   63.694563] x17: 00000000a871ae29 x16: ffffbf101c282f18 x15: 000000007773661b
> > [   63.704552] x14: 0000000000000000 x13: 7374736978652079 x12: 646165726c612027
> > [   63.714539] x11: 736574665f73665f x10: 35786c6d2720656d x9 : ffffbf101c1722f0
> > [   63.724531] x8 : ffff800080d1b890 x7 : 0000000000000001 x6 : 0000000000000001
> > [   63.734518] x5 : ffff001f7d803448 x4 : 0000000000000000 x3 : 0000000000000000
> > [   63.741646] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000108e3a440
> > [   63.751634] Call trace:
> > [   63.754072]  __kmem_cache_create_args+0xb4/0x330
> > [   63.761542]  mlx5_fs_core_alloc+0x158/0x1a8 [mlx5_core]
> > [   63.769708]  mlx5_init_once+0x13c/0x520 [mlx5_core]
> > [   63.777528]  mlx5_init_one_devl_locked+0xa8/0x280 [mlx5_core]
> > [   63.786127]  probe_one+0xe0/0x200 [mlx5_core]
> > [   63.790569]  local_pci_probe+0x48/0xc0
> > [   63.794307]  work_for_cpu_fn+0x24/0x40
> > [   63.798044]  process_one_work+0x180/0x430
> > [   63.802042]  worker_thread+0x25c/0x380
> > [   63.805778]  kthread+0xf4/0x108
> > [   63.808907]  ret_from_fork+0x10/0x20
> >
> >>
> >> > I'd like to ask - how to properly fix it?
> >> >
> >> > We create a "dm_bufio_buffer" or "dm_bufio_buffer-%u" cache with every dm
> >> > bufio client. It used to work (and the duplicate caches are merged), but
> >>
> >> Note the merging can be disabled so then it's really several caches with
> >> exactly same name in /proc/slabinfo and inability to create their
> >> sysfs/debugfs directories.
> >>
> >> > now it warns.
> >> >
> >> > Should I append a pointer to the dm_bufio structure to the slab cache name
> >> > to make them different? Or is there any other preferred solution?
> >>
> >> Anything that uniquely identifies the client should be ok, but beware e.g.
> >> device names that can have slashes, see commit a360f311f57a36 (also for the
> >> simplest possible fix that is an incremented number).
> >>
> >> > Note that it is not possible to pre-create the cache "dm_bufio_buffer-%u"
> >> > in the module's init function, because the size of per-buffer auxiliary
> >> > data is not known at this point.
> >>
> >> Looks like some chose to solve this the harder way, see 4d784c042d164f
> >>
> >> > Mikulas
> >> >
> >>
> >>
>


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2024-11-08 17:44 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-06 11:19 slab warning: kmem_cache of name 'dm_bufio_buffer' already exists Mikulas Patocka
2024-11-06 11:34 ` Vlastimil Babka
2024-11-06 12:05   ` Mikulas Patocka
2024-11-06 16:28     ` Vlastimil Babka
2024-11-06 21:22       ` Mikulas Patocka
2024-11-08  9:56         ` Vlastimil Babka
2024-11-08 11:13           ` Hyeonggon Yoo
2024-11-08 11:21             ` Vlastimil Babka
2024-11-08 13:47           ` Pedro Falcato
2024-11-08 10:08         ` Vlastimil Babka
2024-11-08 14:56           ` Christoph Hellwig
2024-11-08 16:49   ` Yang Shi
2024-11-08 17:00     ` Vlastimil Babka
2024-11-08 17:43       ` Yang Shi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox