Re: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

Re: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Roland Dreier]

> +#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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Roland Dreier]

On Tue, Mar 20, 2012 at 3:39 PM,  <parav.pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
> From: Parav Pandit <parav.pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
>
> - Header file for userspace library and kernel driver interface.

> +struct ocrdma_alloc_ucontext_resp {
> +       u32 dev_id;
> +       u32 wqe_size;
> +       u32 max_inline_data;
> +       u32 dpp_wqe_size;
> +       u64 ah_tbl_page;
> +       u32 ah_tbl_len;
> +       u32 rsvd;
> +       u8 fw_ver[32];
> +       u32 rqe_size;
> +       u64 rsvd1;
> +} __packed;

If I'm reading this correctly, you have the 8-byte rsvd1 member at an
offset only aligned to 4 bytes, because of the __packed directive.  It
would be much better to have these structures laid out so they are
naturally the same on both 32-bit and 64-bit ABIs, and get rid of the
__packed directive, which wrecks gcc code generation in some cases.

In this particular case, it seems you could just move rqe_size into the
slot where rsvd is, and get rid of rsvd1?

> +/* user kernel communication data structures. */
> +struct ocrdma_alloc_pd_ureq {
> +       u64 rsvd1;
> +} __packed;

Similar comment -- __packed is silly for a structure with one reserved
member (and which you don't seem to use anywhere)... why not just
delete this struct?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Roland Dreier]

> +/* mailbox cmd response */
> +struct ocrdma_mbx_rsp {
> +       u32 subsys_op;
> +       u32 status;
> +       u32 rsp_len;
> +       u32 add_rsp_len;
> +} __packed;

...similar comments about only using __packed where you really need it...

> +#define is_cqe_valid(cq, cqe) \
> +       (((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_VALID)\
> +       == cq->phase) ? 1 : 0)
> +#define is_cqe_for_sq(cqe) \
> +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_QTYPE) ? 0 : 1)
> +#define is_cqe_for_rq(cqe) \
> +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_QTYPE) ? 1 : 0)
> +#define is_cqe_invalidated(cqe) \
> +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_INVALIDATE) ? \
> +       1 : 0)
> +#define is_cqe_imm(cqe) \
> +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_IMM) ? 1 : 0)
> +#define is_cqe_wr_imm(cqe) \
> +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_WRITE_IMM) ? 1 : 0)

...similar comment about using readable typesafe inline functions
instead of macros...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[David Laight]

 
> > - Header file for userspace library and kernel driver interface.
> 
> > +struct ocrdma_alloc_ucontext_resp {
> > +       u32 dev_id;
> > +       u32 wqe_size;
> > +       u32 max_inline_data;
> > +       u32 dpp_wqe_size;
> > +       u64 ah_tbl_page;
> > +       u32 ah_tbl_len;
> > +       u32 rsvd;
> > +       u8 fw_ver[32];
> > +       u32 rqe_size;
> > +       u64 rsvd1;
> > +} __packed;
> 
> If I'm reading this correctly, you have the 8-byte rsvd1 member at an
> offset only aligned to 4 bytes, because of the __packed directive.  It
> would be much better to have these structures laid out so they are
> naturally the same on both 32-bit and 64-bit ABIs, and get rid of the
> __packed directive, which wrecks gcc code generation in some cases.
> 

gcc also supports defining types that have non-standard alignment
constraints that can be used to force the same alignment for
64bit fields between i386 and amd64.
Probably __attribute__((aligned,n)) or similar.

This can be used to force 32bit alignment in amd64 code in order
to match definitions in 32bit userspace.
For new things it would make sense to force 64bit alignment
of 64bit fields for 32bit code.

Adding __packed (rather than 32bit alignment) forces the compiler
to generate byte by byte accesses for all the fields on systems
that can't do misaligned accesses in hardware (eg sparc).

	David


	David


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[David Laight]

 
> > +#define is_cqe_wr_imm(cqe) \
> > +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_WRITE_IMM) ? 1 : 0)
> 
> ...similar comment about using readable typesafe inline functions
> instead of macros...

and if you are using #defines, you need to enclose every reference
to the parameters in ().

	David


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Roland Dreier]

