Re: [PATCH] nvmet: dynamically allocate nvmet_ns->nguid

[Jens Axboe <axboe@kernel.dk>]

On 5/3/23 4:39?PM, Chaitanya Kulkarni wrote:
> On 5/3/23 08:45, Jens Axboe wrote:
>> On 5/1/23 11:30?PM, Chaitanya Kulkarni wrote:
>>> The nvmet_ns struct is critical to I/O operations in each backend bdev
>>> and file, but its static nguid array is not accessed in the fast path.
>>> This means that pulling all the memory for the array on each access
>>> is inefficient.
>>>
>>> This patch dynamically allocates the nvmet_ns->nguid array, reducing the
>>> size of the nvmet_ns struct. This optimization should reduce unnecessary
>>> memory access in the fast path that is required for the array vs pointer.
>>> For allocation of nguid with kzalloc() use same policy GFP_KERNEL that is
>>> used to allocate nvmet_ns struct iself.
>> Replacing 16 bytes of embedded memory with 8 bytes and then alloc+free
>> seems like a poor tradeoff.
>>
>> Why not just arrange it a bit more sanely, and also push the config
>> stuff out-of-line as that would not be used in the fast path. The below
>> 30 second job takes it from 456 -> 440 bytes for me, and has a better
>> layout imho.
>>   
> Intentionally I didn't touch reordering since I've asked Christoph J
> look into it so my patch only optimizing the nguid allocation.
> Just looked into his series your patch is missing from that.
> 
> Moving configfs out of the way and combining bool together is
> much better layout ...

But I think you missed my point, which is that replacing 16 bytes with 8
bytes, and then adding an alloc+free on top for the latter, doesn't make
a lot of sense. And some saner reordering gets you a bigger win, without
needing to resort to any of that.

-- 
Jens Axboe