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

From: Max Gurtovoy <maxg@mellanox.com>
To: Sagi Grimberg <sagi@grimberg.me>,
	santosh.shilimkar@oracle.com,
	Aron Silverton <aron.silverton@oracle.com>
Cc: bvanassche@acm.org, Jason Gunthorpe <jgg@mellanox.com>,
	linux-rdma@vger.kernel.org, dledford@redhat.com, leon@kernel.org,
	israelr@mellanox.com, shlomin@mellanox.com
Subject: Re: [PATCH 0/8 v1] Remove FMR support from RDMA drivers
Date: Fri, 15 May 2020 03:37:11 +0300	[thread overview]
Message-ID: <1bd4bf77-b97b-3c6f-ce3a-4d5fc428f454@mellanox.com> (raw)
In-Reply-To: <479add48-6fdb-f925-c3b9-699c6aa4cfbf@grimberg.me>

On 5/15/2020 1:23 AM, 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.
>
>> 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...
>
>> 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.

Ok we can start with this.

Please review patches 5, 6, 7 that are stand alone.

And I'll send a V2 for SRP/ISER/FMR_pool only.