Re: [PATCH] RDMA/srpt: Make slab cache names unique

[Jens Axboe <axboe@kernel.dk>]

On 10/7/24 10:52 AM, Bart Van Assche wrote:
> On 10/7/24 9:28 AM, Jens Axboe wrote:
>> On 10/7/24 10:14 AM, Bart Van Assche wrote:
>>> On 10/7/24 7:06 AM, Jens Axboe wrote:
>>>> Still seems way over engineered, just use an atomic_long_t for a
>>>> continually increasing index number.
>>>
>>> Even an atomic_long_t can wrap around and hence can result in duplicate
>>> slab cache names. With my patch it is guaranteed that slab cache names
>>> are unique. I'm not claiming that this patch is the best possible
>>> solution but it's a working solution and a solution that doesn't require
>>> too many changes to the ib_srpt driver.
>>
>> Come on... The current patch doesn't even check if ida_alloc() got an ID.
>> Without that, using some mechanism to alloc+free an index is surely less
>> than useful.
> 
> Is it necessary in this case to check the ida_alloc() result? ida_free()
> ignores negative values. So if ida_alloc() fails, the worst that can
> happen is that a slab name with a negative number is passed to kmem_cache_create(). Additionally, if my interpretation of the ida code
> is correct, it allocates memory in 128 byte chunks. So if allocation of
> an ida fails, it means that less than 128 bytes of memory are left. More
> than 128 bytes are required by kmem_cache_create(). Hence, if ida
> allocation fails, the kmem cache creation will also fail and the slab
> name with the negative number will not become visible in procfs.
> 
> Do you agree that in this case it is safe not to check whether
> ida_alloc() succeeds?

I'm not worried about OOM, what if you run out of space? And yes the
free part deals with it fine, but you're right back to having duplicate
slab names in that case. I'm done arguing about this silly thing, I
stand by my comment that using ida for this is overengineering. And that
yes there are 3 slab caches, but having 3 per whatever instance is silly
and you should share those 3 across instances. And guess what, if that
was done, then you would not need to worry about creating silly indexes
to avoid conflicts in slab names. Not only would it be more efficient in
terms of overhead, it'd also fix this problem at the same time rather
than paper over it.

-- 
Jens Axboe