> +int ocrdma_qp_state_machine(struct ocrdma_qp *qp, enum ib_qp_state new_ib_state,
> +                           enum ib_qp_state *old_ib_state)
> +{
> +       unsigned long flags;
> +       int status = 0;
> +       enum ocrdma_qp_state new_state;
> +       new_state = get_ocrdma_qp_state(new_ib_state);
> +
> +       /* sync with wqe and rqe posting */
> +       spin_lock_irqsave(&qp->q_lock, flags);
> +
> +       if (old_ib_state)
> +               *old_ib_state = get_ibqp_state(qp->state);
> +       if (new_state == qp->state) {
> +               spin_unlock_irqrestore(&qp->q_lock, flags);
> +               return 1;
> +       }
> +
> +       switch (qp->state) {
> +       case OCRDMA_QPS_RST:
> +               switch (new_state) {
> +               case OCRDMA_QPS_RST:
> +               case OCRDMA_QPS_INIT:
> +                       break;
> +               default:
> +                       status = -EINVAL;
> +                       break;
> +               };
> +               break;
> +       case OCRDMA_QPS_INIT:
> +               /* qps: INIT->XXX */
> +               switch (new_state) {
> +               case OCRDMA_QPS_INIT:
> +               case OCRDMA_QPS_RTR:
> +                       break;
> +               case OCRDMA_QPS_ERR:
> +                       ocrdma_flush_qp(qp);
> +                       break;
> +               default:
> +                       status = -EINVAL;
> +                       break;
> +               };
> +               break;
> +       case OCRDMA_QPS_RTR:
> +               /* qps: RTS->XXX */
> +               switch (new_state) {
> +               case OCRDMA_QPS_RTS:
> +                       break;
> +               case OCRDMA_QPS_ERR:
> +                       ocrdma_flush_qp(qp);
> +                       break;
> +               default:
> +                       status = -EINVAL;
> +                       break;
> +               };
> +               break;
> +       case OCRDMA_QPS_RTS:
> +               /* qps: RTS->XXX */
> +               switch (new_state) {
> +               case OCRDMA_QPS_SQD:
> +               case OCRDMA_QPS_SQE:
> +                       break;
> +               case OCRDMA_QPS_ERR:
> +                       ocrdma_flush_qp(qp);
> +                       break;
> +               default:
> +                       status = -EINVAL;
> +                       break;
> +               };
> +               break;
> +       case OCRDMA_QPS_SQD:
> +               /* qps: SQD->XXX */
> +               switch (new_state) {
> +               case OCRDMA_QPS_RTS:
> +               case OCRDMA_QPS_SQE:
> +               case OCRDMA_QPS_ERR:
> +                       break;
> +               default:
> +                       status = -EINVAL;
> +                       break;
> +               };
> +               break;
> +       case OCRDMA_QPS_SQE:
> +               switch (new_state) {
> +               case OCRDMA_QPS_RTS:
> +               case OCRDMA_QPS_ERR:
> +                       break;
> +               default:
> +                       status = -EINVAL;
> +                       break;
> +               };
> +               break;
> +       case OCRDMA_QPS_ERR:
> +               /* qps: ERR->XXX */
> +               switch (new_state) {
> +               case OCRDMA_QPS_RST:
> +                       break;
> +               default:
> +                       status = -EINVAL;
> +                       break;
> +               };
> +               break;
> +       default:
> +               status = -EINVAL;
> +               break;
> +       };
> +       if (!status)
> +               qp->state = new_state;
> +
> +       spin_unlock_irqrestore(&qp->q_lock, flags);
> +       return status;
> +}

The switch statement here seems to largely reimpliment ib_modify_qp_is_ok()
(which is exported from the rdma midlayer).  Is there some reason that doesn't
work for your driver?  I'd rather fix / generalize the core helper
function instead
of having something mostly duplicate in a hardware driver.

Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Roland Dreier]

> +struct ib_pd *ocrdma_alloc_pd(struct ib_device *ibdev,
> +                             struct ib_ucontext *context,
> +                             struct ib_udata *udata)
> +{
> +       struct ocrdma_dev *dev = get_ocrdma_dev(ibdev);
> +       struct ocrdma_pd *pd;
> +       int status;
> +
> +       pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> +       if (!pd)
> +               return ERR_PTR(-ENOMEM);
> +       pd->dev = dev;
> +       if (udata && context) {
> +               pd->dpp_enabled = (dev->nic_info.dev_family ==
> +                                       OCRDMA_GEN2_FAMILY) ? true : false;

Writing

    (<bool expr>) ? true : false

is pretty silly, since it's just an obfuscated way of writing

    <bool expr>

IOW, you can just write

     pd->dpp_enabled = (dev->nic_info.dev_family == OCRDMA_GEN2_FAMILY);


> +int ocrdma_dealloc_pd(struct ib_pd *ibpd)
> +{
> +       struct ocrdma_pd *pd = get_ocrdma_pd(ibpd);
> +       struct ocrdma_dev *dev = pd->dev;
> +       int status;
> +       u64 usr_db;
> +
> +       if (atomic_read(&pd->use_cnt)) {
> +               ocrdma_err("%s(%d) pd=0x%x is in use.\n",
> +                          __func__, dev->id, pd->id);
> +               status = -EFAULT;
> +               goto dealloc_err;
> +       }

all of the use_cnt tracking in this driver seems to duplicate what the rdma
midlayer already does... is there any reason we need that in the low-level
hardware driver too, or can we just get rid of the various use_cnt members?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Roland Dreier]

On Tue, Mar 20, 2012 at 3:39 PM,  <parav.pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Parav.Pandit]

> -----Original Message-----
> From: Roland Dreier [mailto:roland@purestorage.com]
> Sent: Wednesday, March 21, 2012 9:44 PM
> To: Pandit, Parav
> Cc: linux-rdma@vger.kernel.org; netdev@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.

RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA]

I see couple of comments on rsvd words.
They were primarily not introduced for alignment. But there are other new features that we will be adding with new set of hardware and firmware updates.
I don't want to change the user-kernel interface at such stage by modifying the size of the structure.
For some features its under testing stage internally. 
So once its ready rsvd will be replaced with actual element.
This will avoid abi compatibility issues between library and driver.

I'll consider alignment macro too so that compiler related byte alignment access issue also gets resolved.

Parav

