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
>
