RE: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> -Original Message- > From: Roland Dreier [mailto:rol...@purestorage.com] > Sent: Thursday, March 22, 2012 9:30 PM > To: Pandit, Parav > Cc: sean.he...@intel.com; linux-rdma@vger.kernel.org > Subject: Re: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA > adapter > > On Thu, Mar 22, 2012 at 7:27 AM, wrote: > > I prefer to have it in this first patch and once the internal test cycle is > > finish > (with removed code) in few days, will submit the separate patch? > > It's fine to leave some of these items for future cleanups post-merge. > Just please try to get to the cleanups someday unlike some other drivers... Sure. Item is list and tracking the same. Mostly likely it's going to be 2nd patch after this first release before adding any new features. > (*cough* nes *cough*) > > - R. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Thu, Mar 22, 2012 at 7:27 AM, wrote: > I prefer to have it in this first patch and once the internal test cycle is > finish (with removed code) in few days, will submit the separate patch? It's fine to leave some of these items for future cleanups post-merge. Just please try to get to the cleanups someday unlike some other drivers... (*cough* nes *cough*) - R. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> -Original Message-
> From: Hefty, Sean [mailto:sean.he...@intel.com]
> Sent: Wednesday, March 21, 2012 10:49 PM
> To: Pandit, Parav; linux-rdma@vger.kernel.org
> Subject: RE: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> > +struct ocrdma_cq {
> > + struct ib_cq ibcq;
> > + struct ocrdma_dev *dev;
>
> nit: There are several structures where you store ocrdma_dev *. You can
> remove these and use the struct ib_* to reach it as well.
Removed from all the ocrdma_xxx structures except ocrdma_eq and ocrdma_qp.
ocrdma_eq is anyway necessary.
Removing it from ocrdma_qp will require a lot bigger test cycle at my end.
I prefer to have it in this first patch and once the internal test cycle is
finish (with removed code) in few days, will submit the separate patch?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> -Original Message-
> From: Roland Dreier [mailto:rol...@purestorage.com]
> Sent: Wednesday, March 21, 2012 9:44 PM
> To: Pandit, Parav
> Cc: linux-rdma@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> > +#define ocrdma_err(format, arg...) printk(KERN_ERR format, ##arg)
>
> I think you'd be better off using pr_err() rather than defining your own
> macro.
o.k. I'll change it.
>
>
> > +struct ocrdma_cq {
> > + struct ib_cq ibcq;
> > + struct ocrdma_dev *dev;
> > + struct ocrdma_cqe *va;
> > + u32 phase;
> > + u32 getp; /* pointer to pending wrs to
> > + * return to stack, wrap arounds
> > + * at max_hw_cqe
> > + */
> > + u32 max_hw_cqe;
> > + bool phase_change;
> > + bool armed, solicited;
> > + bool arm_needed;
> > +
> > + spinlock_t cq_lock cacheline_aligned; /* provide
> > + synchronization
> > + * to cq polling
> > + */
> > + /* syncronizes cq completion handler invoked from multiple
> > + context */
> > + spinlock_t comp_handler_lock cacheline_aligned;
>
> You have quite a few of these alignment directives in the middle of
> structures.
> Have you measured that leaving all these gaps gives a reall performance
> boost?
>
>From beginning its cacheline aligned. So didn't tested it without it, but yes
>this is considered. I'll be shifting other widely used elements before the
>lock so that hole is smaller.
> > + u16 id;
> > + u16 eqn;
> > +
> > + struct ocrdma_ucontext *ucontext;
> > + dma_addr_t pa;
> > + u32 len;
> > + atomic_t use_cnt;
> > +
> > + /* head of all qp's sq and rq for which cqes need to be
> > +flushed
> > + * by the software.
> > + */
> > + struct list_head sq_head, rq_head; };
>
>
> > +#define OCRDMA_GET_NUM_POSTED_SHIFT_VAL(qp) \
> > + (((qp->dev->nic_info.dev_family == OCRDMA_GEN2_FAMILY) && \
> > + (qp->id < 64)) ? 24 : 16)
>
> In general it's better to use inline functions when possible instead of
> macros,
> which are less type-safe and harder to read.
o.k. I'll change it.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> +struct ocrdma_cq {
> + struct ib_cq ibcq;
> + struct ocrdma_dev *dev;
nit: There are several structures where you store ocrdma_dev *. You can remove
these and use the struct ib_* to reach it as well.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Tue, Mar 20, 2012 at 3:39 PM, wrote:
> +struct ocrdma_queue_info {
> + void *va;
> + dma_addr_t dma;
> + u32 size;
> + u16 len;
> + u16 entry_size; /* Size of an element in the queue */
> + u16 id; /* qid, where to ring the doorbell. */
> + u16 head, tail;
> + bool created;
> + atomic_t used; /* Number of valid elements in the queue */
> +};
Forgot to mention this before... the only place the used member is touched
that I can find is
> +static inline void ocrdma_mq_inc_head(struct ocrdma_dev *dev)
> +{
> + dev->mq.sq.head = (dev->mq.sq.head + 1) & (OCRDMA_MQ_LEN - 1);
> + atomic_inc(&dev->mq.sq.used);
> +}
but I don't see anywhere that it gets read. Can "used" be deleted?
- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> +#define ocrdma_err(format, arg...) printk(KERN_ERR format, ##arg)
I think you'd be better off using pr_err() rather than defining your own macro.
> +struct ocrdma_cq {
> + struct ib_cq ibcq;
> + struct ocrdma_dev *dev;
> + struct ocrdma_cqe *va;
> + u32 phase;
> + u32 getp; /* pointer to pending wrs to
> + * return to stack, wrap arounds
> + * at max_hw_cqe
> + */
> + u32 max_hw_cqe;
> + bool phase_change;
> + bool armed, solicited;
> + bool arm_needed;
> +
> + spinlock_t cq_lock cacheline_aligned; /* provide synchronization
> + * to cq polling
> + */
> + /* syncronizes cq completion handler invoked from multiple context */
> + spinlock_t comp_handler_lock cacheline_aligned;
You have quite a few of these alignment directives in the middle of structures.
Have you measured that leaving all these gaps gives a reall performance boost?
> + u16 id;
> + u16 eqn;
> +
> + struct ocrdma_ucontext *ucontext;
> + dma_addr_t pa;
> + u32 len;
> + atomic_t use_cnt;
> +
> + /* head of all qp's sq and rq for which cqes need to be flushed
> + * by the software.
> + */
> + struct list_head sq_head, rq_head;
> +};
> +#define OCRDMA_GET_NUM_POSTED_SHIFT_VAL(qp) \
> + (((qp->dev->nic_info.dev_family == OCRDMA_GEN2_FAMILY) && \
> + (qp->id < 64)) ? 24 : 16)
In general it's better to use inline functions when possible
instead of macros, which are less type-safe and harder to read.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Parav Pandit
- Header file for device and resource specific data structures.
Signed-off-by: Parav Pandit
---
drivers/infiniband/hw/ocrdma/ocrdma.h | 392 +
1 files changed, 392 insertions(+), 0 deletions(-)
create mode 100644 drivers/infiniband/hw/ocrdma/ocrdma.h
diff --git a/drivers/infiniband/hw/ocrdma/ocrdma.h
b/drivers/infiniband/hw/ocrdma/ocrdma.h
new file mode 100644
index 000..d7a44b8
--- /dev/null
+++ b/drivers/infiniband/hw/ocrdma/ocrdma.h
@@ -0,0 +1,392 @@
+/***
+ * This file is part of the Emulex RoCE Device Driver for *
+ * RoCE (RDMA over Converged Ethernet) adapters. *
+ * Copyright (C) 2008-2012 Emulex. All rights reserved.*
+ * EMULEX and SLI are trademarks of Emulex.*
+ * www.emulex.com *
+ * *
+ * This program is free software; you can redistribute it and/or *
+ * modify it under the terms of version 2 of the GNU General *
+ * Public License as published by the Free Software Foundation.*
+ * This program is distributed in the hope that it will be useful. *
+ * ALL EXPRESS OR IMPLIED CONDITIONS, REPRESENTATIONS AND *
+ * WARRANTIES, INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, *
+ * FITNESS FOR A PARTICULAR PURPOSE, OR NON-INFRINGEMENT, ARE *
+ * DISCLAIMED, EXCEPT TO THE EXTENT THAT SUCH DISCLAIMERS ARE HELD *
+ * TO BE LEGALLY INVALID. See the GNU General Public License for *
+ * more details, a copy of which can be found in the file COPYING *
+ * included with this package. *
+ *
+ * Contact Information:
+ * linux-driv...@emulex.com
+ *
+ * Emulex
+ * Susan Street
+ * Costa Mesa, CA 92626
+ ***/
+
+#ifndef __OCRDMA_H__
+#define __OCRDMA_H__
+
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#include
+#include "ocrdma_sli.h"
+
+#define OCRDMA_ROCE_DEV_VERSION "1.0.0"
+#define OCRDMA_NODE_DESC "Emulex OneConnect RoCE HCA"
+
+#define ocrdma_err(format, arg...) printk(KERN_ERR format, ##arg)
+
+#define OCRDMA_MAX_AH 512
+
+#define OCRDMA_UVERBS(CMD_NAME) (1ull << IB_USER_VERBS_CMD_##CMD_NAME)
+
+struct ocrdma_dev_attr {
+ u8 fw_ver[32];
+ u32 vendor_id;
+ u32 device_id;
+ u16 max_pd;
+ u16 max_cq;
+ u16 max_cqe;
+ u16 max_qp;
+ u16 max_wqe;
+ u16 max_rqe;
+ u32 max_inline_data;
+ int max_send_sge;
+ int max_recv_sge;
+ int max_mr;
+ u64 max_mr_size;
+ u32 max_num_mr_pbl;
+ int max_fmr;
+ int max_map_per_fmr;
+ int max_pages_per_frmr;
+ u16 max_ord_per_qp;
+ u16 max_ird_per_qp;
+
+ int device_cap_flags;
+ u8 cq_overflow_detect;
+ u8 srq_supported;
+
+ u32 wqe_size;
+ u32 rqe_size;
+ u32 ird_page_size;
+ u8 local_ca_ack_delay;
+ u8 ird;
+ u8 num_ird_pages;
+};
+
+struct ocrdma_pbl {
+ void *va;
+ dma_addr_t pa;
+};
+
+struct ocrdma_queue_info {
+ void *va;
+ dma_addr_t dma;
+ u32 size;
+ u16 len;
+ u16 entry_size; /* Size of an element in the queue */
+ u16 id; /* qid, where to ring the doorbell. */
+ u16 head, tail;
+ bool created;
+ atomic_t used; /* Number of valid elements in the queue */
+};
+
+struct ocrdma_eq {
+ struct ocrdma_queue_info q;
+ u32 vector;
+ int cq_cnt;
+ struct ocrdma_dev *dev;
+ char irq_name[32];
+};
+
+struct ocrdma_mq {
+ struct ocrdma_queue_info sq;
+ struct ocrdma_queue_info cq;
+ bool rearm_cq;
+};
+
+struct mqe_ctx {
+ struct mutex lock; /* for serializing mailbox commands on MQ */
+ wait_queue_head_t cmd_wait;
+ u32 tag;
+ u16 cqe_status;
+ u16 ext_status;
+ bool cmd_done;
+};
+
+struct ocrdma_dev {
+ struct ib_device ibdev;
+ struct ocrdma_dev_attr attr;
+
+ struct mutex dev_lock; /* provides syncronise access to device data */
+ spinlock_t flush_q_lock cacheline_aligned;
+
+ struct ocrdma_cq **cq_tbl;
+ struct ocrdma_qp **qp_tbl;
+
+ struct ocrdma_eq meq;
+ struct ocrdma_eq *qp_eq_tbl;
+ int eq_cnt;
+ u16 base_eqid;
+ u16 max_eq;
+
+ union ib_gid *sgid_tbl;
+ /* provided synchronization to sgid table for
+* updating gid entries triggered by notifier.
+*/
+ spinlock_t sgid_lock;
+
+ int gsi_qp_created;
+ struct ocrdma_cq *gsi_sqcq;
+ struct ocrdma_cq *gsi_rqcq;
+
+ struct {
+ struct ocrdma_av *va;
+ dma_addr_t pa;
+ u32 size;
+ u32 num_ah;
+ /* provide synchronization f