> -----Original Message-----
> From: Roland Dreier [mailto:roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org]
> Sent: Wednesday, March 21, 2012 9:50 PM
> To: Pandit, Parav
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
> 
> On Tue, Mar 20, 2012 at 3:39 PM,  <parav.pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
> > From: Parav Pandit <parav.pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
> >
> > - Header file for userspace library and kernel driver interface.
> 
> > +struct ocrdma_alloc_ucontext_resp {
> > +       u32 dev_id;
> > +       u32 wqe_size;
> > +       u32 max_inline_data;
> > +       u32 dpp_wqe_size;
> > +       u64 ah_tbl_page;
> > +       u32 ah_tbl_len;
> > +       u32 rsvd;
> > +       u8 fw_ver[32];
> > +       u32 rqe_size;
> > +       u64 rsvd1;
> > +} __packed;
> 
> If I'm reading this correctly, you have the 8-byte rsvd1 member at an offset
> only aligned to 4 bytes, because of the __packed directive.  It would be much
> better to have these structures laid out so they are naturally the same on
> both 32-bit and 64-bit ABIs, and get rid of the __packed directive, which
> wrecks gcc code generation in some cases.
> 
> In this particular case, it seems you could just move rqe_size into the slot
> where rsvd is, and get rid of rsvd1?
> 
> > +/* user kernel communication data structures. */ struct
> > +ocrdma_alloc_pd_ureq {
> > +       u64 rsvd1;
> > +} __packed;
> 
> Similar comment -- __packed is silly for a structure with one reserved
> member (and which you don't seem to use anywhere)... why not just delete
> this struct?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA]

> -----Original Message-----
> From: Roland Dreier [mailto:roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org]
> Sent: Wednesday, March 21, 2012 9:56 PM
> To: Pandit, Parav
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
> 
> > +/* mailbox cmd response */
> > +struct ocrdma_mbx_rsp {
> > +       u32 subsys_op;
> > +       u32 status;
> > +       u32 rsp_len;
> > +       u32 add_rsp_len;
> > +} __packed;
> 
> ...similar comments about only using __packed where you really need it...
This pack is required as it is shared with hardware and need to be of 16 bytes for 32 and 64 bit architecture. Do not wanted to take risk of different compiler versions. So keeping it packed.

> 
> > +#define is_cqe_valid(cq, cqe) \
> > +       (((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_VALID)\
> > +       == cq->phase) ? 1 : 0)
> > +#define is_cqe_for_sq(cqe) \
> > +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_QTYPE) ?
> > +0 : 1) #define is_cqe_for_rq(cqe) \
> > +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_QTYPE) ?
> > +1 : 0) #define is_cqe_invalidated(cqe) \
> > +       ((le32_to_cpu(cqe->flags_status_srcqpn) &
> > +OCRDMA_CQE_INVALIDATE) ? \
> > +       1 : 0)
> > +#define is_cqe_imm(cqe) \
> > +       ((le32_to_cpu(cqe->flags_status_srcqpn) & OCRDMA_CQE_IMM) ? 1
> > +: 0) #define is_cqe_wr_imm(cqe) \
> > +       ((le32_to_cpu(cqe->flags_status_srcqpn) &
> > +OCRDMA_CQE_WRITE_IMM) ? 1 : 0)
> 
> ...similar comment about using readable typesafe inline functions instead of
> macros...

Yes, I'll change to inline function.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Parav.Pandit]

> -----Original Message-----
> From: Roland Dreier [mailto:roland@purestorage.com]
> Sent: Wednesday, March 21, 2012 10:04 PM
> To: Pandit, Parav
> Cc: linux-rdma@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
> 
> > +int ocrdma_qp_state_machine(struct ocrdma_qp *qp, enum ib_qp_state
> > +new_ib_state,
> > +                           enum ib_qp_state *old_ib_state) {
> > +       unsigned long flags;
> > +       int status = 0;
> > +       enum ocrdma_qp_state new_state;
> > +       new_state = get_ocrdma_qp_state(new_ib_state);
> > +
> > +       /* sync with wqe and rqe posting */
> > +       spin_lock_irqsave(&qp->q_lock, flags);
> > +
> > +       if (old_ib_state)
> > +               *old_ib_state = get_ibqp_state(qp->state);
> > +       if (new_state == qp->state) {
> > +               spin_unlock_irqrestore(&qp->q_lock, flags);
> > +               return 1;
> > +       }
> > +
> > +       switch (qp->state) {
> > +       case OCRDMA_QPS_RST:
> > +               switch (new_state) {
> > +               case OCRDMA_QPS_RST:
> > +               case OCRDMA_QPS_INIT:
> > +                       break;
> > +               default:
> > +                       status = -EINVAL;
> > +                       break;
> > +               };
> > +               break;
> > +       case OCRDMA_QPS_INIT:
> > +               /* qps: INIT->XXX */
> > +               switch (new_state) {
> > +               case OCRDMA_QPS_INIT:
> > +               case OCRDMA_QPS_RTR:
> > +                       break;
> > +               case OCRDMA_QPS_ERR:
> > +                       ocrdma_flush_qp(qp);
> > +                       break;
> > +               default:
> > +                       status = -EINVAL;
> > +                       break;
> > +               };
> > +               break;
> > +       case OCRDMA_QPS_RTR:
> > +               /* qps: RTS->XXX */
> > +               switch (new_state) {
> > +               case OCRDMA_QPS_RTS:
> > +                       break;
> > +               case OCRDMA_QPS_ERR:
> > +                       ocrdma_flush_qp(qp);
> > +                       break;
> > +               default:
> > +                       status = -EINVAL;
> > +                       break;
> > +               };
> > +               break;
> > +       case OCRDMA_QPS_RTS:
> > +               /* qps: RTS->XXX */
> > +               switch (new_state) {
> > +               case OCRDMA_QPS_SQD:
> > +               case OCRDMA_QPS_SQE:
> > +                       break;
> > +               case OCRDMA_QPS_ERR:
> > +                       ocrdma_flush_qp(qp);
> > +                       break;
> > +               default:
> > +                       status = -EINVAL;
> > +                       break;
> > +               };
> > +               break;
> > +       case OCRDMA_QPS_SQD:
> > +               /* qps: SQD->XXX */
> > +               switch (new_state) {
> > +               case OCRDMA_QPS_RTS:
> > +               case OCRDMA_QPS_SQE:
> > +               case OCRDMA_QPS_ERR:
> > +                       break;
> > +               default:
> > +                       status = -EINVAL;
> > +                       break;
> > +               };
> > +               break;
> > +       case OCRDMA_QPS_SQE:
> > +               switch (new_state) {
> > +               case OCRDMA_QPS_RTS:
> > +               case OCRDMA_QPS_ERR:
> > +                       break;
> > +               default:
> > +                       status = -EINVAL;
> > +                       break;
> > +               };
> > +               break;
> > +       case OCRDMA_QPS_ERR:
> > +               /* qps: ERR->XXX */
> > +               switch (new_state) {
> > +               case OCRDMA_QPS_RST:
> > +                       break;
> > +               default:
> > +                       status = -EINVAL;
> > +                       break;
> > +               };
> > +               break;
> > +       default:
> > +               status = -EINVAL;
> > +               break;
> > +       };
> > +       if (!status)
> > +               qp->state = new_state;
> > +
> > +       spin_unlock_irqrestore(&qp->q_lock, flags);
> > +       return status;
> > +}
> 
> The switch statement here seems to largely reimpliment
> ib_modify_qp_is_ok() (which is exported from the rdma midlayer).  Is there
> some reason that doesn't work for your driver?  I'd rather fix / generalize the
> core helper function instead of having something mostly duplicate in a
> hardware driver.
Yes. Driver needs to put QP to flush state. So that appropriate CQEs can be returned during poll_cq() phase.
So state machine is implemented above.

