| Message ID | 20181009184547.5907-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: Tuesday, October 9, 2018 1:45 PM > To: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca> > Cc: linux-rdma@vger.kernel.org; kamalheib1@gmail.com > Subject: [PATCH rdma-next 00/18] 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. > > Thanks, > Kamal > > Kamal Heib (18): > 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: 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 | 26 +-- > 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 | 97 +++++---- > drivers/infiniband/hw/cxgb3/iwch_provider.c | 64 +++--- > drivers/infiniband/hw/cxgb4/provider.c | 74 +++---- > drivers/infiniband/hw/hfi1/verbs.c | 19 +- > drivers/infiniband/hw/hns/hns_roce_device.h | 1 + > drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 11 ++ > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 11 ++ > drivers/infiniband/hw/hns/hns_roce_main.c | 91 ++++----- > drivers/infiniband/hw/i40iw/i40iw_cm.c | 2 +- > drivers/infiniband/hw/i40iw/i40iw_verbs.c | 66 ++++--- > drivers/infiniband/hw/mlx4/alias_GUID.c | 2 +- > drivers/infiniband/hw/mlx4/main.c | 166 +++++++++------- > drivers/infiniband/hw/mlx5/main.c | 220 ++++++++++++--------- > drivers/infiniband/hw/mthca/mthca_provider.c | 139 ++++++++----- > drivers/infiniband/hw/nes/nes_cm.c | 2 +- > drivers/infiniband/hw/nes/nes_verbs.c | 66 ++++--- > drivers/infiniband/hw/ocrdma/ocrdma_main.c | 92 ++++----- > drivers/infiniband/hw/qedr/main.c | 103 +++++----- > drivers/infiniband/hw/qib/qib_verbs.c | 8 +- > drivers/infiniband/hw/usnic/usnic_ib_main.c | 61 +++--- > drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 82 ++++---- > drivers/infiniband/sw/rdmavt/vt.c | 90 ++++----- > drivers/infiniband/sw/rxe/rxe_verbs.c | 90 +++++---- > 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 +- > include/rdma/ib_verbs.h | 212 ++++++++------------ > net/sunrpc/xprtrdma/fmr_ops.c | 2 +- > 50 files changed, 1257 insertions(+), 1057 deletions(-) > > -- > 2.14.4 I am not sure if this change is needed. But if an ops structure is defined, there should be logical useful group should exist for the ops. Such as 1. datapath_ops (post_send, post_recv, poll_cq, req_notify_cq), which can fall in same cacheline. 2. resource_ops(create, destroy, query, modify of qp, cq, mr, ah, srq) 3. gid_ops(query, add, del) 4. vf_ops(config_get, config_set, stats) 5. stats_ops(alloc, free, get) This aligns to other part of kernel, one example is netdev ops (rtnl, ndo, dcbnl, vf).
On Tue, Oct 09, 2018 at 07:06:56PM +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: Tuesday, October 9, 2018 1:45 PM > > To: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca> > > Cc: linux-rdma@vger.kernel.org; kamalheib1@gmail.com > > Subject: [PATCH rdma-next 00/18] 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. > > > > Thanks, > > Kamal > > > > Kamal Heib (18): > > 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: 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 | 26 +-- > > 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 | 97 +++++---- > > drivers/infiniband/hw/cxgb3/iwch_provider.c | 64 +++--- > > drivers/infiniband/hw/cxgb4/provider.c | 74 +++---- > > drivers/infiniband/hw/hfi1/verbs.c | 19 +- > > drivers/infiniband/hw/hns/hns_roce_device.h | 1 + > > drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 11 ++ > > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 11 ++ > > drivers/infiniband/hw/hns/hns_roce_main.c | 91 ++++----- > > drivers/infiniband/hw/i40iw/i40iw_cm.c | 2 +- > > drivers/infiniband/hw/i40iw/i40iw_verbs.c | 66 ++++--- > > drivers/infiniband/hw/mlx4/alias_GUID.c | 2 +- > > drivers/infiniband/hw/mlx4/main.c | 166 +++++++++------- > > drivers/infiniband/hw/mlx5/main.c | 220 ++++++++++++--------- > > drivers/infiniband/hw/mthca/mthca_provider.c | 139 ++++++++----- > > drivers/infiniband/hw/nes/nes_cm.c | 2 +- > > drivers/infiniband/hw/nes/nes_verbs.c | 66 ++++--- > > drivers/infiniband/hw/ocrdma/ocrdma_main.c | 92 ++++----- > > drivers/infiniband/hw/qedr/main.c | 103 +++++----- > > drivers/infiniband/hw/qib/qib_verbs.c | 8 +- > > drivers/infiniband/hw/usnic/usnic_ib_main.c | 61 +++--- > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 82 ++++---- > > drivers/infiniband/sw/rdmavt/vt.c | 90 ++++----- > > drivers/infiniband/sw/rxe/rxe_verbs.c | 90 +++++---- > > 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 +- > > include/rdma/ib_verbs.h | 212 ++++++++------------ > > net/sunrpc/xprtrdma/fmr_ops.c | 2 +- > > 50 files changed, 1257 insertions(+), 1057 deletions(-) > > > > -- > > 2.14.4 > > I am not sure if this change is needed. The idea behind this change is to make some order in the IB device structure by moving all the IB device operations to one place under the new ib_device_ops structure. > But if an ops structure is defined, there should be logical useful group should exist for the ops. > > Such as > 1. datapath_ops (post_send, post_recv, poll_cq, req_notify_cq), which can fall in same cacheline. > 2. resource_ops(create, destroy, query, modify of qp, cq, mr, ah, srq) > 3. gid_ops(query, add, del) > 4. vf_ops(config_get, config_set, stats) > 5. stats_ops(alloc, free, get) > This aligns to other part of kernel, one example is netdev ops (rtnl, ndo, dcbnl, vf). > > Basiclly, I'm fine with the idea of creating different operations groups based on common logic, but I'm not sure if the netdevice operations is a good example because it includes operations for VF, stats, tunnel, fcoe, fdb and etc. Thanks, Kamal
On Tue, Oct 09, 2018 at 03:26:52PM -0500, Steve Wise wrote: > Hey Kamal, > > What is the justification for these changes? > > Thanks, > > Steve. > Hi Steve, The idea behind this change is to make some order in the IB device structure by moving all the IB device operations to one place under the new ib_device_ops structure. Thanks, Kamal > > On 10/9/2018 1:45 PM, Kamal Heib wrote: > > 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. > > > > Thanks, > > Kamal > > > > Kamal Heib (18): > > 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: 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 | 26 +-- > > 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 | 97 +++++---- > > drivers/infiniband/hw/cxgb3/iwch_provider.c | 64 +++--- > > drivers/infiniband/hw/cxgb4/provider.c | 74 +++---- > > drivers/infiniband/hw/hfi1/verbs.c | 19 +- > > drivers/infiniband/hw/hns/hns_roce_device.h | 1 + > > drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 11 ++ > > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 11 ++ > > drivers/infiniband/hw/hns/hns_roce_main.c | 91 ++++----- > > drivers/infiniband/hw/i40iw/i40iw_cm.c | 2 +- > > drivers/infiniband/hw/i40iw/i40iw_verbs.c | 66 ++++--- > > drivers/infiniband/hw/mlx4/alias_GUID.c | 2 +- > > drivers/infiniband/hw/mlx4/main.c | 166 +++++++++------- > > drivers/infiniband/hw/mlx5/main.c | 220 ++++++++++++--------- > > drivers/infiniband/hw/mthca/mthca_provider.c | 139 ++++++++----- > > drivers/infiniband/hw/nes/nes_cm.c | 2 +- > > drivers/infiniband/hw/nes/nes_verbs.c | 66 ++++--- > > drivers/infiniband/hw/ocrdma/ocrdma_main.c | 92 ++++----- > > drivers/infiniband/hw/qedr/main.c | 103 +++++----- > > drivers/infiniband/hw/qib/qib_verbs.c | 8 +- > > drivers/infiniband/hw/usnic/usnic_ib_main.c | 61 +++--- > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 82 ++++---- > > drivers/infiniband/sw/rdmavt/vt.c | 90 ++++----- > > drivers/infiniband/sw/rxe/rxe_verbs.c | 90 +++++---- > > 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 +- > > include/rdma/ib_verbs.h | 212 ++++++++------------ > > net/sunrpc/xprtrdma/fmr_ops.c | 2 +- > > 50 files changed, 1257 insertions(+), 1057 deletions(-) > > >
On Tue, Oct 09, 2018 at 09:45:29PM +0300, Kamal Heib wrote: > 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. I'm also not sure if this is a real improvement, it was done in userspace because we needed to inject actual dummy handlers that do ENOSUPP and had a complication where the ops were exposed as part of the ABI, but we needed to revise the driver ops. None of these reasons really apply to the kernel.. It is a lot of churn and will hurt backporting.. But it is a little tidier in the drivers. I guess if nobody is opposed it could go forward Jason
> -----Original Message----- > From: Kamal Heib <kamalheib1@gmail.com> > Sent: Wednesday, October 10, 2018 7:17 AM > To: Parav Pandit <parav@mellanox.com> > Cc: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>; > linux-rdma@vger.kernel.org > Subject: Re: [PATCH rdma-next 00/18] RDMA: Add support for ib_device_ops > > On Tue, Oct 09, 2018 at 07:06:56PM +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: Tuesday, October 9, 2018 1:45 PM > > > To: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe > > > <jgg@ziepe.ca> > > > Cc: linux-rdma@vger.kernel.org; kamalheib1@gmail.com > > > Subject: [PATCH rdma-next 00/18] 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. > > > > > > Thanks, > > > Kamal > > > > > > Kamal Heib (18): > > > 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: 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 | 26 +-- > > > 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 | 97 +++++---- > > > drivers/infiniband/hw/cxgb3/iwch_provider.c | 64 +++--- > > > drivers/infiniband/hw/cxgb4/provider.c | 74 +++---- > > > drivers/infiniband/hw/hfi1/verbs.c | 19 +- > > > drivers/infiniband/hw/hns/hns_roce_device.h | 1 + > > > drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 11 ++ > > > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 11 ++ > > > drivers/infiniband/hw/hns/hns_roce_main.c | 91 ++++----- > > > drivers/infiniband/hw/i40iw/i40iw_cm.c | 2 +- > > > drivers/infiniband/hw/i40iw/i40iw_verbs.c | 66 ++++--- > > > drivers/infiniband/hw/mlx4/alias_GUID.c | 2 +- > > > drivers/infiniband/hw/mlx4/main.c | 166 +++++++++------- > > > drivers/infiniband/hw/mlx5/main.c | 220 ++++++++++++--------- > > > drivers/infiniband/hw/mthca/mthca_provider.c | 139 ++++++++----- > > > drivers/infiniband/hw/nes/nes_cm.c | 2 +- > > > drivers/infiniband/hw/nes/nes_verbs.c | 66 ++++--- > > > drivers/infiniband/hw/ocrdma/ocrdma_main.c | 92 ++++----- > > > drivers/infiniband/hw/qedr/main.c | 103 +++++----- > > > drivers/infiniband/hw/qib/qib_verbs.c | 8 +- > > > drivers/infiniband/hw/usnic/usnic_ib_main.c | 61 +++--- > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 82 ++++---- > > > drivers/infiniband/sw/rdmavt/vt.c | 90 ++++----- > > > drivers/infiniband/sw/rxe/rxe_verbs.c | 90 +++++---- > > > 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 +- > > > include/rdma/ib_verbs.h | 212 ++++++++------------ > > > net/sunrpc/xprtrdma/fmr_ops.c | 2 +- > > > 50 files changed, 1257 insertions(+), 1057 deletions(-) > > > > > > -- > > > 2.14.4 > > > > I am not sure if this change is needed. > > The idea behind this change is to make some order in the IB device structure > by moving all the IB device operations to one place under the new > ib_device_ops structure. > > > But if an ops structure is defined, there should be logical useful group > should exist for the ops. > > > > Such as > > 1. datapath_ops (post_send, post_recv, poll_cq, req_notify_cq), which can > fall in same cacheline. > > 2. resource_ops(create, destroy, query, modify of qp, cq, mr, ah, srq) > > 3. gid_ops(query, add, del) 4. vf_ops(config_get, config_set, stats) > > 5. stats_ops(alloc, free, get) This aligns to other part of kernel, > > one example is netdev ops (rtnl, ndo, dcbnl, vf). > > > > > > Basiclly, I'm fine with the idea of creating different operations groups based > on common logic, but I'm not sure if the netdevice operations is a good > example because it includes operations for VF, stats, tunnel, fcoe, fdb and > etc. And similarly we have operations for rdma networking devices too on VFs, stats, resources. At minimum 5 broad logical groups I posted above.
On Tue, Oct 09, 2018 at 09:45:29PM +0300, Kamal Heib wrote: > 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. Nice, I though about doing this for a while. Any reason why you don't just do the switchover in a single patch? In the end that would be a whole lot simpler.
>> 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. > > I'm also not sure if this is a real improvement, it was done in > userspace because we needed to inject actual dummy handlers that do > ENOSUPP and had a complication where the ops were exposed as part of > the ABI, but we needed to revise the driver ops. None of these reasons > really apply to the kernel.. > > It is a lot of churn and will hurt backporting.. But it is a little > tidier in the drivers. > > I guess if nobody is opposed it could go forward I think its OK'ish... I agree with Parav that it would be much more appealing if we could actually sort fast-path vs. slow-path references (which would also move stuff like device name and various lists and locks to live behind it). But that could be done as an incremental patch..