| Message ID | 20181105113528.8317-1-kamalheib1@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | RDMA: Add support for ib_device_ops | expand |
> -----Original Message----- > From: linux-rdma-owner@vger.kernel.org <linux-rdma- > owner@vger.kernel.org> On Behalf Of Kamal Heib > Sent: Monday, November 5, 2018 5:35 AM > To: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca> > Cc: linux-rdma@vger.kernel.org; kamalheib1@gmail.com > Subject: [PATCH rdma-next v3 00/20] RDMA: Add support for ib_device_ops > > This patchset introduce a new structure that will contain all the infiniband > device operations, the structure will be used by the providers to initialize > their supported operations. This patchset also includes the required changes > in the core and ulps to start using it. > > Changes from v2: > * Sort the operations based on logical groups. > * Add patch 18 for fixing rvt_create_ah prototype. > * Add patch 19 for initialize ib_device_ops struct. > > Changes from V1: > * Rebase. > * Constify the ib_device_ops. > * Remove vertical white spaces. > * Clang-format ib_device_ops struct. > * Return the descriptive comments into ib_device_ops struct. > * Move the assignment of the ib_device operations to the last patch in > the patchset. > * Remove the existing method assignment at the same time of initializing > the ib_device_ops. > > Thanks, > Kamal > > Kamal Heib (20): > RDMA/core: Introduce ib_device_ops > RDMA/bnxt_re: Initialize ib_device_ops struct > RDMA/cxgb3: Initialize ib_device_ops struct > RDMA/cxgb4: Initialize ib_device_ops struct > RDMA/hfi1: Initialize ib_device_ops struct > RDMA/hns: Initialize ib_device_ops struct > RDMA/i40iw: Initialize ib_device_ops struct > RDMA/mlx4: Initialize ib_device_ops struct > RDMA/mlx5: Initialize ib_device_ops struct > RDMA/mthca: Initialize ib_device_ops struct > RDMA/nes: Initialize ib_device_ops struct > RDMA/ocrdma: Initialize ib_device_ops struct > RDMA/qedr: Initialize ib_device_ops struct > RDMA/qib: Initialize ib_device_ops struct > RDMA/usnic: Initialize ib_device_ops struct > RDMA/vmw_pvrdma: Initialize ib_device_ops struct > RDMA/rxe: Initialize ib_device_ops struct > RDMA/rdmavt: Fix rvt_create_ah prototype > RDMA/rdmavt: Initialize ib_device_ops struct > RDMA: Start use ib_device_ops > > drivers/infiniband/core/cache.c | 12 +- > drivers/infiniband/core/core_priv.h | 12 +- > drivers/infiniband/core/cq.c | 6 +- > drivers/infiniband/core/device.c | 136 ++++- > drivers/infiniband/core/fmr_pool.c | 4 +- > drivers/infiniband/core/mad.c | 24 +- > drivers/infiniband/core/nldev.c | 4 +- > drivers/infiniband/core/opa_smi.h | 4 +- > drivers/infiniband/core/rdma_core.c | 6 +- > drivers/infiniband/core/security.c | 8 +- > drivers/infiniband/core/smi.h | 4 +- > drivers/infiniband/core/sysfs.c | 28 +- > drivers/infiniband/core/ucm.c | 2 +- > drivers/infiniband/core/uverbs_cmd.c | 64 +-- > drivers/infiniband/core/uverbs_main.c | 14 +- > drivers/infiniband/core/uverbs_std_types.c | 2 +- > .../infiniband/core/uverbs_std_types_counters.c | 10 +- > drivers/infiniband/core/uverbs_std_types_cq.c | 4 +- > drivers/infiniband/core/uverbs_std_types_dm.c | 6 +- > .../infiniband/core/uverbs_std_types_flow_action.c | 14 +- > drivers/infiniband/core/uverbs_std_types_mr.c | 4 +- > drivers/infiniband/core/verbs.c | 149 +++--- > drivers/infiniband/hw/bnxt_re/main.c | 107 ++-- > drivers/infiniband/hw/cxgb3/iwch_provider.c | 75 +-- > drivers/infiniband/hw/cxgb4/provider.c | 86 ++-- > drivers/infiniband/hw/hfi1/verbs.c | 23 +- > drivers/infiniband/hw/hns/hns_roce_device.h | 1 + > drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 13 + > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 13 + > drivers/infiniband/hw/hns/hns_roce_main.c | 112 +++-- > drivers/infiniband/hw/i40iw/i40iw_cm.c | 2 +- > drivers/infiniband/hw/i40iw/i40iw_verbs.c | 76 +-- > drivers/infiniband/hw/mlx4/alias_GUID.c | 2 +- > drivers/infiniband/hw/mlx4/main.c | 191 ++++--- > drivers/infiniband/hw/mlx5/main.c | 237 +++++---- > drivers/infiniband/hw/mthca/mthca_provider.c | 151 ++++-- > drivers/infiniband/hw/nes/nes_cm.c | 2 +- > drivers/infiniband/hw/nes/nes_verbs.c | 77 +-- > drivers/infiniband/hw/ocrdma/ocrdma_main.c | 102 ++-- > drivers/infiniband/hw/qedr/main.c | 114 +++-- > drivers/infiniband/hw/qib/qib_verbs.c | 10 +- > drivers/infiniband/hw/usnic/usnic_ib_main.c | 71 +-- > drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 92 ++-- > drivers/infiniband/sw/rdmavt/ah.c | 4 +- > drivers/infiniband/sw/rdmavt/ah.h | 3 +- > drivers/infiniband/sw/rdmavt/vt.c | 312 +++--------- > drivers/infiniband/sw/rxe/rxe_verbs.c | 102 ++-- > drivers/infiniband/ulp/ipoib/ipoib_main.c | 12 +- > drivers/infiniband/ulp/iser/iser_memory.c | 4 +- > drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c | 8 +- > drivers/infiniband/ulp/srp/ib_srp.c | 6 +- > fs/cifs/smbdirect.c | 2 +- > include/rdma/ib_verbs.h | 546 ++++++++++----------- > net/rds/ib.c | 4 +- > net/sunrpc/xprtrdma/fmr_ops.c | 2 +- > 55 files changed, 1654 insertions(+), 1425 deletions(-) > > -- > 2.14.5 Sorry for the late comment for Jason's point in v2 series to have one giant structure. With one giant structure, fill_res_entry() provider callback function of rdma_restrack_root doesn't deserve to be inside a logically well separate structure rdma_restrack_root. We should take out fill_res_entry() and put into ib_device_ops. But I like the approach of rdma_restrack_root which keeps all logical entities together. And with that good example I continue to propose same logical separate for rest of the callbacks too. With that non hot control path routines can possibly stay outside of ib_device apart from code readability.
On Tue, Nov 06, 2018 at 05:27:25PM +0000, Parav Pandit wrote: > > > > -----Original Message----- > > From: linux-rdma-owner@vger.kernel.org <linux-rdma- > > owner@vger.kernel.org> On Behalf Of Kamal Heib > > Sent: Monday, November 5, 2018 5:35 AM > > To: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca> > > Cc: linux-rdma@vger.kernel.org; kamalheib1@gmail.com > > Subject: [PATCH rdma-next v3 00/20] RDMA: Add support for ib_device_ops > > > > This patchset introduce a new structure that will contain all the infiniband > > device operations, the structure will be used by the providers to initialize > > their supported operations. This patchset also includes the required changes > > in the core and ulps to start using it. > > > > Changes from v2: > > * Sort the operations based on logical groups. > > * Add patch 18 for fixing rvt_create_ah prototype. > > * Add patch 19 for initialize ib_device_ops struct. > > > > Changes from V1: > > * Rebase. > > * Constify the ib_device_ops. > > * Remove vertical white spaces. > > * Clang-format ib_device_ops struct. > > * Return the descriptive comments into ib_device_ops struct. > > * Move the assignment of the ib_device operations to the last patch in > > the patchset. > > * Remove the existing method assignment at the same time of initializing > > the ib_device_ops. > > > > Thanks, > > Kamal > > > > Kamal Heib (20): > > RDMA/core: Introduce ib_device_ops > > RDMA/bnxt_re: Initialize ib_device_ops struct > > RDMA/cxgb3: Initialize ib_device_ops struct > > RDMA/cxgb4: Initialize ib_device_ops struct > > RDMA/hfi1: Initialize ib_device_ops struct > > RDMA/hns: Initialize ib_device_ops struct > > RDMA/i40iw: Initialize ib_device_ops struct > > RDMA/mlx4: Initialize ib_device_ops struct > > RDMA/mlx5: Initialize ib_device_ops struct > > RDMA/mthca: Initialize ib_device_ops struct > > RDMA/nes: Initialize ib_device_ops struct > > RDMA/ocrdma: Initialize ib_device_ops struct > > RDMA/qedr: Initialize ib_device_ops struct > > RDMA/qib: Initialize ib_device_ops struct > > RDMA/usnic: Initialize ib_device_ops struct > > RDMA/vmw_pvrdma: Initialize ib_device_ops struct > > RDMA/rxe: Initialize ib_device_ops struct > > RDMA/rdmavt: Fix rvt_create_ah prototype > > RDMA/rdmavt: Initialize ib_device_ops struct > > RDMA: Start use ib_device_ops > > > > drivers/infiniband/core/cache.c | 12 +- > > drivers/infiniband/core/core_priv.h | 12 +- > > drivers/infiniband/core/cq.c | 6 +- > > drivers/infiniband/core/device.c | 136 ++++- > > drivers/infiniband/core/fmr_pool.c | 4 +- > > drivers/infiniband/core/mad.c | 24 +- > > drivers/infiniband/core/nldev.c | 4 +- > > drivers/infiniband/core/opa_smi.h | 4 +- > > drivers/infiniband/core/rdma_core.c | 6 +- > > drivers/infiniband/core/security.c | 8 +- > > drivers/infiniband/core/smi.h | 4 +- > > drivers/infiniband/core/sysfs.c | 28 +- > > drivers/infiniband/core/ucm.c | 2 +- > > drivers/infiniband/core/uverbs_cmd.c | 64 +-- > > drivers/infiniband/core/uverbs_main.c | 14 +- > > drivers/infiniband/core/uverbs_std_types.c | 2 +- > > .../infiniband/core/uverbs_std_types_counters.c | 10 +- > > drivers/infiniband/core/uverbs_std_types_cq.c | 4 +- > > drivers/infiniband/core/uverbs_std_types_dm.c | 6 +- > > .../infiniband/core/uverbs_std_types_flow_action.c | 14 +- > > drivers/infiniband/core/uverbs_std_types_mr.c | 4 +- > > drivers/infiniband/core/verbs.c | 149 +++--- > > drivers/infiniband/hw/bnxt_re/main.c | 107 ++-- > > drivers/infiniband/hw/cxgb3/iwch_provider.c | 75 +-- > > drivers/infiniband/hw/cxgb4/provider.c | 86 ++-- > > drivers/infiniband/hw/hfi1/verbs.c | 23 +- > > drivers/infiniband/hw/hns/hns_roce_device.h | 1 + > > drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 13 + > > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 13 + > > drivers/infiniband/hw/hns/hns_roce_main.c | 112 +++-- > > drivers/infiniband/hw/i40iw/i40iw_cm.c | 2 +- > > drivers/infiniband/hw/i40iw/i40iw_verbs.c | 76 +-- > > drivers/infiniband/hw/mlx4/alias_GUID.c | 2 +- > > drivers/infiniband/hw/mlx4/main.c | 191 ++++--- > > drivers/infiniband/hw/mlx5/main.c | 237 +++++---- > > drivers/infiniband/hw/mthca/mthca_provider.c | 151 ++++-- > > drivers/infiniband/hw/nes/nes_cm.c | 2 +- > > drivers/infiniband/hw/nes/nes_verbs.c | 77 +-- > > drivers/infiniband/hw/ocrdma/ocrdma_main.c | 102 ++-- > > drivers/infiniband/hw/qedr/main.c | 114 +++-- > > drivers/infiniband/hw/qib/qib_verbs.c | 10 +- > > drivers/infiniband/hw/usnic/usnic_ib_main.c | 71 +-- > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 92 ++-- > > drivers/infiniband/sw/rdmavt/ah.c | 4 +- > > drivers/infiniband/sw/rdmavt/ah.h | 3 +- > > drivers/infiniband/sw/rdmavt/vt.c | 312 +++--------- > > drivers/infiniband/sw/rxe/rxe_verbs.c | 102 ++-- > > drivers/infiniband/ulp/ipoib/ipoib_main.c | 12 +- > > drivers/infiniband/ulp/iser/iser_memory.c | 4 +- > > drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c | 8 +- > > drivers/infiniband/ulp/srp/ib_srp.c | 6 +- > > fs/cifs/smbdirect.c | 2 +- > > include/rdma/ib_verbs.h | 546 ++++++++++----------- > > net/rds/ib.c | 4 +- > > net/sunrpc/xprtrdma/fmr_ops.c | 2 +- > > 55 files changed, 1654 insertions(+), 1425 deletions(-) > > > > -- > > 2.14.5 > > Sorry for the late comment for Jason's point in v2 series to have one giant structure. > > With one giant structure, > fill_res_entry() provider callback function of rdma_restrack_root doesn't deserve to be inside a logically well separate structure rdma_restrack_root. > We should take out fill_res_entry() and put into ib_device_ops. > > But I like the approach of rdma_restrack_root which keeps all logical entities together. > And with that good example I continue to propose same logical separate for rest of the callbacks too. > With that non hot control path routines can possibly stay outside of ib_device apart from code readability. Second for that, It will also give us a chance to optimize for cache alignment for such paths. Plus review will be much cleaner, we will be able to easily spot the need of performance numbers for changes in those flows. Thanks > >
On Tue, Nov 06, 2018 at 08:41:54PM +0200, Leon Romanovsky wrote: > > Sorry for the late comment for Jason's point in v2 series to have > > one giant structure. > > > > With one giant structure, fill_res_entry() provider callback > > function of rdma_restrack_root doesn't deserve to be inside a > > logically well separate structure rdma_restrack_root. We should > > take out fill_res_entry() and put into ib_device_ops. This is possibly true.. It is cleaner for drivers if they don't have to reach into all sorts of different places to put function pointers. > > But I like the approach of rdma_restrack_root which keeps all > > logical entities together. And with that good example I continue > > to propose same logical separate for rest of the callbacks too. > > With that non hot control path routines can possibly stay outside > > of ib_device apart from code readability. > > Second for that, > It will also give us a chance to optimize for cache alignment for such > paths. As long as the function pointers are stored in memory allocated as part of struct ib_device, adding more structs will not help performance. The usual approach of sorting hot data together into common cache lines is appropriate here. Kamal sorted by functionality, which is not necessarily great, it would be better to sort based on some profiling, but our prior list wasn't optimized either.. > Plus review will be much cleaner, we will be able to easily spot the > need of performance numbers for changes in those flows. I think having micro structures would make a huge mess of the drivers. Every driver would have to individually split all the ops (across all the variations) into a huge number of micro structs and juggle all of that. So unnecessary and complicated.. The only value I see in this series is that it makes the drivers much simpler. I would not throw that way for micro-structures.. Jason
> -----Original Message----- > From: linux-rdma-owner@vger.kernel.org <linux-rdma- > owner@vger.kernel.org> On Behalf Of Jason Gunthorpe > Sent: Tuesday, November 6, 2018 1:37 PM > To: Leon Romanovsky <leon@kernel.org> > Cc: Parav Pandit <parav@mellanox.com>; Kamal Heib > <kamalheib1@gmail.com>; Doug Ledford <dledford@redhat.com>; linux- > rdma@vger.kernel.org > Subject: Re: [PATCH rdma-next v3 00/20] RDMA: Add support for > ib_device_ops > > On Tue, Nov 06, 2018 at 08:41:54PM +0200, Leon Romanovsky wrote: > > > > Sorry for the late comment for Jason's point in v2 series to have > > > one giant structure. > > > > > > With one giant structure, fill_res_entry() provider callback > > > function of rdma_restrack_root doesn't deserve to be inside a > > > logically well separate structure rdma_restrack_root. We should > > > take out fill_res_entry() and put into ib_device_ops. > > This is possibly true.. It is cleaner for drivers if they don't have to reach into > all sorts of different places to put function pointers. > :-) those all sorts of places are logical places. > > > But I like the approach of rdma_restrack_root which keeps all > > > logical entities together. And with that good example I continue to > > > propose same logical separate for rest of the callbacks too. > > > With that non hot control path routines can possibly stay outside of > > > ib_device apart from code readability. > > > > Second for that, > > It will also give us a chance to optimize for cache alignment for such > > paths. > > As long as the function pointers are stored in memory allocated as part of > struct ib_device, adding more structs will not help performance. The usual > approach of sorting hot data together into common cache lines is > appropriate here. That is the point, the whole structure doesn't stay inside the ib_device. It should be something similar to netdev's, const struct dcbnl_rtnl_ops *dcbnl_ops; const struct rtnl_link_ops *rtnl_link_ops; Fairly large number of function pointers will move out for vfs, stats, gids etc. > > Kamal sorted by functionality, which is not necessarily great, it would be > better to sort based on some profiling, but our prior list wasn't optimized > either.. > > > Plus review will be much cleaner, we will be able to easily spot the > > need of performance numbers for changes in those flows. > > I think having micro structures would make a huge mess of the drivers. > > Every driver would have to individually split all the ops (across all the > variations) into a huge number of micro structs and juggle all of that. So > unnecessary and complicated.. > Most of them will go as const. For hns driver there are several instances of ib_device_ops created just for 2 functions in it to make use of this new API. Micro structures would be equally clean though after looking at hns driver multiple structures, but I am fine if you want to continue with just one structure. > The only value I see in this series is that it makes the drivers much simpler. I > would not throw that way for micro-structures.. > > Jason
On Tue, Nov 06, 2018 at 08:55:25PM +0000, Parav Pandit wrote: > > On Tue, Nov 06, 2018 at 08:41:54PM +0200, Leon Romanovsky wrote: > > > > > > Sorry for the late comment for Jason's point in v2 series to have > > > > one giant structure. > > > > > > > > With one giant structure, fill_res_entry() provider callback > > > > function of rdma_restrack_root doesn't deserve to be inside a > > > > logically well separate structure rdma_restrack_root. We should > > > > take out fill_res_entry() and put into ib_device_ops. > > > > This is possibly true.. It is cleaner for drivers if they don't have to reach into > > all sorts of different places to put function pointers. > > > :-) those all sorts of places are logical places. > > > > > But I like the approach of rdma_restrack_root which keeps all > > > > logical entities together. And with that good example I continue to > > > > propose same logical separate for rest of the callbacks too. > > > > With that non hot control path routines can possibly stay outside of > > > > ib_device apart from code readability. > > > > > > Second for that, > > > It will also give us a chance to optimize for cache alignment for such > > > paths. > > > > As long as the function pointers are stored in memory allocated as part of > > struct ib_device, adding more structs will not help performance. The usual > > approach of sorting hot data together into common cache lines is > > appropriate here. > That is the point, the whole structure doesn't stay inside the ib_device. > > It should be something similar to netdev's, > const struct dcbnl_rtnl_ops *dcbnl_ops; > const struct rtnl_link_ops *rtnl_link_ops; > > Fairly large number of function pointers will move out for vfs, stats, gids etc. The size of the struct doesn't matter, the cacheline layout does, and as long as the user of the ops can reasonably be expected to be touching cachelines in the ib_device, it doesn't really matter. Adding more indirection to outside tables is more likely to make things slower, as now we have to touch more cachelines to get the same thing. I don't think any real performance claim can be made either way. > > > Plus review will be much cleaner, we will be able to easily spot the > > > need of performance numbers for changes in those flows. > > > > I think having micro structures would make a huge mess of the drivers. > > > > Every driver would have to individually split all the ops (across all the > > variations) into a huge number of micro structs and juggle all of that. So > > unnecessary and complicated.. > > Most of them will go as const. This series already has const structs.. > For hns driver there are several instances of ib_device_ops created > just for 2 functions in it to make use of this new API. Yes, that is unfortunate, but that is still less structs over all then if there were micro structs. > Micro structures would be equally clean though after looking at hns > driver multiple structures, but I am fine if you want to continue > with just one structure. And only hns really had this problem badly, other drivers had largely a single structs. But yes, this waste is something I didn't like too much. Again, the value here is to make the drivers simpler, making the drivers complicated again to juggle these structs seems bad. Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Wednesday, November 7, 2018 2:08 PM > To: Parav Pandit <parav@mellanox.com> > Cc: Leon Romanovsky <leon@kernel.org>; Kamal Heib > <kamalheib1@gmail.com>; Doug Ledford <dledford@redhat.com>; linux- > rdma@vger.kernel.org > Subject: Re: [PATCH rdma-next v3 00/20] RDMA: Add support for > ib_device_ops > > On Tue, Nov 06, 2018 at 08:55:25PM +0000, Parav Pandit wrote: > > > On Tue, Nov 06, 2018 at 08:41:54PM +0200, Leon Romanovsky wrote: > > > > > > > > Sorry for the late comment for Jason's point in v2 series to > > > > > have one giant structure. > > > > > > > > > > With one giant structure, fill_res_entry() provider callback > > > > > function of rdma_restrack_root doesn't deserve to be inside a > > > > > logically well separate structure rdma_restrack_root. We should > > > > > take out fill_res_entry() and put into ib_device_ops. > > > > > > This is possibly true.. It is cleaner for drivers if they don't have > > > to reach into all sorts of different places to put function pointers. > > > > > :-) those all sorts of places are logical places. > > > > > > > But I like the approach of rdma_restrack_root which keeps all > > > > > logical entities together. And with that good example I > > > > > continue to propose same logical separate for rest of the callbacks > too. > > > > > With that non hot control path routines can possibly stay > > > > > outside of ib_device apart from code readability. > > > > > > > > Second for that, > > > > It will also give us a chance to optimize for cache alignment for > > > > such paths. > > > > > > As long as the function pointers are stored in memory allocated as > > > part of struct ib_device, adding more structs will not help > > > performance. The usual approach of sorting hot data together into > > > common cache lines is appropriate here. > > That is the point, the whole structure doesn't stay inside the ib_device. > > > > It should be something similar to netdev's, const struct > > dcbnl_rtnl_ops *dcbnl_ops; const struct rtnl_link_ops *rtnl_link_ops; > > > > Fairly large number of function pointers will move out for vfs, stats, gids > etc. > > The size of the struct doesn't matter, the cacheline layout does, and as long > as the user of the ops can reasonably be expected to be touching cachelines > in the ib_device, it doesn't really matter. Adding more indirection to outside > tables is more likely to make things slower, as now we have to touch more > cachelines to get the same thing. > Indirection table only for non-data-path functions, rest will continue to be part of _ops. > I don't think any real performance claim can be made either way. > > > > > Plus review will be much cleaner, we will be able to easily spot the > > > > need of performance numbers for changes in those flows. > > > > > > I think having micro structures would make a huge mess of the drivers. > > > > > > Every driver would have to individually split all the ops (across > > > all the > > > variations) into a huge number of micro structs and juggle all of > > > that. So unnecessary and complicated.. > > > > Most of them will go as const. > > This series already has const structs.. > > > For hns driver there are several instances of ib_device_ops created > > just for 2 functions in it to make use of this new API. > > Yes, that is unfortunate, but that is still less structs over all then if there > were micro structs. > > > Micro structures would be equally clean though after looking at hns > > driver multiple structures, but I am fine if you want to continue with > > just one structure. > > And only hns really had this problem badly, other drivers had largely a single > structs. But yes, this waste is something I didn't like too much. > > Again, the value here is to make the drivers simpler, making the drivers > complicated again to juggle these structs seems bad. > Ok. thanks.