diff mbox series

[rdma-next,v3,02/20] RDMA/bnxt_re: Initialize ib_device_ops struct

Message ID 20181105113528.8317-3-kamalheib1@gmail.com (mailing list archive)
State Superseded
Headers show
Series RDMA: Add support for ib_device_ops | expand

Commit Message

Kamal Heib Nov. 5, 2018, 11:35 a.m. UTC 
Initialize ib_device_ops with the supported operations using
ib_set_device_ops().

Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
---
 drivers/infiniband/hw/bnxt_re/main.c | 107 ++++++++++++++++++-----------------
 1 file changed, 56 insertions(+), 51 deletions(-)

Comments

Jason Gunthorpe Nov. 6, 2018, 3:51 p.m. UTC | #1 
On Mon, Nov 05, 2018 at 01:35:10PM +0200, Kamal Heib wrote:
> Initialize ib_device_ops with the supported operations using
> ib_set_device_ops().
> 
> Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
>  drivers/infiniband/hw/bnxt_re/main.c | 107 ++++++++++++++++++-----------------
>  1 file changed, 56 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index cf2282654210..6e7b3eb75fce 100644
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -568,6 +568,61 @@ static void bnxt_re_unregister_ib(struct bnxt_re_dev *rdev)
>  	ib_unregister_device(&rdev->ibdev);
>  }
>  
> +static const struct ib_device_ops bnxt_re_dev_ops = {
> +	/* Device operations */
> +	.query_device = bnxt_re_query_device,
> +	.modify_device = bnxt_re_modify_device,
> +	.get_dev_fw_str = bnxt_re_query_fw_str,
> +	/* Port operations */
> +	.query_port = bnxt_re_query_port,
> +	.get_port_immutable = bnxt_re_get_port_immutable,
> +	.query_pkey = bnxt_re_query_pkey,
> +	.get_netdev = bnxt_re_get_netdev,
> +	.get_link_layer = bnxt_re_get_link_layer,
> +	/* GID operations */
> +	.add_gid = bnxt_re_add_gid,
> +	.del_gid = bnxt_re_del_gid,
> +	/* PD operations */
> +	.alloc_pd = bnxt_re_alloc_pd,
> +	.dealloc_pd = bnxt_re_dealloc_pd,
> +	/* AH operations */
> +	.create_ah = bnxt_re_create_ah,
> +	.modify_ah = bnxt_re_modify_ah,
> +	.query_ah = bnxt_re_query_ah,
> +	.destroy_ah = bnxt_re_destroy_ah,
> +	/* SRQ operations */
> +	.create_srq = bnxt_re_create_srq,
> +	.modify_srq = bnxt_re_modify_srq,
> +	.query_srq = bnxt_re_query_srq,
> +	.destroy_srq = bnxt_re_destroy_srq,
> +	.post_srq_recv = bnxt_re_post_srq_recv,
> +	/* QP operations */
> +	.create_qp = bnxt_re_create_qp,
> +	.modify_qp = bnxt_re_modify_qp,
> +	.query_qp = bnxt_re_query_qp,
> +	.destroy_qp = bnxt_re_destroy_qp,
> +	.post_send = bnxt_re_post_send,
> +	.post_recv = bnxt_re_post_recv,
> +	/* CQ operations */
> +	.create_cq = bnxt_re_create_cq,
> +	.destroy_cq = bnxt_re_destroy_cq,
> +	.poll_cq = bnxt_re_poll_cq,
> +	.req_notify_cq = bnxt_re_req_notify_cq,
> +	/* MR operations */
> +	.get_dma_mr = bnxt_re_get_dma_mr,
> +	.dereg_mr = bnxt_re_dereg_mr,
> +	.alloc_mr = bnxt_re_alloc_mr,
> +	.map_mr_sg = bnxt_re_map_mr_sg,
> +	.reg_user_mr = bnxt_re_reg_user_mr,
> +	/* Ucontext operations */
> +	.alloc_ucontext = bnxt_re_alloc_ucontext,
> +	.dealloc_ucontext = bnxt_re_dealloc_ucontext,
> +	.mmap = bnxt_re_mmap,
> +	/* Stats operations */
> +	.get_hw_stats = bnxt_re_ib_get_hw_stats,
> +	.alloc_hw_stats = bnxt_re_ib_alloc_hw_stats,
> +};

