Re: [PATCH 0/8 v1] Remove FMR support from RDMA drivers

[santosh.shilimkar@oracle.com]

On 5/14/20 3:23 PM, Sagi Grimberg wrote:
> 
>>> +Santosh
>>>
>>> You probably meant to copy the RDS maintainer? Not sure if this 
>>> should have
>>> also been sent to netdev@vger.kernel.org.
>>>
>> Thanks Aron.
>>
>>>
>>>> On May 14, 2020, at 7:02 AM, Max Gurtovoy <maxg@mellanox.com> wrote:
>>>>
>>>> This series removes the support for FMR mode to register memory. 
>>>> This ancient
>>>> mode is unsafe and not maintained/tested in the last few years. It 
>>>> also doesn't
>>>> have any reasonable advantage over other memory registration methods 
>>>> such as
>>>> FRWR (that is implemented in all the recent RDMA adapters). This 
>>>> series should
>>>> be reviewed and approved by the maintainer of the effected drivers 
>>>> and I
>>>> suggest to test it as well.
>>>>
>> I know the security issue has been brought up before and this plan of 
>> removal of FMR support was on the cards
> 
> Actually, what is unsafe is not necessarily fmrs, but rather the
> fmr_pool interface. So Max, you can keep fmr if rds wants it, but
> we can get rid of fmr pools.
>
Good point. We aren't using the fmr_pools.

>> but on RDS at least on CX3 we
>> got more throughput with FMR vs FRWR. And the reasons are well
>> understood as well why its the case.
> 
> Looking at the rds code, it seems that rds doesn't do any fast
> registration at all, rkeys are long lived and are only invalidated (or
> unmaped) when need recycling or when a socket is torn down...
> 
> So I'm not clear exactly about the model here, but seems to me
> its almost like rds needs something like phys_mr, which is static for
> all of its lifetime. It seems that fmrs just create a hassle for
> rds, unless I'm missing something...
> 
> Having said that, it surely isn't the most secure behavior...
> At least its not the global dma rkey...
>
There are couple of layers but you can see the FRWR code inside,
net/rds/ib_frmr.c. The MR allocation as well as free/invalidation
is managed from user-land instead of ULP data path. There are
couple of cases where some use_once semantics does MR invalidation
within kernel but thats only because userland indicated that MR
key can be invalidated after issued RDMA ops is complete.

>> Is it possible to keep core support still around so that HCA's which
>> supports FMR, ULPs can still can leverage it if they want.
>>  From RDS perspective, if the HCA like CX3 doesn't support both modes,
>> code prefers FMR vs FRWR and hence the question.
> 
> Max can start by removing fmr_pools, fmrs can stay as there is nothing
> fundamentally wrong with them. And apparently there are still users.

That will surely help if its an option. RDS don't use fmr_pools so no
issues there.

Regards,
Santosh