| Message ID | 20200514120305.189738-1-maxg@mellanox.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Remove FMR support from RDMA drivers | expand |
+Santosh You probably meant to copy the RDS maintainer? Not sure if this should have also been sent to netdev@vger.kernel.org. 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. > > The tests that I made for this series (fio benchmarks and fio verify data): > 1. iSER initiator on ConnectX-4 > 2. iSER initiator on ConnectX-3 > 3. SRP initiator on ConnectX-4 (loopback to SRP target) > 4. SRP initiator on ConnectX-3 > > Not tested: > 1. RDS > 2. mthca > 3. rdmavt > > Israel Rukshin (1): > RDMA/iser: Remove support for FMR memory registration > > Max Gurtovoy (7): > RDMA/mlx4: remove FMR support for memory registration > RDMA/rds: remove FMR support for memory registration > RDMA/mthca: remove FMR support for memory registration > RDMA/rdmavt: remove FMR memory registration > RDMA/srp: remove support for FMR memory registration > RDMA/core: remove FMR pool API > RDMA/core: remove FMR device ops > > Documentation/driver-api/infiniband.rst | 3 - > Documentation/infiniband/core_locking.rst | 2 - > drivers/infiniband/core/Makefile | 2 +- > drivers/infiniband/core/device.c | 4 - > drivers/infiniband/core/fmr_pool.c | 494 --------------------------- > drivers/infiniband/core/verbs.c | 48 --- > drivers/infiniband/hw/mlx4/main.c | 10 - > drivers/infiniband/hw/mlx4/mlx4_ib.h | 16 - > drivers/infiniband/hw/mlx4/mr.c | 93 ----- > drivers/infiniband/hw/mthca/mthca_dev.h | 10 - > drivers/infiniband/hw/mthca/mthca_mr.c | 262 +------------- > drivers/infiniband/hw/mthca/mthca_provider.c | 86 ----- > drivers/infiniband/sw/rdmavt/mr.c | 154 --------- > drivers/infiniband/sw/rdmavt/mr.h | 15 - > drivers/infiniband/sw/rdmavt/vt.c | 4 - > drivers/infiniband/ulp/iser/iscsi_iser.h | 79 +---- > drivers/infiniband/ulp/iser/iser_initiator.c | 19 +- > drivers/infiniband/ulp/iser/iser_memory.c | 188 +--------- > drivers/infiniband/ulp/iser/iser_verbs.c | 126 +------ > drivers/infiniband/ulp/srp/ib_srp.c | 222 +----------- > drivers/infiniband/ulp/srp/ib_srp.h | 27 +- > drivers/net/ethernet/mellanox/mlx4/mr.c | 183 ---------- > include/linux/mlx4/device.h | 21 +- > include/rdma/ib_fmr_pool.h | 93 ----- > include/rdma/ib_verbs.h | 45 --- > net/rds/Makefile | 2 +- > net/rds/ib.c | 14 +- > net/rds/ib.h | 1 - > net/rds/ib_cm.c | 4 +- > net/rds/ib_fmr.c | 269 --------------- > net/rds/ib_mr.h | 12 - > net/rds/ib_rdma.c | 16 +- > 32 files changed, 77 insertions(+), 2447 deletions(-) > delete mode 100644 drivers/infiniband/core/fmr_pool.c > delete mode 100644 include/rdma/ib_fmr_pool.h > delete mode 100644 net/rds/ib_fmr.c > > -- > 1.8.3.1 >
On 14/05/2020 15:02, Max Gurtovoy 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. > > The tests that I made for this series (fio benchmarks and fio verify data): > 1. iSER initiator on ConnectX-4 > 2. iSER initiator on ConnectX-3 > 3. SRP initiator on ConnectX-4 (loopback to SRP target) > 4. SRP initiator on ConnectX-3 > > Not tested: > 1. RDS > 2. mthca > 3. rdmavt I think there are a few leftovers: From f289a67b47e03d268469211065bf114cbb1c7125 Mon Sep 17 00:00:00 2001 From: Gal Pressman <galpress@amazon.com> Date: Wed, 13 May 2020 10:49:09 +0300 Subject: [PATCH] RDMA/mlx5: Remove FMR leftovers Remove a few leftovers from FMR functionality which are no longer used. Signed-off-by: Gal Pressman <galpress@amazon.com> --- drivers/infiniband/hw/mlx5/mlx5_ib.h | 8 -------- 1 file changed, 8 deletions(-) diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h index 482b54eb9764..40c461017763 100644 --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h @@ -675,12 +675,6 @@ struct umr_common { struct semaphore sem; }; -enum { - MLX5_FMR_INVALID, - MLX5_FMR_VALID, - MLX5_FMR_BUSY, -}; - struct mlx5_cache_ent { struct list_head head; /* sync access to the cahce entry @@ -1253,8 +1247,6 @@ int mlx5_query_mad_ifc_port(struct ib_device *ibdev, u8 port, struct ib_port_attr *props); int mlx5_ib_query_port(struct ib_device *ibdev, u8 port, struct ib_port_attr *props); -int mlx5_ib_init_fmr(struct mlx5_ib_dev *dev); -void mlx5_ib_cleanup_fmr(struct mlx5_ib_dev *dev); void mlx5_ib_cont_pages(struct ib_umem *umem, u64 addr, unsigned long max_page_shift, int *count, int *shift,
On 5/14/20 8:13 AM, Aron Silverton 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 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. 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. Also while re-posting the series, please copy me on the patches. Regards, Santosh
On 5/14/2020 9:18 PM, santosh.shilimkar@oracle.com wrote: > > > On 5/14/20 8:13 AM, Aron Silverton 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 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. Please share the benchmark for both and the reasons that you understood for the difference. We'll try to analyze how to improve ConnectX-3 FRWR > > 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. Well this series delete around 2400-2500 LOC that are probably almost not used and tested. Lets try to understand the perf degradation. > > Also while re-posting the series, please copy me on the patches. Sure. > > Regards, > Santosh >
>> +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.
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
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.
On 5/14/2020 7:41 PM, santosh.shilimkar@oracle.com wrote: > 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. It's not only the fmr_pools. The FMR API operates on a page granularity, so a sub-page registration, or a non-page-aligned one, ends up exposing data outside the buffer. This is done silently, and is a significant vulnerability for most upper layers. Tom. >>> 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 > > > >
> It's not only the fmr_pools. The FMR API operates on a page granularity, > so a sub-page registration, or a non-page-aligned one, ends up exposing > data outside the buffer. This is done silently, and is a significant > vulnerability for most upper layers. You're right Tom, I forgot about that... I guess I'd vote to remove it altogether then...
On 5/14/2020 7:00 PM, Gal Pressman wrote: > On 14/05/2020 15:02, Max Gurtovoy 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. >> >> The tests that I made for this series (fio benchmarks and fio verify data): >> 1. iSER initiator on ConnectX-4 >> 2. iSER initiator on ConnectX-3 >> 3. SRP initiator on ConnectX-4 (loopback to SRP target) >> 4. SRP initiator on ConnectX-3 >> >> Not tested: >> 1. RDS >> 2. mthca >> 3. rdmavt > I think there are a few leftovers: > > From f289a67b47e03d268469211065bf114cbb1c7125 Mon Sep 17 00:00:00 2001 > From: Gal Pressman <galpress@amazon.com> > Date: Wed, 13 May 2020 10:49:09 +0300 > Subject: [PATCH] RDMA/mlx5: Remove FMR leftovers > > Remove a few leftovers from FMR functionality which are no longer used. > > Signed-off-by: Gal Pressman <galpress@amazon.com> > --- > drivers/infiniband/hw/mlx5/mlx5_ib.h | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h > index 482b54eb9764..40c461017763 100644 > --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h > @@ -675,12 +675,6 @@ struct umr_common { > struct semaphore sem; > }; > > -enum { > - MLX5_FMR_INVALID, > - MLX5_FMR_VALID, > - MLX5_FMR_BUSY, > -}; > - > struct mlx5_cache_ent { > struct list_head head; > /* sync access to the cahce entry > @@ -1253,8 +1247,6 @@ int mlx5_query_mad_ifc_port(struct ib_device *ibdev, u8 port, > struct ib_port_attr *props); > int mlx5_ib_query_port(struct ib_device *ibdev, u8 port, > struct ib_port_attr *props); > -int mlx5_ib_init_fmr(struct mlx5_ib_dev *dev); > -void mlx5_ib_cleanup_fmr(struct mlx5_ib_dev *dev); > void mlx5_ib_cont_pages(struct ib_umem *umem, u64 addr, > unsigned long max_page_shift, > int *count, int *shift, Thanks Gal. This is a dead code regardless to this series but I'll add it.
On 5/15/2020 9:59 PM, Sagi Grimberg wrote: > >> It's not only the fmr_pools. The FMR API operates on a page granularity, >> so a sub-page registration, or a non-page-aligned one, ends up exposing >> data outside the buffer. This is done silently, and is a significant >> vulnerability for most upper layers. > > You're right Tom, I forgot about that... > I guess I'd vote to remove it altogether then... Santosh, I don't really understand the mechanism you use from user/kernel regarding the registration. But I saw you use wait_event and wait for completing the FRWR/INVALIDATION. For sure this is not optimal. But can't really say what is the bottleneck in your case. Can you share the numbers you get for FMR/FRWR benchmarks ?
On 5/14/2020 8:02 AM, Max Gurtovoy 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. > > The tests that I made for this series (fio benchmarks and fio verify data): > 1. iSER initiator on ConnectX-4 > 2. iSER initiator on ConnectX-3 > 3. SRP initiator on ConnectX-4 (loopback to SRP target) > 4. SRP initiator on ConnectX-3 > > Not tested: > 1. RDS > 2. mthca > 3. rdmavt This will effectively kill qib which uses rdmavt. It's gonna have to be a NAK from me. -Denny
On 5/17/20 3:51 AM, Max Gurtovoy wrote: > > On 5/15/2020 9:59 PM, Sagi Grimberg wrote: >> >>> It's not only the fmr_pools. The FMR API operates on a page granularity, >>> so a sub-page registration, or a non-page-aligned one, ends up exposing >>> data outside the buffer. This is done silently, and is a significant >>> vulnerability for most upper layers. >> >> You're right Tom, I forgot about that... >> I guess I'd vote to remove it altogether then... > > Santosh, > > I don't really understand the mechanism you use from user/kernel > regarding the registration. > > But I saw you use wait_event and wait for completing the > FRWR/INVALIDATION. For sure this is not optimal. > > But can't really say what is the bottleneck in your case. > > Can you share the numbers you get for FMR/FRWR benchmarks ? > The upstream code for FRWR is behind the optimal code we have been using. The invalidation path is removed from datapath with proxy QP within the PD. Anyway, please bounce your patches for me to review to make sure nothing breaks. From various responses, it seems to be aligned to move ahead with the patch-set, so lets go with it. Regards, Santosh
On Mon, May 18, 2020 at 11:20:04AM -0400, Dennis Dalessandro wrote: > On 5/14/2020 8:02 AM, Max Gurtovoy 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. > > > > The tests that I made for this series (fio benchmarks and fio verify data): > > 1. iSER initiator on ConnectX-4 > > 2. iSER initiator on ConnectX-3 > > 3. SRP initiator on ConnectX-4 (loopback to SRP target) > > 4. SRP initiator on ConnectX-3 > > > > Not tested: > > 1. RDS > > 2. mthca > > 3. rdmavt > > This will effectively kill qib which uses rdmavt. It's gonna have to be a > NAK from me. Are you objecting the SRP and iSER changes too? Jason
On 5/18/2020 2:10 PM, Jason Gunthorpe wrote: > On Mon, May 18, 2020 at 11:20:04AM -0400, Dennis Dalessandro wrote: >> On 5/14/2020 8:02 AM, Max Gurtovoy 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. >>> >>> The tests that I made for this series (fio benchmarks and fio verify data): >>> 1. iSER initiator on ConnectX-4 >>> 2. iSER initiator on ConnectX-3 >>> 3. SRP initiator on ConnectX-4 (loopback to SRP target) >>> 4. SRP initiator on ConnectX-3 >>> >>> Not tested: >>> 1. RDS >>> 2. mthca >>> 3. rdmavt >> >> This will effectively kill qib which uses rdmavt. It's gonna have to be a >> NAK from me. > > Are you objecting the SRP and iSER changes too? No, just want to keep basic verbs support at least. NFS already dropped, similarly we are ok with dropping it from SRP/iSER as a next step. -Denny
On Tue, May 19, 2020 at 09:43:14AM -0400, Dennis Dalessandro wrote: > On 5/18/2020 2:10 PM, Jason Gunthorpe wrote: > > On Mon, May 18, 2020 at 11:20:04AM -0400, Dennis Dalessandro wrote: > > > On 5/14/2020 8:02 AM, Max Gurtovoy 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. > > > > > > > > The tests that I made for this series (fio benchmarks and fio verify data): > > > > 1. iSER initiator on ConnectX-4 > > > > 2. iSER initiator on ConnectX-3 > > > > 3. SRP initiator on ConnectX-4 (loopback to SRP target) > > > > 4. SRP initiator on ConnectX-3 > > > > > > > > Not tested: > > > > 1. RDS > > > > 2. mthca > > > > 3. rdmavt > > > > > > This will effectively kill qib which uses rdmavt. It's gonna have to be a > > > NAK from me. > > > > Are you objecting the SRP and iSER changes too? > > No, just want to keep basic verbs support at least. NFS already dropped, > similarly we are ok with dropping it from SRP/iSER as a next step. So you see a major user in RDS for qib? Jason
On Tue, May 19, 2020 at 10:53:52AM -0300, Jason Gunthorpe wrote: > On Tue, May 19, 2020 at 09:43:14AM -0400, Dennis Dalessandro wrote: > > On 5/18/2020 2:10 PM, Jason Gunthorpe wrote: > > > On Mon, May 18, 2020 at 11:20:04AM -0400, Dennis Dalessandro wrote: > > > > On 5/14/2020 8:02 AM, Max Gurtovoy 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. > > > > > > > > > > The tests that I made for this series (fio benchmarks and fio verify data): > > > > > 1. iSER initiator on ConnectX-4 > > > > > 2. iSER initiator on ConnectX-3 > > > > > 3. SRP initiator on ConnectX-4 (loopback to SRP target) > > > > > 4. SRP initiator on ConnectX-3 > > > > > > > > > > Not tested: > > > > > 1. RDS > > > > > 2. mthca > > > > > 3. rdmavt > > > > > > > > This will effectively kill qib which uses rdmavt. It's gonna have to be a > > > > NAK from me. > > > > > > Are you objecting the SRP and iSER changes too? > > > > No, just want to keep basic verbs support at least. NFS already dropped, > > similarly we are ok with dropping it from SRP/iSER as a next step. > > So you see a major user in RDS for qib? Didn't we agree to drop it from RDS too? Thanks > > Jason
On 5/19/2020 10:19 AM, Leon Romanovsky wrote: > On Tue, May 19, 2020 at 10:53:52AM -0300, Jason Gunthorpe wrote: >> On Tue, May 19, 2020 at 09:43:14AM -0400, Dennis Dalessandro wrote: >>> On 5/18/2020 2:10 PM, Jason Gunthorpe wrote: >>>> On Mon, May 18, 2020 at 11:20:04AM -0400, Dennis Dalessandro wrote: >>>>> On 5/14/2020 8:02 AM, Max Gurtovoy 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. >>>>>> >>>>>> The tests that I made for this series (fio benchmarks and fio verify data): >>>>>> 1. iSER initiator on ConnectX-4 >>>>>> 2. iSER initiator on ConnectX-3 >>>>>> 3. SRP initiator on ConnectX-4 (loopback to SRP target) >>>>>> 4. SRP initiator on ConnectX-3 >>>>>> >>>>>> Not tested: >>>>>> 1. RDS >>>>>> 2. mthca >>>>>> 3. rdmavt >>>>> >>>>> This will effectively kill qib which uses rdmavt. It's gonna have to be a >>>>> NAK from me. >>>> >>>> Are you objecting the SRP and iSER changes too? >>> >>> No, just want to keep basic verbs support at least. NFS already dropped, >>> similarly we are ok with dropping it from SRP/iSER as a next step. >> >> So you see a major user in RDS for qib? > > Didn't we agree to drop it from RDS too? > Just basic verbs application support is enough for qib I think. I don't see any major use of RDS. -Denny
On Tue, May 19, 2020 at 10:26:37AM -0400, Dennis Dalessandro wrote: > On 5/19/2020 10:19 AM, Leon Romanovsky wrote: > > On Tue, May 19, 2020 at 10:53:52AM -0300, Jason Gunthorpe wrote: > > > On Tue, May 19, 2020 at 09:43:14AM -0400, Dennis Dalessandro wrote: > > > > On 5/18/2020 2:10 PM, Jason Gunthorpe wrote: > > > > > On Mon, May 18, 2020 at 11:20:04AM -0400, Dennis Dalessandro wrote: > > > > > > On 5/14/2020 8:02 AM, Max Gurtovoy 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. > > > > > > > > > > > > > > The tests that I made for this series (fio benchmarks and fio verify data): > > > > > > > 1. iSER initiator on ConnectX-4 > > > > > > > 2. iSER initiator on ConnectX-3 > > > > > > > 3. SRP initiator on ConnectX-4 (loopback to SRP target) > > > > > > > 4. SRP initiator on ConnectX-3 > > > > > > > > > > > > > > Not tested: > > > > > > > 1. RDS > > > > > > > 2. mthca > > > > > > > 3. rdmavt > > > > > > > > > > > > This will effectively kill qib which uses rdmavt. It's gonna have to be a > > > > > > NAK from me. > > > > > > > > > > Are you objecting the SRP and iSER changes too? > > > > > > > > No, just want to keep basic verbs support at least. NFS already dropped, > > > > similarly we are ok with dropping it from SRP/iSER as a next step. > > > > > > So you see a major user in RDS for qib? > > > > Didn't we agree to drop it from RDS too? > > > > Just basic verbs application support is enough for qib I think. I don't see > any major use of RDS. Well, once the in-kernel users of an API are gone that API will be purged. This is standard kernel policy. So you can't NAK this series on the grounds you want to keep an API without users, presumably for out of tree modules... Jason
On 5/19/2020 10:30 AM, Jason Gunthorpe wrote: > On Tue, May 19, 2020 at 10:26:37AM -0400, Dennis Dalessandro wrote: >> On 5/19/2020 10:19 AM, Leon Romanovsky wrote: >>> On Tue, May 19, 2020 at 10:53:52AM -0300, Jason Gunthorpe wrote: >>>> On Tue, May 19, 2020 at 09:43:14AM -0400, Dennis Dalessandro wrote: >>>>> On 5/18/2020 2:10 PM, Jason Gunthorpe wrote: >>>>>> On Mon, May 18, 2020 at 11:20:04AM -0400, Dennis Dalessandro wrote: >>>>>>> On 5/14/2020 8:02 AM, Max Gurtovoy 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. >>>>>>>> >>>>>>>> The tests that I made for this series (fio benchmarks and fio verify data): >>>>>>>> 1. iSER initiator on ConnectX-4 >>>>>>>> 2. iSER initiator on ConnectX-3 >>>>>>>> 3. SRP initiator on ConnectX-4 (loopback to SRP target) >>>>>>>> 4. SRP initiator on ConnectX-3 >>>>>>>> >>>>>>>> Not tested: >>>>>>>> 1. RDS >>>>>>>> 2. mthca >>>>>>>> 3. rdmavt >>>>>>> >>>>>>> This will effectively kill qib which uses rdmavt. It's gonna have to be a >>>>>>> NAK from me. >>>>>> >>>>>> Are you objecting the SRP and iSER changes too? >>>>> >>>>> No, just want to keep basic verbs support at least. NFS already dropped, >>>>> similarly we are ok with dropping it from SRP/iSER as a next step. >>>> >>>> So you see a major user in RDS for qib? >>> >>> Didn't we agree to drop it from RDS too? >>> >> >> Just basic verbs application support is enough for qib I think. I don't see >> any major use of RDS. > > Well, once the in-kernel users of an API are gone that API will be > purged. This is standard kernel policy. > > So you can't NAK this series on the grounds you want to keep an API > without users, presumably for out of tree modules... > Maybe I need to look at this again. I thought it would kill off user access as well. We don't need any kernel ULPs. -Denny
On Tue, May 19, 2020 at 10:37:07AM -0400, Dennis Dalessandro wrote: > On 5/19/2020 10:30 AM, Jason Gunthorpe wrote: > > On Tue, May 19, 2020 at 10:26:37AM -0400, Dennis Dalessandro wrote: > > > On 5/19/2020 10:19 AM, Leon Romanovsky wrote: > > > > On Tue, May 19, 2020 at 10:53:52AM -0300, Jason Gunthorpe wrote: > > > > > On Tue, May 19, 2020 at 09:43:14AM -0400, Dennis Dalessandro wrote: > > > > > > On 5/18/2020 2:10 PM, Jason Gunthorpe wrote: > > > > > > > On Mon, May 18, 2020 at 11:20:04AM -0400, Dennis Dalessandro wrote: > > > > > > > > On 5/14/2020 8:02 AM, Max Gurtovoy 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. > > > > > > > > > > > > > > > > > > The tests that I made for this series (fio benchmarks and fio verify data): > > > > > > > > > 1. iSER initiator on ConnectX-4 > > > > > > > > > 2. iSER initiator on ConnectX-3 > > > > > > > > > 3. SRP initiator on ConnectX-4 (loopback to SRP target) > > > > > > > > > 4. SRP initiator on ConnectX-3 > > > > > > > > > > > > > > > > > > Not tested: > > > > > > > > > 1. RDS > > > > > > > > > 2. mthca > > > > > > > > > 3. rdmavt > > > > > > > > > > > > > > > > This will effectively kill qib which uses rdmavt. It's gonna have to be a > > > > > > > > NAK from me. > > > > > > > > > > > > > > Are you objecting the SRP and iSER changes too? > > > > > > > > > > > > No, just want to keep basic verbs support at least. NFS already dropped, > > > > > > similarly we are ok with dropping it from SRP/iSER as a next step. > > > > > > > > > > So you see a major user in RDS for qib? > > > > > > > > Didn't we agree to drop it from RDS too? > > > > > > > > > > Just basic verbs application support is enough for qib I think. I don't see > > > any major use of RDS. > > > > Well, once the in-kernel users of an API are gone that API will be > > purged. This is standard kernel policy. > > > > So you can't NAK this series on the grounds you want to keep an API > > without users, presumably for out of tree modules... > > > > Maybe I need to look at this again. I thought it would kill off user access > as well. We don't need any kernel ULPs. Did you make a conclusion? Seems like everyone else is in agreement here, if Max resends a v2 I'm inclined to take it unless RDS objects. I did not think FMR or FRWR were available from userspace at all. Jason
On 5/23/2020 6:08 PM, Jason Gunthorpe wrote: > On Tue, May 19, 2020 at 10:37:07AM -0400, Dennis Dalessandro wrote: >> On 5/19/2020 10:30 AM, Jason Gunthorpe wrote: >>> On Tue, May 19, 2020 at 10:26:37AM -0400, Dennis Dalessandro wrote: >>>> On 5/19/2020 10:19 AM, Leon Romanovsky wrote: >>>>> On Tue, May 19, 2020 at 10:53:52AM -0300, Jason Gunthorpe wrote: >>>>>> On Tue, May 19, 2020 at 09:43:14AM -0400, Dennis Dalessandro wrote: >>>>>>> On 5/18/2020 2:10 PM, Jason Gunthorpe wrote: >>>>>>>> On Mon, May 18, 2020 at 11:20:04AM -0400, Dennis Dalessandro wrote: >>>>>>>>> On 5/14/2020 8:02 AM, Max Gurtovoy 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. >>>>>>>>>> >>>>>>>>>> The tests that I made for this series (fio benchmarks and fio verify data): >>>>>>>>>> 1. iSER initiator on ConnectX-4 >>>>>>>>>> 2. iSER initiator on ConnectX-3 >>>>>>>>>> 3. SRP initiator on ConnectX-4 (loopback to SRP target) >>>>>>>>>> 4. SRP initiator on ConnectX-3 >>>>>>>>>> >>>>>>>>>> Not tested: >>>>>>>>>> 1. RDS >>>>>>>>>> 2. mthca >>>>>>>>>> 3. rdmavt >>>>>>>>> >>>>>>>>> This will effectively kill qib which uses rdmavt. It's gonna have to be a >>>>>>>>> NAK from me. >>>>>>>> >>>>>>>> Are you objecting the SRP and iSER changes too? >>>>>>> >>>>>>> No, just want to keep basic verbs support at least. NFS already dropped, >>>>>>> similarly we are ok with dropping it from SRP/iSER as a next step. >>>>>> >>>>>> So you see a major user in RDS for qib? >>>>> >>>>> Didn't we agree to drop it from RDS too? >>>>> >>>> >>>> Just basic verbs application support is enough for qib I think. I don't see >>>> any major use of RDS. >>> >>> Well, once the in-kernel users of an API are gone that API will be >>> purged. This is standard kernel policy. >>> >>> So you can't NAK this series on the grounds you want to keep an API >>> without users, presumably for out of tree modules... >>> >> >> Maybe I need to look at this again. I thought it would kill off user access >> as well. We don't need any kernel ULPs. > > Did you make a conclusion? Seems like everyone else is in agreement > here, if Max resends a v2 I'm inclined to take it unless RDS objects. > > I did not think FMR or FRWR were available from userspace at all. I certainly hope they're not available from userspace, as they both take physaddrs to reference memory! Tom.
On 5/23/2020 6:08 PM, Jason Gunthorpe wrote: > On Tue, May 19, 2020 at 10:37:07AM -0400, Dennis Dalessandro wrote: >> On 5/19/2020 10:30 AM, Jason Gunthorpe wrote: >>> On Tue, May 19, 2020 at 10:26:37AM -0400, Dennis Dalessandro wrote: >>>> On 5/19/2020 10:19 AM, Leon Romanovsky wrote: >>>>> On Tue, May 19, 2020 at 10:53:52AM -0300, Jason Gunthorpe wrote: >>>>>> On Tue, May 19, 2020 at 09:43:14AM -0400, Dennis Dalessandro wrote: >>>>>>> On 5/18/2020 2:10 PM, Jason Gunthorpe wrote: >>>>>>>> On Mon, May 18, 2020 at 11:20:04AM -0400, Dennis Dalessandro wrote: >>>>>>>>> On 5/14/2020 8:02 AM, Max Gurtovoy 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. >>>>>>>>>> >>>>>>>>>> The tests that I made for this series (fio benchmarks and fio verify data): >>>>>>>>>> 1. iSER initiator on ConnectX-4 >>>>>>>>>> 2. iSER initiator on ConnectX-3 >>>>>>>>>> 3. SRP initiator on ConnectX-4 (loopback to SRP target) >>>>>>>>>> 4. SRP initiator on ConnectX-3 >>>>>>>>>> >>>>>>>>>> Not tested: >>>>>>>>>> 1. RDS >>>>>>>>>> 2. mthca >>>>>>>>>> 3. rdmavt >>>>>>>>> >>>>>>>>> This will effectively kill qib which uses rdmavt. It's gonna have to be a >>>>>>>>> NAK from me. >>>>>>>> >>>>>>>> Are you objecting the SRP and iSER changes too? >>>>>>> >>>>>>> No, just want to keep basic verbs support at least. NFS already dropped, >>>>>>> similarly we are ok with dropping it from SRP/iSER as a next step. >>>>>> >>>>>> So you see a major user in RDS for qib? >>>>> >>>>> Didn't we agree to drop it from RDS too? >>>>> >>>> >>>> Just basic verbs application support is enough for qib I think. I don't see >>>> any major use of RDS. >>> >>> Well, once the in-kernel users of an API are gone that API will be >>> purged. This is standard kernel policy. >>> >>> So you can't NAK this series on the grounds you want to keep an API >>> without users, presumably for out of tree modules... >>> >> >> Maybe I need to look at this again. I thought it would kill off user access >> as well. We don't need any kernel ULPs. > > Did you make a conclusion? Seems like everyone else is in agreement > here, if Max resends a v2 I'm inclined to take it unless RDS objects. > > I did not think FMR or FRWR were available from userspace at all. Yeah, looked it over again and agree it's OK. No issues here now. Thanks for checking. -Denny