I think the comment last time was keep sorted, why make this into
something even harder to maintain properly? The comments here add no
value at all.

Don't like.

Jason
Kamal Heib Nov. 6, 2018, 8:03 p.m. UTC | #2 
On Tue, Nov 06, 2018 at 08:51:00AM -0700, Jason Gunthorpe wrote:
> On Mon, Nov 05, 2018 at 01:35:10PM +0200, Kamal Heib wrote:
> > Initialize ib_device_ops with the supported operations using
> > ib_set_device_ops().
> > 
> > Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> >  drivers/infiniband/hw/bnxt_re/main.c | 107 ++++++++++++++++++-----------------
> >  1 file changed, 56 insertions(+), 51 deletions(-)
> > 
> > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > index cf2282654210..6e7b3eb75fce 100644
> > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > @@ -568,6 +568,61 @@ static void bnxt_re_unregister_ib(struct bnxt_re_dev *rdev)
> >  	ib_unregister_device(&rdev->ibdev);
> >  }
> >  
> > +static const struct ib_device_ops bnxt_re_dev_ops = {
> > +	/* Device operations */
> > +	.query_device = bnxt_re_query_device,
> > +	.modify_device = bnxt_re_modify_device,
> > +	.get_dev_fw_str = bnxt_re_query_fw_str,
> > +	/* Port operations */
> > +	.query_port = bnxt_re_query_port,
> > +	.get_port_immutable = bnxt_re_get_port_immutable,
> > +	.query_pkey = bnxt_re_query_pkey,
> > +	.get_netdev = bnxt_re_get_netdev,
> > +	.get_link_layer = bnxt_re_get_link_layer,
> > +	/* GID operations */
> > +	.add_gid = bnxt_re_add_gid,
> > +	.del_gid = bnxt_re_del_gid,
> > +	/* PD operations */
> > +	.alloc_pd = bnxt_re_alloc_pd,
> > +	.dealloc_pd = bnxt_re_dealloc_pd,
> > +	/* AH operations */
> > +	.create_ah = bnxt_re_create_ah,
> > +	.modify_ah = bnxt_re_modify_ah,
> > +	.query_ah = bnxt_re_query_ah,
> > +	.destroy_ah = bnxt_re_destroy_ah,
> > +	/* SRQ operations */
> > +	.create_srq = bnxt_re_create_srq,
> > +	.modify_srq = bnxt_re_modify_srq,
> > +	.query_srq = bnxt_re_query_srq,
> > +	.destroy_srq = bnxt_re_destroy_srq,
> > +	.post_srq_recv = bnxt_re_post_srq_recv,
> > +	/* QP operations */
> > +	.create_qp = bnxt_re_create_qp,
> > +	.modify_qp = bnxt_re_modify_qp,
> > +	.query_qp = bnxt_re_query_qp,
> > +	.destroy_qp = bnxt_re_destroy_qp,
> > +	.post_send = bnxt_re_post_send,
> > +	.post_recv = bnxt_re_post_recv,
> > +	/* CQ operations */
> > +	.create_cq = bnxt_re_create_cq,
> > +	.destroy_cq = bnxt_re_destroy_cq,
> > +	.poll_cq = bnxt_re_poll_cq,
> > +	.req_notify_cq = bnxt_re_req_notify_cq,
> > +	/* MR operations */
> > +	.get_dma_mr = bnxt_re_get_dma_mr,
> > +	.dereg_mr = bnxt_re_dereg_mr,
> > +	.alloc_mr = bnxt_re_alloc_mr,
> > +	.map_mr_sg = bnxt_re_map_mr_sg,
> > +	.reg_user_mr = bnxt_re_reg_user_mr,
> > +	/* Ucontext operations */
> > +	.alloc_ucontext = bnxt_re_alloc_ucontext,
> > +	.dealloc_ucontext = bnxt_re_dealloc_ucontext,
> > +	.mmap = bnxt_re_mmap,
> > +	/* Stats operations */
> > +	.get_hw_stats = bnxt_re_ib_get_hw_stats,
> > +	.alloc_hw_stats = bnxt_re_ib_alloc_hw_stats,
> > +};
> 
> I think the comment last time was keep sorted, why make this into
> something even harder to maintain properly? The comments here add no
> value at all.
> 
> Don't like.
> 
> Jason

