[PATCH] nvme: don't set a virt_boundary unless needed

[Sagi Grimberg]

On 12/25/23 12:36, Max Gurtovoy wrote:
> 
> 
> On 25/12/2023 12:08, Sagi Grimberg wrote:
>>
>>
>> On 12/22/23 03:16, Max Gurtovoy wrote:
>>>
>>>
>>> On 21/12/2023 11:30, Sagi Grimberg wrote:
>>>>
>>>>> NVMe PRPs are a pain and force the expensive virt_boundary checking on
>>>>> block layer, prevent secure passthrough and require scatter/gather I/O
>>>>> to be split into multiple commands which is problematic for the 
>>>>> upcoming
>>>>> atomic write support.
>>>>
>>>> But is the threshold still correct? meaning for I/Os small enough the
>>>> device will have lower performance? I'm not advocating that we keep it,
>>>> but we should at least mention the tradeoff in the change log.
>>>>
>>>>> Fix the NVMe core to require an opt-in from the drivers for it.
>>>>>
>>>>> For nvme-apple it is always required as the driver only supports PRPs.
>>>>>
>>>>> For nvme-pci when SGLs are supported we'll always use them for data 
>>>>> I/O
>>>>> that would require a virt_boundary.
>>>>>
>>>>> For nvme-rdma the virt boundary is always required, as RMDA MRs are 
>>>>> just
>>>>> as dumb as NVMe PRPs.
>>>>
>>>> That is actually device dependent. The driver can ask for a pool of
>>>> mrs with type IB_MR_TYPE_SG_GAPS if the device supports 
>>>> IBK_SG_GAPS_REG.
>>>>
>>>> See from ib_srp.c:
>>>> -- 
>>>>         if (device->attrs.kernel_cap_flags & IBK_SG_GAPS_REG)
>>>>                  mr_type = IB_MR_TYPE_SG_GAPS;
>>>>          else
>>>>                  mr_type = IB_MR_TYPE_MEM_REG;
>>>
>>> For now, I prefer not using the IB_MR_TYPE_SG_GAPS MR in NVMe/RDMA 
>>> since in the case of virtual contiguous data buffers it is better to 
>>> use IB_MR_TYPE_MEM_REG. It gives much better performance. This is the 
>>> reason I didn't add IB_MR_TYPE_SG_GAPS MR support for NVMe/RDMA.
>>
>> I see. I guess it is not *that* trivial then.
>>
>>> I actually had a plan to re-write the IB_MR_TYPE_SG_GAPS MR logic (or 
>>> create a new MR type) that will internally open 2 MRs so if the IO is 
>>> contiguous it will use the MTT/MEM_REG and if it isn't it will use 
>>> the KLM/SG_GAPS.
>>> This is how we implemented the SIG_MR but still didn't make it for 
>>> the IB_MR_TYPE_SG_GAPS MR.
>>
>> Sounds like a reasonable option. But doesn't think mean that the
>> driver will need to scan the page scatterlist to determine what internal
>> mr to use? Even a fallback mechanism can be affected by a given
>> workload. Plus there is the cost of doubling the number of preallocated
>> mrs.
>>
> 
> Scanning the scatterlist is done anyway for mapping purposes so I don't 
> think it will affect the performance.
> The cost of doubling the number of MRs is the what we need to pay to get 
> optimal performance for contig and discontig IOs, I guess..
> 
>>> Actually, I think we should have the same logic in the NVMe PCI driver:
>>> if the IOs can be delivered as PRPs then the driver will prepare SQE 
>>> with PRP. Otherwise, driver will prepare SGL.
>>> I think that doing the check in the driver for each IO is not so bad 
>>> and devices will get benefit from it. Usually HW devices like to work 
>>> with contiguous buffers. If the buffers can't be mapped with PRPs, 
>>> then the HW will work a bit harder and use SGLs (it is better than 
>>> doing a bounce buffer in the block layer).
>>>
>>> I actually did a POC internally for NVMe/RDMA and created sg_gaps 
>>> ib_mr and mem_reg ib_mr and checked the buffers mapping for each IO 
>>> and got a big benefit if the buffers were discontig (used the sg_gaps 
>>> mr). Also the contig buffers performance didn't degraded because of 
>>> the check of the buffers mapping.
>>>
>>> I created a fio flags that in purpose sends discontig IOs for my 
>>> testing.
>>>
>>> WDYT ?
>>
>> Sounds possible. However for rdma we probably want this transparent to
>> the ulp such that all consumers can have this benefit. Also perhaps add
>> this logic in the rdma core so other drivers can use it as well
>> (although I don't know if any other rdma driver supports sg gaps
>> anyways).
>>
>> If this proves to be a good approach, pci can do something similar.
> 
> For RDMA, I plan to do it in the device driver (mlx5) layer and not the 
> ib_core layer. It is unique to our implementation.

Well, SG_GAPS is not intended to be a unique capability (although it is
today in practice I guess).

> 
> For the NVMe PCI case, I suggested doing it unrelated to the NVMe/RDMA 
> solution. The NVMe/PCI is actually the device driver of the PCI device 
> and the scanning of the scatterlist should happen in the device driver.
> I suggest to try this solution since we always debating about thresholds 
> and when to use SGLs.
> Now that Christoph opens the gate for the driver to work with discontig 
> IOs I believe that for *any* discontig IO we should use SGLs and for 
> *any* contig IO we should use PRPs.

Why *any* contig IO? There are certainly cases where sgls would perform
better with sgls than prps I'd assume... For example if a large buffer
is physically contiguous (say a huge page)?