RE: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA]

> -----Original Message-----
> From: Roland Dreier [mailto:roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org]
> Sent: Wednesday, March 21, 2012 10:13 PM
> To: Pandit, Parav
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
> 
> > +struct ib_pd *ocrdma_alloc_pd(struct ib_device *ibdev,
> > +                             struct ib_ucontext *context,
> > +                             struct ib_udata *udata) {
> > +       struct ocrdma_dev *dev = get_ocrdma_dev(ibdev);
> > +       struct ocrdma_pd *pd;
> > +       int status;
> > +
> > +       pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> > +       if (!pd)
> > +               return ERR_PTR(-ENOMEM);
> > +       pd->dev = dev;
> > +       if (udata && context) {
> > +               pd->dpp_enabled = (dev->nic_info.dev_family ==
> > +                                       OCRDMA_GEN2_FAMILY) ? true :
> > + false;
> 
> Writing
> 
>     (<bool expr>) ? true : false
> 
> is pretty silly, since it's just an obfuscated way of writing
> 
>     <bool expr>
> 
> IOW, you can just write
> 
>      pd->dpp_enabled = (dev->nic_info.dev_family ==
> OCRDMA_GEN2_FAMILY);
> 
> 
> > +int ocrdma_dealloc_pd(struct ib_pd *ibpd) {
> > +       struct ocrdma_pd *pd = get_ocrdma_pd(ibpd);
> > +       struct ocrdma_dev *dev = pd->dev;
> > +       int status;
> > +       u64 usr_db;
> > +
> > +       if (atomic_read(&pd->use_cnt)) {
> > +               ocrdma_err("%s(%d) pd=0x%x is in use.\n",
> > +                          __func__, dev->id, pd->id);
> > +               status = -EFAULT;
> > +               goto dealloc_err;
> > +       }
> 
> all of the use_cnt tracking in this driver seems to duplicate what the rdma
> midlayer already does... is there any reason we need that in the low-level
> hardware driver too, or can we just get rid of the various use_cnt members?

This use_cnt can be removed from low-level hardware driver. I'll remove it.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Roland Dreier]

On Wed, Mar 21, 2012 at 12:09 PM,  <Parav.Pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
> Yes. Driver needs to put QP to flush state. So that appropriate CQEs can be returned during poll_cq() phase.
> So state machine is implemented above.

Couldn't you just write

    if (ib_modify_qp_is_ok(...)) {
        if (new_state == OCRDMA_QPS_ERR)
            ocrdma_flush_qp(qp);
    } else {
        status = -EINVAL;
    }

and save about 100 lines of code?

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Roland Dreier]

On Wed, Mar 21, 2012 at 12:04 PM,  <Parav.Pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
>> > +/* mailbox cmd response */
>> > +struct ocrdma_mbx_rsp {
>> > +       u32 subsys_op;
>> > +       u32 status;
>> > +       u32 rsp_len;
>> > +       u32 add_rsp_len;
>> > +} __packed;

>> ...similar comments about only using __packed where you really need it...

> This pack is required as it is shared with hardware and need to be of 16 bytes for 32 and 64 bit architecture. Do not wanted to take risk of different compiler versions. So keeping it packed.

I really think if you can't trust your compiler to lay this structure
out properly,
you have a lot of bigger problems.  But whatever, it's not a big deal.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA]

> -----Original Message-----
> From: Roland Dreier [mailto:roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org]
> Sent: Thursday, March 22, 2012 1:02 AM
> To: Pandit, Parav
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
> 
> On Wed, Mar 21, 2012 at 12:09 PM,  <Parav.Pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
> > Yes. Driver needs to put QP to flush state. So that appropriate CQEs can be
> returned during poll_cq() phase.
> > So state machine is implemented above.
> 
> Couldn't you just write
> 
>     if (ib_modify_qp_is_ok(...)) {
>         if (new_state == OCRDMA_QPS_ERR)
>             ocrdma_flush_qp(qp);
>     } else {
>         status = -EINVAL;
>     }
> 
> and save about 100 lines of code?
> 
Yes, this can be done. This is one path.
Another path is async_event coming from adapter. So I still need qp_state_machine function but as you suggested, I'll remove the states and will have invoke flush_qp() on error.

>  - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Jason Gunthorpe]

On Wed, Mar 21, 2012 at 12:33:00PM -0700, Roland Dreier wrote:
> On Wed, Mar 21, 2012 at 12:04 PM,  <Parav.Pandit-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
> >> > +/* mailbox cmd response */
> >> > +struct ocrdma_mbx_rsp {
> >> > + ? ? ? u32 subsys_op;
> >> > + ? ? ? u32 status;
> >> > + ? ? ? u32 rsp_len;
> >> > + ? ? ? u32 add_rsp_len;
> >> > +} __packed;
> 
> >> ...similar comments about only using __packed where you really need it...
> 
> > This pack is required as it is shared with hardware and need to be
> > of 16 bytes for 32 and 64 bit architecture. Do not wanted to take
> > risk of different compiler versions. So keeping it packed.
> 
> I really think if you can't trust your compiler to lay this
> structure out properly, you have a lot of bigger problems.  But
> whatever, it's not a big deal.

Doesn't packed penalize all access to the structure on some
architectures, eg sparc?

A static assert is a better choice than packed...

BUILD_BUG_ON(sizeof(ocrdma_mbx_rsp) != 16);

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA]