I see, so you prefer to sort the operation alphabetically within the providers?

Are you fine with the functionality sort that was done within the ib_device_ops
struct (patch #1)?

Thanks,
Kamal
Jason Gunthorpe Nov. 7, 2018, 10:11 p.m. UTC | #3 
On Tue, Nov 06, 2018 at 10:03:33PM +0200, Kamal Heib wrote:
> On Tue, Nov 06, 2018 at 08:51:00AM -0700, Jason Gunthorpe wrote:
> > On Mon, Nov 05, 2018 at 01:35:10PM +0200, Kamal Heib wrote:
> > > Initialize ib_device_ops with the supported operations using
> > > ib_set_device_ops().
> > > 
> > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> > >  drivers/infiniband/hw/bnxt_re/main.c | 107 ++++++++++++++++++-----------------
> > >  1 file changed, 56 insertions(+), 51 deletions(-)
> > > 
> > > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > > index cf2282654210..6e7b3eb75fce 100644
> > > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > > @@ -568,6 +568,61 @@ static void bnxt_re_unregister_ib(struct bnxt_re_dev *rdev)
> > >  	ib_unregister_device(&rdev->ibdev);
> > >  }
> > >  
> > > +static const struct ib_device_ops bnxt_re_dev_ops = {
> > > +	/* Device operations */
> > > +	.query_device = bnxt_re_query_device,
> > > +	.modify_device = bnxt_re_modify_device,
> > > +	.get_dev_fw_str = bnxt_re_query_fw_str,
> > > +	/* Port operations */
> > > +	.query_port = bnxt_re_query_port,
> > > +	.get_port_immutable = bnxt_re_get_port_immutable,
> > > +	.query_pkey = bnxt_re_query_pkey,
> > > +	.get_netdev = bnxt_re_get_netdev,
> > > +	.get_link_layer = bnxt_re_get_link_layer,
> > > +	/* GID operations */
> > > +	.add_gid = bnxt_re_add_gid,
> > > +	.del_gid = bnxt_re_del_gid,
> > > +	/* PD operations */
> > > +	.alloc_pd = bnxt_re_alloc_pd,
> > > +	.dealloc_pd = bnxt_re_dealloc_pd,
> > > +	/* AH operations */
> > > +	.create_ah = bnxt_re_create_ah,
> > > +	.modify_ah = bnxt_re_modify_ah,
> > > +	.query_ah = bnxt_re_query_ah,
> > > +	.destroy_ah = bnxt_re_destroy_ah,
> > > +	/* SRQ operations */
> > > +	.create_srq = bnxt_re_create_srq,
> > > +	.modify_srq = bnxt_re_modify_srq,
> > > +	.query_srq = bnxt_re_query_srq,
> > > +	.destroy_srq = bnxt_re_destroy_srq,
> > > +	.post_srq_recv = bnxt_re_post_srq_recv,
> > > +	/* QP operations */
> > > +	.create_qp = bnxt_re_create_qp,
> > > +	.modify_qp = bnxt_re_modify_qp,
> > > +	.query_qp = bnxt_re_query_qp,
> > > +	.destroy_qp = bnxt_re_destroy_qp,
> > > +	.post_send = bnxt_re_post_send,
> > > +	.post_recv = bnxt_re_post_recv,
> > > +	/* CQ operations */
> > > +	.create_cq = bnxt_re_create_cq,
> > > +	.destroy_cq = bnxt_re_destroy_cq,
> > > +	.poll_cq = bnxt_re_poll_cq,
> > > +	.req_notify_cq = bnxt_re_req_notify_cq,
> > > +	/* MR operations */
> > > +	.get_dma_mr = bnxt_re_get_dma_mr,
> > > +	.dereg_mr = bnxt_re_dereg_mr,
> > > +	.alloc_mr = bnxt_re_alloc_mr,
> > > +	.map_mr_sg = bnxt_re_map_mr_sg,
> > > +	.reg_user_mr = bnxt_re_reg_user_mr,
> > > +	/* Ucontext operations */
> > > +	.alloc_ucontext = bnxt_re_alloc_ucontext,
> > > +	.dealloc_ucontext = bnxt_re_dealloc_ucontext,
> > > +	.mmap = bnxt_re_mmap,
> > > +	/* Stats operations */
> > > +	.get_hw_stats = bnxt_re_ib_get_hw_stats,
> > > +	.alloc_hw_stats = bnxt_re_ib_alloc_hw_stats,
> > > +};
> > 
> > I think the comment last time was keep sorted, why make this into
> > something even harder to maintain properly? The comments here add no
> > value at all.
> > 
> > Don't like.
> > 
> > Jason
> 
> I see, so you prefer to sort the operation alphabetically within the providers?
> 
> Are you fine with the functionality sort that was done within the ib_device_ops
> struct (patch #1)?

I'm sort of 'meh' on it.

If you want to sort for performance it needs to be usage driven, not
something arbitary like by category.

Ie there is no reason to put infrequent control plane stuff like
destroy_qp in the same cacheline as post_send/post_recv - there are
surely better datapath things that could go in that hot cacheline,
like poll_cq for instance.

Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index cf2282654210..6e7b3eb75fce 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -568,6 +568,61 @@  static void bnxt_re_unregister_ib(struct bnxt_re_dev *rdev)
 	ib_unregister_device(&rdev->ibdev);
 }
 
