[PATCH v2 0/3] nvmet-rdma: SRQ per completion vector

[Max Gurtovoy]

On 11/18/2017 4:40 PM, Leon Romanovsky wrote:
> On Sat, Nov 18, 2017 at 03:57:15PM +0200, Max Gurtovoy wrote:
>>
>>
>> On 11/18/2017 2:52 PM, Leon Romanovsky wrote:
>>> On Fri, Nov 17, 2017 at 09:32:42PM +0200, Max Gurtovoy wrote:
>>>>
>>>>
>>>> On 11/16/2017 8:36 PM, Sagi Grimberg wrote:
>>>>>
>>>>>> Since there is an active discussion regarding the CQ pool
>>>>>> architecture, I decided to push
>>>>>> this feature (maybe it can be pushed before CQ pool).
>>>>>>
>>>>>> This is a new feature for NVMEoF RDMA target,
>>>>>
>>>>> Any chance having this for the rest? isert, srpt, svcrdma?
>>>>>
>>>>
>>>> We can implement it for isert, but I think it's better to see how the CQ
>>>> pool will be defined first.
>>>> It can bring a big benefit and improvement for ib_srpt (similar to NVMEoF
>>>> target) but I'm not sure if I can commit for that one soon..
>>>
>>> Too bad, but I don't see inclusion of generic SRQ pool code in RDMA
>>> subsystem without actual conversion of existing ULP clients.
>>>
>>> Thanks
>>>
>>
>> This patchset adds this feature to NVMEoF target so actually there are ULPs
>> that use it. Same issue we have with mr_pool that only RDMA rw.c use it (Now
>> we're adding it to NVMEoF initiators too - in review).
> 
> The difference between your code and mr_pool is that mr_pool is part of
> RDMA/core and in use by RDMA/core (rw.c), which in use by all ULPs.
> 
> However if you insist, we can remove EXPORT_SYMBOL from mr_pool
> implementation, because of being part of RDMA/core and it blows
> symbols map without need. Should I?

No, we'll use it in NVMEoF host as I mentioned earlier.
> 
> In your case, you are proposing generic interface, which supposed to be
> good fit for all ULPs but without those ULPs.
> 
>> I can add srq_pool to iSER target code but I don't want to re-write it again
>> in few weeks, when the CQ pool will be added.
> 
> So, please finalize interface in RFC stage and once you are ready, proceed to
> the actual patches.
> 
>> Regarding other ULPs, we don't have a testing environment for them so I
>> prefer not to commit on their implementation in the near future.
> 
> You are not expected to have all testing environment, it is their (ULPs
> maintainers) responsibility to test your conversion, because you are
> doing conversion to generic interface.
> 
>>
>> I don't know why we can't add this feature "as is".
>> Other ULPs maintainers might use it once it will be pushed.
> 
> Sorry, but it is not how kernel development process works.
> "You propose -> you do" and not "You propose -> they do".

I'm not changing an interface here. So all the other ULPs that use SRQ 
(ipoib and srpt) currently will cuntinue using it.
I don't know why this patchset brought up the idea to add SRQ pools to 
isert/svcrdma/etc.., but knowing that there are patches (under 
discussions) that will have a big enfluance on these drivers (at least 
isert), it doesn't make sence to implement *new* feature (SRQ usage) and 
chage it a week afterwards.

I will send V3 in a few days with some fixes that I got, so it would be 
nice to have a more comments on the code (I don't see a problem with the 
kernel development process in this patchset).


> 
> Thanks
> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html