> -----Original Message-----
> From: David Laight [mailto:David.Laight-ZS65k/vG3HxXrIkS9f7CXA@public.gmane.org]
> Sent: Wednesday, March 21, 2012 10:02 PM
> To: Roland Dreier; Pandit, Parav
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
> 
> 
> > > - Header file for userspace library and kernel driver interface.
> >
> > > +struct ocrdma_alloc_ucontext_resp {
> > > +       u32 dev_id;
> > > +       u32 wqe_size;
> > > +       u32 max_inline_data;
> > > +       u32 dpp_wqe_size;
> > > +       u64 ah_tbl_page;
> > > +       u32 ah_tbl_len;
> > > +       u32 rsvd;
> > > +       u8 fw_ver[32];
> > > +       u32 rqe_size;
> > > +       u64 rsvd1;
> > > +} __packed;
> >
> > If I'm reading this correctly, you have the 8-byte rsvd1 member at an
> > offset only aligned to 4 bytes, because of the __packed directive.  It
> > would be much better to have these structures laid out so they are
> > naturally the same on both 32-bit and 64-bit ABIs, and get rid of the
> > __packed directive, which wrecks gcc code generation in some cases.
> >
> 
> gcc also supports defining types that have non-standard alignment
> constraints that can be used to force the same alignment for 64bit fields
> between i386 and amd64.
> Probably __attribute__((aligned,n)) or similar.
> 
> This can be used to force 32bit alignment in amd64 code in order to match
> definitions in 32bit userspace.
> For new things it would make sense to force 64bit alignment of 64bit fields
> for 32bit code.

o.k. so I'll use aligned attribute to align user-kernel interface data structure to 8 byte boundary.
That should work for 32-bit and 64-bit user and kernel space and does't hurt performance either?

Driver-adapter structures will be aligned to 4 byte boundary using aligned attribute instead of packed.

> 
> Adding __packed (rather than 32bit alignment) forces the compiler to
> generate byte by byte accesses for all the fields on systems that can't do
> misaligned accesses in hardware (eg sparc).
> 
> 	David
> 
> 
> 	David
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Jason Gunthorpe]

On Thu, Mar 22, 2012 at 01:52:30PM -0700, Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA@public.gmane.org wrote:

> > This can be used to force 32bit alignment in amd64 code in order to match
> > definitions in 32bit userspace.
> > For new things it would make sense to force 64bit alignment of 64bit fields
> > for 32bit code.
> 
> o.k. so I'll use aligned attribute to align user-kernel interface
> data structure to 8 byte boundary.  That should work for 32-bit and
> 64-bit user and kernel space and does't hurt performance either?

If the structure is only for user/kernel interfacing then it is much
better to add explicit padding fields to naturally place 64 bit
quantities on an 8 byte alignment than to mess with gcc specific
attributes (user space has a much wide choice of compilers).

This was David's second suggestion. Better to do this now before the
driver is accepted :)

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org]
> Sent: Friday, March 23, 2012 2:28 AM
> To: Pandit, Parav
> Cc: David.Laight-ZS65k/vG3HxXrIkS9f7CXA@public.gmane.org; roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
> 
> On Thu, Mar 22, 2012 at 01:52:30PM -0700, Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA@public.gmane.org
> wrote:
> 
> > > This can be used to force 32bit alignment in amd64 code in order to
> > > match definitions in 32bit userspace.
> > > For new things it would make sense to force 64bit alignment of 64bit
> > > fields for 32bit code.
> >
> > o.k. so I'll use aligned attribute to align user-kernel interface data
> > structure to 8 byte boundary.  That should work for 32-bit and 64-bit
> > user and kernel space and does't hurt performance either?
> 
> If the structure is only for user/kernel interfacing then it is much better to
> add explicit padding fields to naturally place 64 bit quantities on an 8 byte
> alignment than to mess with gcc specific attributes (user space has a much
> wide choice of compilers).
> 
> This was David's second suggestion. Better to do this now before the driver is
> accepted :)
> 
o.k. I'll align them to naturally 8 byte boundary. 

> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA]

I got a question here lately.