+static const struct ib_device_ops bnxt_re_dev_ops = {
+	/* Device operations */
+	.query_device = bnxt_re_query_device,
+	.modify_device = bnxt_re_modify_device,
+	.get_dev_fw_str = bnxt_re_query_fw_str,
+	/* Port operations */
+	.query_port = bnxt_re_query_port,
+	.get_port_immutable = bnxt_re_get_port_immutable,
+	.query_pkey = bnxt_re_query_pkey,
+	.get_netdev = bnxt_re_get_netdev,
+	.get_link_layer = bnxt_re_get_link_layer,
+	/* GID operations */
+	.add_gid = bnxt_re_add_gid,
+	.del_gid = bnxt_re_del_gid,
+	/* PD operations */
+	.alloc_pd = bnxt_re_alloc_pd,
+	.dealloc_pd = bnxt_re_dealloc_pd,
+	/* AH operations */
+	.create_ah = bnxt_re_create_ah,
+	.modify_ah = bnxt_re_modify_ah,
+	.query_ah = bnxt_re_query_ah,
+	.destroy_ah = bnxt_re_destroy_ah,
+	/* SRQ operations */
+	.create_srq = bnxt_re_create_srq,
+	.modify_srq = bnxt_re_modify_srq,
+	.query_srq = bnxt_re_query_srq,
+	.destroy_srq = bnxt_re_destroy_srq,
+	.post_srq_recv = bnxt_re_post_srq_recv,
+	/* QP operations */
+	.create_qp = bnxt_re_create_qp,
+	.modify_qp = bnxt_re_modify_qp,
+	.query_qp = bnxt_re_query_qp,
+	.destroy_qp = bnxt_re_destroy_qp,
+	.post_send = bnxt_re_post_send,
+	.post_recv = bnxt_re_post_recv,
+	/* CQ operations */
+	.create_cq = bnxt_re_create_cq,
+	.destroy_cq = bnxt_re_destroy_cq,
+	.poll_cq = bnxt_re_poll_cq,
+	.req_notify_cq = bnxt_re_req_notify_cq,
+	/* MR operations */
+	.get_dma_mr = bnxt_re_get_dma_mr,
+	.dereg_mr = bnxt_re_dereg_mr,
+	.alloc_mr = bnxt_re_alloc_mr,
+	.map_mr_sg = bnxt_re_map_mr_sg,
+	.reg_user_mr = bnxt_re_reg_user_mr,
+	/* Ucontext operations */
+	.alloc_ucontext = bnxt_re_alloc_ucontext,
+	.dealloc_ucontext = bnxt_re_dealloc_ucontext,
+	.mmap = bnxt_re_mmap,
+	/* Stats operations */
+	.get_hw_stats = bnxt_re_ib_get_hw_stats,
+	.alloc_hw_stats = bnxt_re_ib_alloc_hw_stats,
+};
+
 static int bnxt_re_register_ib(struct bnxt_re_dev *rdev)
 {
 	struct ib_device *ibdev = &rdev->ibdev;
@@ -614,60 +669,10 @@  static int bnxt_re_register_ib(struct bnxt_re_dev *rdev)
 			(1ull << IB_USER_VERBS_CMD_DESTROY_AH);
 	/* POLL_CQ and REQ_NOTIFY_CQ is directly handled in libbnxt_re */
 
-	/* Kernel verbs */
-	ibdev->query_device		= bnxt_re_query_device;
-	ibdev->modify_device		= bnxt_re_modify_device;
-
-	ibdev->query_port		= bnxt_re_query_port;
-	ibdev->get_port_immutable	= bnxt_re_get_port_immutable;
-	ibdev->get_dev_fw_str           = bnxt_re_query_fw_str;
-	ibdev->query_pkey		= bnxt_re_query_pkey;
-	ibdev->get_netdev		= bnxt_re_get_netdev;
-	ibdev->add_gid			= bnxt_re_add_gid;
-	ibdev->del_gid			= bnxt_re_del_gid;
-	ibdev->get_link_layer		= bnxt_re_get_link_layer;
-
-	ibdev->alloc_pd			= bnxt_re_alloc_pd;
-	ibdev->dealloc_pd		= bnxt_re_dealloc_pd;
-
-	ibdev->create_ah		= bnxt_re_create_ah;
-	ibdev->modify_ah		= bnxt_re_modify_ah;
-	ibdev->query_ah			= bnxt_re_query_ah;
-	ibdev->destroy_ah		= bnxt_re_destroy_ah;
-
-	ibdev->create_srq		= bnxt_re_create_srq;
-	ibdev->modify_srq		= bnxt_re_modify_srq;
-	ibdev->query_srq		= bnxt_re_query_srq;
-	ibdev->destroy_srq		= bnxt_re_destroy_srq;
-	ibdev->post_srq_recv		= bnxt_re_post_srq_recv;
-
-	ibdev->create_qp		= bnxt_re_create_qp;
-	ibdev->modify_qp		= bnxt_re_modify_qp;
-	ibdev->query_qp			= bnxt_re_query_qp;
-	ibdev->destroy_qp		= bnxt_re_destroy_qp;
-
-	ibdev->post_send		= bnxt_re_post_send;
-	ibdev->post_recv		= bnxt_re_post_recv;
-
-	ibdev->create_cq		= bnxt_re_create_cq;
-	ibdev->destroy_cq		= bnxt_re_destroy_cq;
-	ibdev->poll_cq			= bnxt_re_poll_cq;
-	ibdev->req_notify_cq		= bnxt_re_req_notify_cq;
-
-	ibdev->get_dma_mr		= bnxt_re_get_dma_mr;
-	ibdev->dereg_mr			= bnxt_re_dereg_mr;
-	ibdev->alloc_mr			= bnxt_re_alloc_mr;
-	ibdev->map_mr_sg		= bnxt_re_map_mr_sg;
-
-	ibdev->reg_user_mr		= bnxt_re_reg_user_mr;
-	ibdev->alloc_ucontext		= bnxt_re_alloc_ucontext;
-	ibdev->dealloc_ucontext		= bnxt_re_dealloc_ucontext;
-	ibdev->mmap			= bnxt_re_mmap;
-	ibdev->get_hw_stats             = bnxt_re_ib_get_hw_stats;
-	ibdev->alloc_hw_stats           = bnxt_re_ib_alloc_hw_stats;
 
 	rdma_set_device_sysfs_group(ibdev, &bnxt_re_dev_attr_group);
 	ibdev->driver_id = RDMA_DRIVER_BNXT_RE;
+	ib_set_device_ops(ibdev, &bnxt_re_dev_ops);
 	return ib_register_device(ibdev, "bnxt_re%d", NULL);
 }