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