aligned directive will ensure that it will fall on boundary.
Say aligned(4) ensures that structure is aligned to 4 byte boundary.
Compiler can (at least theoretically) still have 4 byte structure aligned to 8 byte boundary on 64-bit platform (which is 4 byte aligned too).

struct {
	u32 field;
} attribute ((aligned(4));

However requirement is to have this structure only 4 byte size( because adapter excepts it to be 4B sise) and therefor packed is used.
I don't know the way to ensure size of 4 byte and alignment too.
Or I am misunderstanding?

Parav

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA@public.gmane.org
> Sent: Friday, March 23, 2012 2:41 AM
> To: jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org
> Cc: David.Laight-ZS65k/vG3HxXrIkS9f7CXA@public.gmane.org; roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
> 
> 
> 
> > -----Original Message-----
> > From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org]
> > Sent: Friday, March 23, 2012 2:28 AM
> > To: Pandit, Parav
> > Cc: David.Laight-ZS65k/vG3HxXrIkS9f7CXA@public.gmane.org; roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org; linux-
> > rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> > adapter
> >
> > On Thu, Mar 22, 2012 at 01:52:30PM -0700, Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA@public.gmane.org
> > wrote:
> >
> > > > This can be used to force 32bit alignment in amd64 code in order
> > > > to match definitions in 32bit userspace.
> > > > For new things it would make sense to force 64bit alignment of
> > > > 64bit fields for 32bit code.
> > >
> > > o.k. so I'll use aligned attribute to align user-kernel interface
> > > data structure to 8 byte boundary.  That should work for 32-bit and
> > > 64-bit user and kernel space and does't hurt performance either?
> >
> > If the structure is only for user/kernel interfacing then it is much
> > better to add explicit padding fields to naturally place 64 bit
> > quantities on an 8 byte alignment than to mess with gcc specific
> > attributes (user space has a much wide choice of compilers).
> >
> > This was David's second suggestion. Better to do this now before the
> > driver is accepted :)
> >
> o.k. I'll align them to naturally 8 byte boundary.
> 
> > Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the
> body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Jason Gunthorpe]

On Thu, Mar 22, 2012 at 02:20:28PM -0700, Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA@public.gmane.org wrote:
> I got a question here lately.
> 
> aligned directive will ensure that it will fall on boundary.  Say
> aligned(4) ensures that structure is aligned to 4 byte boundary.
> Compiler can (at least theoretically) still have 4 byte structure
> aligned to 8 byte boundary on 64-bit platform (which is 4 byte
> aligned too).

There are very specific rules defined in the platform's ABI for how C
structures are layed out in memory, each ABI (ie CPU) has its own
specific quirks, but broadly in Linux land you can boil it down to:

1) The alignment of a structure is the greatest alignment of all the
   members
2) Each member is aligned to its alignment.

The alignment of the structure drives the total size of the structure,
and specifically the padding added at the end to reach that alignment.

So, no, a compiler that increased the alignment of a struct with one
u32 to 8 would violate the various ABIs and not be usuable for
Linux. It is important to bear in mind that Linux targets a particular
set of ABI conventions, and it is not 'anything goes'.

> struct {
> 	u32 field;
> };

So in this case: the u32 is aligned to 4, the structure is aligned to
4 and the total size of the structure is 4 on everything linux
supports.

> struct {
>       u64 fielda
> 	u32 field;
> };

In this case: On 64 bit: the u64 is aligned to 8 and the u32 is aligned to 4. So
the structure is aligned to 8. A pad is inserted at the end of the
struct to bring it out. On 32 bit, the u64 is aligned to 4, so the
struct is aligned to 4, so no pad is added.
 
> struct {
>       __float128 fielda
> 	u32 field;
> };

In this case the float128 is aligned to 16 and thus the structure is
aligned to 16 and 12 pad bytes are added.

> However requirement is to have this structure only 4 byte size(
> because adapter excepts it to be 4B sise) and therefor packed is
> used.  I don't know the way to ensure size of 4 byte and alignment
> too.  Or I am misunderstanding?

Yes, you are mis-understanding the rules for padding.. Structures are
only padded out to their alignment, which depends on their constituent
types. This is so arrays of structures have each array element
starting on its natural alignment.

The aligned attribute overrides the automatic determination of the
alignment based on the contents and just forces it.

So, as an example, if you have this hardware layout:

struct {
  u32 fielda;
  u64 fieldb;
} attribute ((aligned(4));

David is saying you will get a 12 byte struct and fieldb will be
unaligned. Since 12 is aligned to 4 no padding is added.

For hardware facing structures I'd combine this with a static assert
to verify structure size at compile time.

So..

1) Avoid using attributes unless the structure has unaligned members.
2) Avoid creating structures with unaligned members (eg for userspace
   communication)
3) Frown at hardware/firmware developers who make communication
   structures with unaligned members :)
4) Be explicit about padding in your layout for 64/32
   compatibility.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Roland Dreier]

On Thu, Mar 22, 2012 at 3:44 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> For hardware facing structures I'd combine this with a static assert
> to verify structure size at compile time.
>
> So..
>
> 1) Avoid using attributes unless the structure has unaligned members.
> 2) Avoid creating structures with unaligned members (eg for userspace
>   communication)
> 3) Frown at hardware/firmware developers who make communication
>   structures with unaligned members :)
> 4) Be explicit about padding in your layout for 64/32
>   compatibility.

