netdev - RE: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 21 Mar 2012 11:58:31 -0700
From:	<Parav.Pandit@...lex.Com>
To:	<roland@...estorage.com>
CC:	<linux-rdma@...r.kernel.org>, <netdev@...r.kernel.org>
Subject: RE: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter



> -----Original Message-----
> From: Roland Dreier [mailto:roland@...estorage.com]
> Sent: Wednesday, March 21, 2012 9:44 PM
> To: Pandit, Parav
> Cc: linux-rdma@...r.kernel.org; netdev@...r.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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html