Re: [PATCH] nvmet-rdma: use sbitmap to replace rsp free list

[Guixin Liu <kanie@linux.alibaba.com>]

在 2024/10/7 15:05, Christoph Hellwig 写道:
> On Wed, Sep 25, 2024 at 06:51:00PM +0800, Guixin Liu wrote:
>> We can use sbitmap to manage all the nvmet_rdma_rsp instead of using
>> free lists and spinlock, and we can use an additional tag to
>> determine whether the nvmet_rdma_rsp is extra allocated.
> Can you explain why?  I guess it's better performance for some definition
> of "performance", but it would be great to state that here, preferably
> including numbers.

Sure, my test result:

Use local rxe rdma device and mem-based back device.

Fio command, test the average 5 times:

fio -filename=/dev/nvme0n1 --ioengine=libaio -direct=1 -size=1G -name=1 
-thread -runtime=60 -time_based -rw=read -numjobs=16 -iodepth=128 -bs=4k 
-group_reporting

Before: 241k IOPS

After:  256k IOPS

I will add the result into v2.

>>   static int nvmet_rdma_alloc_rsp(struct nvmet_rdma_device *ndev,
>> -				struct nvmet_rdma_rsp *r);
>> +				struct nvmet_rdma_rsp *r,
>> +				int tag);
> Nit: take can go on the line above.
OK
>> -		ret = nvmet_rdma_alloc_rsp(queue->dev, rsp);
>> +		ret = nvmet_rdma_alloc_rsp(queue->dev, rsp, -1);
> Please add a symbolic name (NVME_RDMA_RSP_DYNAMIC?) for the magic -1
> tag.
OK
>> @@ -241,17 +236,13 @@ nvmet_rdma_get_rsp(struct nvmet_rdma_queue *queue)
>>   static inline void
>>   nvmet_rdma_put_rsp(struct nvmet_rdma_rsp *rsp)
>>   {
>> -	unsigned long flags;
>> -
>> -	if (unlikely(rsp->allocated)) {
>> +	if (unlikely(rsp->tag < 0)) {
> .. and use it here.
>
> Otherwise this looks reasonable to me.

Thanks, Best Regards,

Guixin Liu