Excellent summary, thanks Jason.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[David Laight]

 
> > struct {
> >       u64 fielda
> > 	u32 field;
> > };
> 
> In this case: On 64 bit: the u64 is aligned to 8 and the u32 
> is aligned to 4. So
> the structure is aligned to 8. A pad is inserted at the end of the
> struct to bring it out. On 32 bit, the u64 is aligned to 4, so the
> struct is aligned to 4, so no pad is added.

That is true for 32bit i386, some 32bit ABI may require 64bit
alignment for 64bit items.

...
> The aligned attribute overrides the automatic determination of the
> alignment based on the contents and just forces it.
> 
> So, as an example, if you have this hardware layout:
> 
> struct {
>   u32 fielda;
>   u64 fieldb;
> } attribute ((aligned(4));
> 
> David is saying you will get a 12 byte struct and fieldb will be
> unaligned. Since 12 is aligned to 4 no padding is added.

I was actually suggesting defining (modulo syntax errors):
typedef u64 u64_aligned_8 attribute ((aligned(8));

Then you can define:
struct foo {
	u32 fielda;
	u64_aligned_8 fieldb;
};
and know that the aligment should be the same on all systems.
(But I'd still add the explicit pad in the above.)

This is actually more useful when you need to process structures
that have misaligned 64bit items - as happens when doing 'compat 32'
code in a 64bit kernel. Or trying to map the x86 disk mbr.

> For hardware facing structures I'd combine this with a static assert
> to verify structure size at compile time.

Also useful if you need to ensure a structure doesn't accidentally
change size when you need to preserve a userspace interface.

> So..
> 
> 1) Avoid using attributes unless the structure has unaligned members.
> 2) Avoid creating structures with unaligned members (eg for userspace
>    communication)
> 3) Frown at hardware/firmware developers who make communication
>    structures with unaligned members :)
> 4) Be explicit about padding in your layout for 64/32
>    compatibility.

Agreed.

	David
 


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org]
> Sent: Friday, March 23, 2012 4:14 AM
> To: Pandit, Parav
> Cc: David.Laight-ZS65k/vG3HxXrIkS9f7CXA@public.gmane.org; roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
> 
> On Thu, Mar 22, 2012 at 02:20:28PM -0700, Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA@public.gmane.org
> wrote:
> > I got a question here lately.
> >
> > aligned directive will ensure that it will fall on boundary.  Say
> > aligned(4) ensures that structure is aligned to 4 byte boundary.
> > Compiler can (at least theoretically) still have 4 byte structure
> > aligned to 8 byte boundary on 64-bit platform (which is 4 byte aligned
> > too).
> 
> There are very specific rules defined in the platform's ABI for how C
> structures are layed out in memory, each ABI (ie CPU) has its own specific
> quirks, but broadly in Linux land you can boil it down to:
> 
> 1) The alignment of a structure is the greatest alignment of all the
>    members
> 2) Each member is aligned to its alignment.
> 
> The alignment of the structure drives the total size of the structure, and
> specifically the padding added at the end to reach that alignment.
> 
> So, no, a compiler that increased the alignment of a struct with one
> u32 to 8 would violate the various ABIs and not be usuable for Linux. It is
> important to bear in mind that Linux targets a particular set of ABI
> conventions, and it is not 'anything goes'.
> 
> > struct {
> > 	u32 field;
> > };
> 
> So in this case: the u32 is aligned to 4, the structure is aligned to
> 4 and the total size of the structure is 4 on everything linux supports.
> 
> > struct {
> >       u64 fielda
> > 	u32 field;
> > };
> 
> In this case: On 64 bit: the u64 is aligned to 8 and the u32 is aligned to 4. So
> the structure is aligned to 8. A pad is inserted at the end of the struct to bring
> it out. On 32 bit, the u64 is aligned to 4, so the struct is aligned to 4, so no pad
> is added.
> 
> > struct {
> >       __float128 fielda
> > 	u32 field;
> > };
> 
> In this case the float128 is aligned to 16 and thus the structure is aligned to 16
> and 12 pad bytes are added.
> 
> > However requirement is to have this structure only 4 byte size(
> > because adapter excepts it to be 4B sise) and therefor packed is used.
> > I don't know the way to ensure size of 4 byte and alignment too.  Or I
> > am misunderstanding?
> 
> Yes, you are mis-understanding the rules for padding.. Structures are only
> padded out to their alignment, which depends on their constituent types.
> This is so arrays of structures have each array element starting on its natural
> alignment.
> 
> The aligned attribute overrides the automatic determination of the
> alignment based on the contents and just forces it.
> 
> So, as an example, if you have this hardware layout:
> 
> struct {
>   u32 fielda;
>   u64 fieldb;
> } attribute ((aligned(4));
> 
> David is saying you will get a 12 byte struct and fieldb will be unaligned. Since
> 12 is aligned to 4 no padding is added.

So I decided to experiment above example before implementing in driver. However I find structure of 16 bytes (instead of 12) with padding after fielda in below example.
Am I missing some compiler option or syntax error in attribute? Sorry to ask this silly question.
I tried __attribute__((__aligned__(4))); too based on usage in other kernel code.

#include <linux/module.h>

struct foo {
        u32 a;
        u64 b;
} __attribute__((aligned(4)));

static int __init main_init(void)
{
        printk("<1> sizeof foo=%ld\n", sizeof(struct foo));
        printk("<1> offset of a=%ld\n", offsetof(struct foo, a));
        printk("<1> offset of b=%ld\n", offsetof(struct foo, b));
        return 0;
}
static void __exit main_exit(void)
{
}
module_init(main_init);
module_exit(main_exit);
Output:
Mar 24 05:44:10 parav-sles11-sp1-64 kernel: [2006356.094931]  sizeof foo=16
Mar 24 05:44:10 parav-sles11-sp1-64 kernel: [2006356.094934]  offset of a=0
Mar 24 05:44:10 parav-sles11-sp1-64 kernel: [2006356.094936]  offset of b=8

Gcc version is below.

Using built-in specs.
Target: x86_64-suse-linux
Configured with: ../configure --prefix=/usr --infodir=/usr/share/info --mandir=/usr/share/man --libdir=/usr/lib64 --libexecdir=/usr/lib64 --enable-languages=c,c++,objc,fortran,obj-c++,java,ada --enable-checking=release --with-gxx-include-dir=/usr/include/c++/4.3 --enable-ssp --disable-libssp --with-bugurl=http://bugs.opensuse.org/ --with-pkgversion='SUSE Linux' --disable-libgcj --disable-libmudflap --with-slibdir=/lib64 --with-system-zlib --enable-__cxa_atexit --enable-libstdcxx-allocator=new --disable-libstdcxx-pch --enable-version-specific-runtime-libs --program-suffix=-4.3 --enable-linux-futex --without-system-libunwind --with-cpu=generic --build=x86_64-suse-linux
Thread model: posix
gcc version 4.3.4 [gcc-4_3-branch revision 152973] (SUSE Linux)

> For hardware facing structures I'd combine this with a static assert to verify
> structure size at compile time.
> 
> So..
> 
> 1) Avoid using attributes unless the structure has unaligned members.
> 2) Avoid creating structures with unaligned members (eg for userspace
>    communication)
> 3) Frown at hardware/firmware developers who make communication
>    structures with unaligned members :)
> 4) Be explicit about padding in your layout for 64/32
>    compatibility.
> 
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Jason Gunthorpe]

On Fri, Mar 23, 2012 at 07:03:37AM -0700, Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA@public.gmane.org wrote:

> > David is saying you will get a 12 byte struct and fieldb will be unaligned. Since
> > 12 is aligned to 4 no padding is added.
> 
> So I decided to experiment above example before implementing in
> driver. However I find structure of 16 bytes (instead of 12) with
> padding after fielda in below example.  Am I missing some compiler
> option or syntax error in attribute? Sorry to ask this silly
> question.  I tried __attribute__((__aligned__(4))); too based on
> usage in other kernel code.

I got the syntax wrong for that specific case (it is a little
unintuitive.. IMHO, capping the alignment of a container should cap
the alignment of all members, otherwise it is nonsense!):

typedef uint64_t u64_unaligned_8 __attribute__((__aligned__(4)));

struct foo {
        uint32_t fielda;
        u64_unaligned_8 fieldb;
};

struct foo2 {
        uint32_t fielda;
        uint64_t fieldb;
};

int main(int argc,const char *argv[])
{
	printf("sizeof(foo) = %zu, fieldb = %zu\n",sizeof(struct foo),
	       offsetof(struct foo,fieldb));
	printf("sizeof(foo2) = %zu, fieldb = %zu\n",sizeof(struct foo2),
	       offsetof(struct foo2,fieldb));
	return 0;
}

sizeof(foo) = 12, fieldb = 4
sizeof(foo2) = 16, fieldb = 8

gcc version 4.6.1 (Ubuntu/Linaro 4.6.1-9ubuntu3) 

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter

[Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA]

Got it. You did mention about typedef in email chain, but I understood as different way to achieve same.

I reviewed my code and found that most of the fields between driver-adapter doesn't need attribute.
So far (a) removing packed and (b) BUILD_BUG_ON looks sufficient for current set of structures. Initial test suffice the need.

Thanks.
Parav

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org]
> Sent: Friday, March 23, 2012 10:24 PM
> To: Pandit, Parav
> Cc: David.Laight-ZS65k/vG3HxXrIkS9f7CXA@public.gmane.org; roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
> 
> On Fri, Mar 23, 2012 at 07:03:37AM -0700, Parav.Pandit-iH1Dq9VlAzfQT0dZR+AlfA@public.gmane.org wrote:
> 
> > > David is saying you will get a 12 byte struct and fieldb will be
> > > unaligned. Since
> > > 12 is aligned to 4 no padding is added.
> >
> > So I decided to experiment above example before implementing in
> > driver. However I find structure of 16 bytes (instead of 12) with
> > padding after fielda in below example.  Am I missing some compiler
> > option or syntax error in attribute? Sorry to ask this silly question.
> > I tried __attribute__((__aligned__(4))); too based on usage in other
> > kernel code.
> 
> I got the syntax wrong for that specific case (it is a little unintuitive.. IMHO,
> capping the alignment of a container should cap the alignment of all
> members, otherwise it is nonsense!):
> 
> typedef uint64_t u64_unaligned_8 __attribute__((__aligned__(4)));
> 
> struct foo {
>         uint32_t fielda;
>         u64_unaligned_8 fieldb;
> };
> 
> struct foo2 {
>         uint32_t fielda;
>         uint64_t fieldb;
> };
> 
> int main(int argc,const char *argv[])
> {
> 	printf("sizeof(foo) = %zu, fieldb = %zu\n",sizeof(struct foo),
> 	       offsetof(struct foo,fieldb));
> 	printf("sizeof(foo2) = %zu, fieldb = %zu\n",sizeof(struct foo2),
> 	       offsetof(struct foo2,fieldb));
> 	return 0;
> }
> 
> sizeof(foo) = 12, fieldb = 4
> sizeof(foo2) = 16, fieldb = 8
> 
> gcc version 4.6.1 (Ubuntu/Linaro 4.6.1-9ubuntu3)
> 
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html