| Message ID | 20190805141708.9004-2-bmt@zurich.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix siw CQ processing for 32 bit archtecture support | expand |
On Mon, Aug 05, 2019 at 04:17:08PM +0200, Bernard Metzler wrote: > Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com> > --- > drivers/infiniband/sw/siw/Kconfig | 2 +- > drivers/infiniband/sw/siw/siw.h | 2 +- > drivers/infiniband/sw/siw/siw_qp.c | 14 ++++++++++---- > drivers/infiniband/sw/siw/siw_verbs.c | 16 +++++++++++----- > include/uapi/rdma/siw-abi.h | 3 ++- > 5 files changed, 25 insertions(+), 12 deletions(-) > > diff --git a/drivers/infiniband/sw/siw/Kconfig b/drivers/infiniband/sw/siw/Kconfig > index dace276aea14..b622fc62f2cd 100644 > --- a/drivers/infiniband/sw/siw/Kconfig > +++ b/drivers/infiniband/sw/siw/Kconfig > @@ -1,6 +1,6 @@ > config RDMA_SIW > tristate "Software RDMA over TCP/IP (iWARP) driver" > - depends on INET && INFINIBAND && LIBCRC32C && 64BIT > + depends on INET && INFINIBAND && LIBCRC32C > select DMA_VIRT_OPS > help > This driver implements the iWARP RDMA transport over > diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h > index 03fd7b2f595f..77b1aabf6ff3 100644 > --- a/drivers/infiniband/sw/siw/siw.h > +++ b/drivers/infiniband/sw/siw/siw.h > @@ -214,7 +214,7 @@ struct siw_wqe { > struct siw_cq { > struct ib_cq base_cq; > spinlock_t lock; > - u64 *notify; > + struct siw_cq_ctrl *notify; > struct siw_cqe *queue; > u32 cq_put; > u32 cq_get; > diff --git a/drivers/infiniband/sw/siw/siw_qp.c b/drivers/infiniband/sw/siw/siw_qp.c > index e27bd5b35b96..0990307c5d2c 100644 > --- a/drivers/infiniband/sw/siw/siw_qp.c > +++ b/drivers/infiniband/sw/siw/siw_qp.c > @@ -1013,18 +1013,24 @@ int siw_activate_tx(struct siw_qp *qp) > */ > static bool siw_cq_notify_now(struct siw_cq *cq, u32 flags) > { > - u64 cq_notify; > + u32 cq_notify; > > if (!cq->base_cq.comp_handler) > return false; > > - cq_notify = READ_ONCE(*cq->notify); > + /* Read application shared notification state */ > + cq_notify = READ_ONCE(cq->notify->flags); > > if ((cq_notify & SIW_NOTIFY_NEXT_COMPLETION) || > ((cq_notify & SIW_NOTIFY_SOLICITED) && > (flags & SIW_WQE_SOLICITED))) { > - /* dis-arm CQ */ > - smp_store_mb(*cq->notify, SIW_NOTIFY_NOT); > + /* > + * CQ notification is one-shot: Since the > + * current CQE causes user notification, > + * the CQ gets dis-aremd and must be re-aremd > + * by the user for a new notification. > + */ > + WRITE_ONCE(cq->notify->flags, SIW_NOTIFY_NOT); > > return true; > } > diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c > index 32dc79d0e898..e7f3a2379d9d 100644 > --- a/drivers/infiniband/sw/siw/siw_verbs.c > +++ b/drivers/infiniband/sw/siw/siw_verbs.c > @@ -1049,7 +1049,7 @@ int siw_create_cq(struct ib_cq *base_cq, const struct ib_cq_init_attr *attr, > > spin_lock_init(&cq->lock); > > - cq->notify = &((struct siw_cq_ctrl *)&cq->queue[size])->notify; > + cq->notify = (struct siw_cq_ctrl *)&cq->queue[size]; > > if (udata) { > struct siw_uresp_create_cq uresp = {}; > @@ -1141,11 +1141,17 @@ int siw_req_notify_cq(struct ib_cq *base_cq, enum ib_cq_notify_flags flags) > siw_dbg_cq(cq, "flags: 0x%02x\n", flags); > > if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_SOLICITED) > - /* CQ event for next solicited completion */ > - smp_store_mb(*cq->notify, SIW_NOTIFY_SOLICITED); > + /* > + * Enable CQ event for next solicited completion. > + * and make it visible to all associated producers. > + */ > + smp_store_mb(cq->notify->flags, SIW_NOTIFY_SOLICITED); > else > - /* CQ event for any signalled completion */ > - smp_store_mb(*cq->notify, SIW_NOTIFY_ALL); > + /* > + * Enable CQ event for any signalled completion. > + * and make it visible to all associated producers. > + */ > + smp_store_mb(cq->notify->flags, SIW_NOTIFY_ALL); > > if (flags & IB_CQ_REPORT_MISSED_EVENTS) > return cq->cq_put - cq->cq_get; > diff --git a/include/uapi/rdma/siw-abi.h b/include/uapi/rdma/siw-abi.h > index 7de68f1dc707..af735f55b291 100644 > --- a/include/uapi/rdma/siw-abi.h > +++ b/include/uapi/rdma/siw-abi.h > @@ -180,6 +180,7 @@ struct siw_cqe { > * to control CQ arming. > */ > struct siw_cq_ctrl { > - __aligned_u64 notify; > + __u32 flags; > + __u32 pad; You can't do it, it will break backward compatibility with rdma-core. https://github.com/linux-rdma/rdma-core/blob/2066065574554229f5e4ef1a37abf637938b71e3/providers/siw/siw.c#L175 Thanks > }; > #endif > -- > 2.17.2 >
-----"Leon Romanovsky" <leon@kernel.org> wrote: ----- >To: "Bernard Metzler" <bmt@zurich.ibm.com> >From: "Leon Romanovsky" <leon@kernel.org> >Date: 08/05/2019 07:09PM >Cc: linux-rdma@vger.kernel.org, jgg@ziepe.ca >Subject: [EXTERNAL] Re: [PATCH 1/1] Make user mmapped CQ arming flags >field 32 bit size to remove 64 bit architecture dependency of siw. > >On Mon, Aug 05, 2019 at 04:17:08PM +0200, Bernard Metzler wrote: >> Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com> >> --- >> drivers/infiniband/sw/siw/Kconfig | 2 +- >> drivers/infiniband/sw/siw/siw.h | 2 +- >> drivers/infiniband/sw/siw/siw_qp.c | 14 ++++++++++---- >> drivers/infiniband/sw/siw/siw_verbs.c | 16 +++++++++++----- >> include/uapi/rdma/siw-abi.h | 3 ++- >> 5 files changed, 25 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/infiniband/sw/siw/Kconfig >b/drivers/infiniband/sw/siw/Kconfig >> index dace276aea14..b622fc62f2cd 100644 >> --- a/drivers/infiniband/sw/siw/Kconfig >> +++ b/drivers/infiniband/sw/siw/Kconfig >> @@ -1,6 +1,6 @@ >> config RDMA_SIW >> tristate "Software RDMA over TCP/IP (iWARP) driver" >> - depends on INET && INFINIBAND && LIBCRC32C && 64BIT >> + depends on INET && INFINIBAND && LIBCRC32C >> select DMA_VIRT_OPS >> help >> This driver implements the iWARP RDMA transport over >> diff --git a/drivers/infiniband/sw/siw/siw.h >b/drivers/infiniband/sw/siw/siw.h >> index 03fd7b2f595f..77b1aabf6ff3 100644 >> --- a/drivers/infiniband/sw/siw/siw.h >> +++ b/drivers/infiniband/sw/siw/siw.h >> @@ -214,7 +214,7 @@ struct siw_wqe { >> struct siw_cq { >> struct ib_cq base_cq; >> spinlock_t lock; >> - u64 *notify; >> + struct siw_cq_ctrl *notify; >> struct siw_cqe *queue; >> u32 cq_put; >> u32 cq_get; >> diff --git a/drivers/infiniband/sw/siw/siw_qp.c >b/drivers/infiniband/sw/siw/siw_qp.c >> index e27bd5b35b96..0990307c5d2c 100644 >> --- a/drivers/infiniband/sw/siw/siw_qp.c >> +++ b/drivers/infiniband/sw/siw/siw_qp.c >> @@ -1013,18 +1013,24 @@ int siw_activate_tx(struct siw_qp *qp) >> */ >> static bool siw_cq_notify_now(struct siw_cq *cq, u32 flags) >> { >> - u64 cq_notify; >> + u32 cq_notify; >> >> if (!cq->base_cq.comp_handler) >> return false; >> >> - cq_notify = READ_ONCE(*cq->notify); >> + /* Read application shared notification state */ >> + cq_notify = READ_ONCE(cq->notify->flags); >> >> if ((cq_notify & SIW_NOTIFY_NEXT_COMPLETION) || >> ((cq_notify & SIW_NOTIFY_SOLICITED) && >> (flags & SIW_WQE_SOLICITED))) { >> - /* dis-arm CQ */ >> - smp_store_mb(*cq->notify, SIW_NOTIFY_NOT); >> + /* >> + * CQ notification is one-shot: Since the >> + * current CQE causes user notification, >> + * the CQ gets dis-aremd and must be re-aremd >> + * by the user for a new notification. >> + */ >> + WRITE_ONCE(cq->notify->flags, SIW_NOTIFY_NOT); >> >> return true; >> } >> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c >b/drivers/infiniband/sw/siw/siw_verbs.c >> index 32dc79d0e898..e7f3a2379d9d 100644 >> --- a/drivers/infiniband/sw/siw/siw_verbs.c >> +++ b/drivers/infiniband/sw/siw/siw_verbs.c >> @@ -1049,7 +1049,7 @@ int siw_create_cq(struct ib_cq *base_cq, >const struct ib_cq_init_attr *attr, >> >> spin_lock_init(&cq->lock); >> >> - cq->notify = &((struct siw_cq_ctrl *)&cq->queue[size])->notify; >> + cq->notify = (struct siw_cq_ctrl *)&cq->queue[size]; >> >> if (udata) { >> struct siw_uresp_create_cq uresp = {}; >> @@ -1141,11 +1141,17 @@ int siw_req_notify_cq(struct ib_cq >*base_cq, enum ib_cq_notify_flags flags) >> siw_dbg_cq(cq, "flags: 0x%02x\n", flags); >> >> if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_SOLICITED) >> - /* CQ event for next solicited completion */ >> - smp_store_mb(*cq->notify, SIW_NOTIFY_SOLICITED); >> + /* >> + * Enable CQ event for next solicited completion. >> + * and make it visible to all associated producers. >> + */ >> + smp_store_mb(cq->notify->flags, SIW_NOTIFY_SOLICITED); >> else >> - /* CQ event for any signalled completion */ >> - smp_store_mb(*cq->notify, SIW_NOTIFY_ALL); >> + /* >> + * Enable CQ event for any signalled completion. >> + * and make it visible to all associated producers. >> + */ >> + smp_store_mb(cq->notify->flags, SIW_NOTIFY_ALL); >> >> if (flags & IB_CQ_REPORT_MISSED_EVENTS) >> return cq->cq_put - cq->cq_get; >> diff --git a/include/uapi/rdma/siw-abi.h >b/include/uapi/rdma/siw-abi.h >> index 7de68f1dc707..af735f55b291 100644 >> --- a/include/uapi/rdma/siw-abi.h >> +++ b/include/uapi/rdma/siw-abi.h >> @@ -180,6 +180,7 @@ struct siw_cqe { >> * to control CQ arming. >> */ >> struct siw_cq_ctrl { >> - __aligned_u64 notify; >> + __u32 flags; >> + __u32 pad; > >You can't do it, it will break backward compatibility with rdma-core. >https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_linux >-2Drdma_rdma-2Dcore_blob_2066065574554229f5e4ef1a37abf637938b71e3_pro >viders_siw_siw.c-23L175&d=DwIBAg&c=jf_iaSHvJObTbx-siA1ZOg&r=2TaYXQ0T- >r8ZO1PP1alNwU_QJcRRLfmYTAgd3QCvqSc&m=7tPE1DK3hqqKmVY0xAMOdRXzQJYsxDwj >RVAlCUxMcVo&s=kdWgjm1Qah8ux50emKGomnnwyScBHDivqUSyVkjnNbw&e= > We would still mmap the 64bits of a notifications field of the CQ, which is now (see siw-abi.h): struct siw_cq_ctrl { __u32 flags; __u32 pad; }; I changed the variable name to 'flags' though, which shall improve readability. The only change in siw user lib would be as below. Would that be acceptable? Many thanks! Bernard. From 2456a7fb4bb4c55a34087b40486a30c06a67654e Mon Sep 17 00:00:00 2001 From: Bernard Metzler <bmt@zurich.ibm.com> Date: Tue, 6 Aug 2019 13:48:42 +0200 Subject: [PATCH] Change user mmapped siw CQ notifications flags to 32bit. Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com> --- providers/siw/siw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/providers/siw/siw.c b/providers/siw/siw.c index c1acf398..b4b491b4 100644 --- a/providers/siw/siw.c +++ b/providers/siw/siw.c @@ -173,7 +173,7 @@ static struct ibv_cq *siw_create_cq(struct ibv_context *ctx, int num_cqe, goto fail; } cq->ctrl = (struct siw_cq_ctrl *)&cq->queue[cq->num_cqe]; - cq->ctrl->notify = SIW_NOTIFY_NOT; + cq->ctrl->flags = SIW_NOTIFY_NOT; return &cq->base_cq; fail: @@ -482,7 +482,7 @@ static void siw_async_event(struct ibv_context *ctx, static int siw_notify_cq(struct ibv_cq *ibcq, int solicited) { struct siw_cq *cq = cq_base2siw(ibcq); - atomic_ulong *notifyp = (atomic_ulong *)&cq->ctrl->notify; + atomic_uint *notifyp = (atomic_uint *)&cq->ctrl->flags; int rv = 0; if (solicited)
On Mon, Aug 05, 2019 at 04:17:08PM +0200, Bernard Metzler wrote: > Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com> > --- Don't send patches with empty commit messages. Every patch must have a comprehensive commit message from now on. > drivers/infiniband/sw/siw/Kconfig | 2 +- > drivers/infiniband/sw/siw/siw.h | 2 +- > drivers/infiniband/sw/siw/siw_qp.c | 14 ++++++++++---- > drivers/infiniband/sw/siw/siw_verbs.c | 16 +++++++++++----- > include/uapi/rdma/siw-abi.h | 3 ++- > 5 files changed, 25 insertions(+), 12 deletions(-) > > diff --git a/drivers/infiniband/sw/siw/Kconfig b/drivers/infiniband/sw/siw/Kconfig > index dace276aea14..b622fc62f2cd 100644 > --- a/drivers/infiniband/sw/siw/Kconfig > +++ b/drivers/infiniband/sw/siw/Kconfig > @@ -1,6 +1,6 @@ > config RDMA_SIW > tristate "Software RDMA over TCP/IP (iWARP) driver" > - depends on INET && INFINIBAND && LIBCRC32C && 64BIT > + depends on INET && INFINIBAND && LIBCRC32C > select DMA_VIRT_OPS > help > This driver implements the iWARP RDMA transport over > diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h > index 03fd7b2f595f..77b1aabf6ff3 100644 > --- a/drivers/infiniband/sw/siw/siw.h > +++ b/drivers/infiniband/sw/siw/siw.h > @@ -214,7 +214,7 @@ struct siw_wqe { > struct siw_cq { > struct ib_cq base_cq; > spinlock_t lock; > - u64 *notify; > + struct siw_cq_ctrl *notify; > struct siw_cqe *queue; > u32 cq_put; > u32 cq_get; > diff --git a/drivers/infiniband/sw/siw/siw_qp.c b/drivers/infiniband/sw/siw/siw_qp.c > index e27bd5b35b96..0990307c5d2c 100644 > --- a/drivers/infiniband/sw/siw/siw_qp.c > +++ b/drivers/infiniband/sw/siw/siw_qp.c > @@ -1013,18 +1013,24 @@ int siw_activate_tx(struct siw_qp *qp) > */ > static bool siw_cq_notify_now(struct siw_cq *cq, u32 flags) > { > - u64 cq_notify; > + u32 cq_notify; > > if (!cq->base_cq.comp_handler) > return false; > > - cq_notify = READ_ONCE(*cq->notify); > + /* Read application shared notification state */ > + cq_notify = READ_ONCE(cq->notify->flags); > > if ((cq_notify & SIW_NOTIFY_NEXT_COMPLETION) || > ((cq_notify & SIW_NOTIFY_SOLICITED) && > (flags & SIW_WQE_SOLICITED))) { > - /* dis-arm CQ */ > - smp_store_mb(*cq->notify, SIW_NOTIFY_NOT); > + /* > + * CQ notification is one-shot: Since the > + * current CQE causes user notification, > + * the CQ gets dis-aremd and must be re-aremd > + * by the user for a new notification. > + */ > + WRITE_ONCE(cq->notify->flags, SIW_NOTIFY_NOT); > > return true; > } > diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c > index 32dc79d0e898..e7f3a2379d9d 100644 > --- a/drivers/infiniband/sw/siw/siw_verbs.c > +++ b/drivers/infiniband/sw/siw/siw_verbs.c > @@ -1049,7 +1049,7 @@ int siw_create_cq(struct ib_cq *base_cq, const struct ib_cq_init_attr *attr, > > spin_lock_init(&cq->lock); > > - cq->notify = &((struct siw_cq_ctrl *)&cq->queue[size])->notify; > + cq->notify = (struct siw_cq_ctrl *)&cq->queue[size]; > > if (udata) { > struct siw_uresp_create_cq uresp = {}; > @@ -1141,11 +1141,17 @@ int siw_req_notify_cq(struct ib_cq *base_cq, enum ib_cq_notify_flags flags) > siw_dbg_cq(cq, "flags: 0x%02x\n", flags); > > if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_SOLICITED) > - /* CQ event for next solicited completion */ > - smp_store_mb(*cq->notify, SIW_NOTIFY_SOLICITED); > + /* > + * Enable CQ event for next solicited completion. > + * and make it visible to all associated producers. > + */ > + smp_store_mb(cq->notify->flags, SIW_NOTIFY_SOLICITED); > else > - /* CQ event for any signalled completion */ > - smp_store_mb(*cq->notify, SIW_NOTIFY_ALL); > + /* > + * Enable CQ event for any signalled completion. > + * and make it visible to all associated producers. > + */ > + smp_store_mb(cq->notify->flags, SIW_NOTIFY_ALL); this isn't what we talked about, is it? > index 7de68f1dc707..af735f55b291 100644 > --- a/include/uapi/rdma/siw-abi.h > +++ b/include/uapi/rdma/siw-abi.h > @@ -180,6 +180,7 @@ struct siw_cqe { > * to control CQ arming. > */ > struct siw_cq_ctrl { > - __aligned_u64 notify; > + __u32 flags; > + __u32 pad; The commit message needs to explain why this is compatible with existing user space, if it is even is safe.. Jason
On Tue, 2019-08-06 at 09:10 -0300, Jason Gunthorpe wrote: > On Mon, Aug 05, 2019 at 04:17:08PM +0200, Bernard Metzler wrote: > > Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com> > > --- > > Don't send patches with empty commit messages. Every patch must have a > comprehensive commit message from now on. > > > drivers/infiniband/sw/siw/Kconfig | 2 +- > > drivers/infiniband/sw/siw/siw.h | 2 +- > > drivers/infiniband/sw/siw/siw_qp.c | 14 ++++++++++---- He had a decent commit log message, it was just in the cover letter. Bernard, on single patch submissions, skip the cover letter and just send the patch by itself. Then the nice explanation you gave in the cover letter should go in the commit message itself.
-----"Doug Ledford" <dledford@redhat.com> wrote: ----- >To: "Jason Gunthorpe" <jgg@ziepe.ca>, "Bernard Metzler" ><bmt@zurich.ibm.com> >From: "Doug Ledford" <dledford@redhat.com> >Date: 08/06/2019 02:33PM >Cc: linux-rdma@vger.kernel.org >Subject: [EXTERNAL] Re: [PATCH 1/1] Make user mmapped CQ arming flags >field 32 bit size to remove 64 bit architecture dependency of siw. > >On Tue, 2019-08-06 at 09:10 -0300, Jason Gunthorpe wrote: >> On Mon, Aug 05, 2019 at 04:17:08PM +0200, Bernard Metzler wrote: >> > Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com> >> > --- >> >> Don't send patches with empty commit messages. Every patch must >have a >> comprehensive commit message from now on. >> >> > drivers/infiniband/sw/siw/Kconfig | 2 +- >> > drivers/infiniband/sw/siw/siw.h | 2 +- >> > drivers/infiniband/sw/siw/siw_qp.c | 14 ++++++++++---- > >He had a decent commit log message, it was just in the cover letter. >Bernard, on single patch submissions, skip the cover letter and just >send the patch by itself. Then the nice explanation you gave in the >cover letter should go in the commit message itself. > sorry about this. Frankly I am still newbie here and obviously and unfortunately behave like this... A lame excuse, but I'll try hard to improve. Bernard.
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: ----- >To: "Bernard Metzler" <bmt@zurich.ibm.com> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >Date: 08/06/2019 02:10PM >Cc: linux-rdma@vger.kernel.org >Subject: [EXTERNAL] Re: [PATCH 1/1] Make user mmapped CQ arming flags >field 32 bit size to remove 64 bit architecture dependency of siw. > >On Mon, Aug 05, 2019 at 04:17:08PM +0200, Bernard Metzler wrote: >> Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com> >> --- > >Don't send patches with empty commit messages. Every patch must have >a >comprehensive commit message from now on. Sorry about this. As Doug pointed out - I sent an extra cover letter. > >> drivers/infiniband/sw/siw/Kconfig | 2 +- >> drivers/infiniband/sw/siw/siw.h | 2 +- >> drivers/infiniband/sw/siw/siw_qp.c | 14 ++++++++++---- >> drivers/infiniband/sw/siw/siw_verbs.c | 16 +++++++++++----- >> include/uapi/rdma/siw-abi.h | 3 ++- >> 5 files changed, 25 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/infiniband/sw/siw/Kconfig >b/drivers/infiniband/sw/siw/Kconfig >> index dace276aea14..b622fc62f2cd 100644 >> --- a/drivers/infiniband/sw/siw/Kconfig >> +++ b/drivers/infiniband/sw/siw/Kconfig >> @@ -1,6 +1,6 @@ >> config RDMA_SIW >> tristate "Software RDMA over TCP/IP (iWARP) driver" >> - depends on INET && INFINIBAND && LIBCRC32C && 64BIT >> + depends on INET && INFINIBAND && LIBCRC32C >> select DMA_VIRT_OPS >> help >> This driver implements the iWARP RDMA transport over >> diff --git a/drivers/infiniband/sw/siw/siw.h >b/drivers/infiniband/sw/siw/siw.h >> index 03fd7b2f595f..77b1aabf6ff3 100644 >> --- a/drivers/infiniband/sw/siw/siw.h >> +++ b/drivers/infiniband/sw/siw/siw.h >> @@ -214,7 +214,7 @@ struct siw_wqe { >> struct siw_cq { >> struct ib_cq base_cq; >> spinlock_t lock; >> - u64 *notify; >> + struct siw_cq_ctrl *notify; >> struct siw_cqe *queue; >> u32 cq_put; >> u32 cq_get; >> diff --git a/drivers/infiniband/sw/siw/siw_qp.c >b/drivers/infiniband/sw/siw/siw_qp.c >> index e27bd5b35b96..0990307c5d2c 100644 >> --- a/drivers/infiniband/sw/siw/siw_qp.c >> +++ b/drivers/infiniband/sw/siw/siw_qp.c >> @@ -1013,18 +1013,24 @@ int siw_activate_tx(struct siw_qp *qp) >> */ >> static bool siw_cq_notify_now(struct siw_cq *cq, u32 flags) >> { >> - u64 cq_notify; >> + u32 cq_notify; >> >> if (!cq->base_cq.comp_handler) >> return false; >> >> - cq_notify = READ_ONCE(*cq->notify); >> + /* Read application shared notification state */ >> + cq_notify = READ_ONCE(cq->notify->flags); >> >> if ((cq_notify & SIW_NOTIFY_NEXT_COMPLETION) || >> ((cq_notify & SIW_NOTIFY_SOLICITED) && >> (flags & SIW_WQE_SOLICITED))) { >> - /* dis-arm CQ */ >> - smp_store_mb(*cq->notify, SIW_NOTIFY_NOT); >> + /* >> + * CQ notification is one-shot: Since the >> + * current CQE causes user notification, >> + * the CQ gets dis-aremd and must be re-aremd >> + * by the user for a new notification. >> + */ >> + WRITE_ONCE(cq->notify->flags, SIW_NOTIFY_NOT); >> >> return true; >> } >> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c >b/drivers/infiniband/sw/siw/siw_verbs.c >> index 32dc79d0e898..e7f3a2379d9d 100644 >> --- a/drivers/infiniband/sw/siw/siw_verbs.c >> +++ b/drivers/infiniband/sw/siw/siw_verbs.c >> @@ -1049,7 +1049,7 @@ int siw_create_cq(struct ib_cq *base_cq, >const struct ib_cq_init_attr *attr, >> >> spin_lock_init(&cq->lock); >> >> - cq->notify = &((struct siw_cq_ctrl *)&cq->queue[size])->notify; >> + cq->notify = (struct siw_cq_ctrl *)&cq->queue[size]; >> >> if (udata) { >> struct siw_uresp_create_cq uresp = {}; >> @@ -1141,11 +1141,17 @@ int siw_req_notify_cq(struct ib_cq >*base_cq, enum ib_cq_notify_flags flags) >> siw_dbg_cq(cq, "flags: 0x%02x\n", flags); >> >> if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_SOLICITED) >> - /* CQ event for next solicited completion */ >> - smp_store_mb(*cq->notify, SIW_NOTIFY_SOLICITED); >> + /* >> + * Enable CQ event for next solicited completion. >> + * and make it visible to all associated producers. >> + */ >> + smp_store_mb(cq->notify->flags, SIW_NOTIFY_SOLICITED); >> else >> - /* CQ event for any signalled completion */ >> - smp_store_mb(*cq->notify, SIW_NOTIFY_ALL); >> + /* >> + * Enable CQ event for any signalled completion. >> + * and make it visible to all associated producers. >> + */ >> + smp_store_mb(cq->notify->flags, SIW_NOTIFY_ALL); > >this isn't what we talked about, is it? I actually abandoned the test_and_clear_bit() thing, since it requires an unsigned long as the bitfield. This would make the abi-file arch dependent with the hassle of #ifdef 64 or 32 bit stuff in there. > >> index 7de68f1dc707..af735f55b291 100644 >> --- a/include/uapi/rdma/siw-abi.h >> +++ b/include/uapi/rdma/siw-abi.h >> @@ -180,6 +180,7 @@ struct siw_cqe { >> * to control CQ arming. >> */ >> struct siw_cq_ctrl { >> - __aligned_u64 notify; >> + __u32 flags; >> + __u32 pad; > >The commit message needs to explain why this is compatible with >existing user space, if it is even is safe.. > Old libsiw would remain compatible with the new layout, since it simply reads the 32bit 'flags' and zeroed 32bit 'pad' into a 64bit 'notify', ending with reading the same bits. Thanks Bernard.
On Tue, Aug 06, 2019 at 02:53:49PM +0000, Bernard Metzler wrote: > >> index 7de68f1dc707..af735f55b291 100644 > >> +++ b/include/uapi/rdma/siw-abi.h > >> @@ -180,6 +180,7 @@ struct siw_cqe { > >> * to control CQ arming. > >> */ > >> struct siw_cq_ctrl { > >> - __aligned_u64 notify; > >> + __u32 flags; > >> + __u32 pad; > > > >The commit message needs to explain why this is compatible with > >existing user space, if it is even is safe.. > > > Old libsiw would remain compatible with the new layout, since it > simply reads the 32bit 'flags' and zeroed 32bit 'pad' into a 64bit > 'notify', ending with reading the same bits. Even on big endian? Jason
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: ----- >To: "Bernard Metzler" <BMT@zurich.ibm.com> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >Date: 08/06/2019 05:31PM >Cc: linux-rdma@vger.kernel.org >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field >32 bit size to remove 64 bit architecture dependency of siw. > >On Tue, Aug 06, 2019 at 02:53:49PM +0000, Bernard Metzler wrote: > >> >> index 7de68f1dc707..af735f55b291 100644 >> >> +++ b/include/uapi/rdma/siw-abi.h >> >> @@ -180,6 +180,7 @@ struct siw_cqe { >> >> * to control CQ arming. >> >> */ >> >> struct siw_cq_ctrl { >> >> - __aligned_u64 notify; >> >> + __u32 flags; >> >> + __u32 pad; >> > >> >The commit message needs to explain why this is compatible with >> >existing user space, if it is even is safe.. >> > >> Old libsiw would remain compatible with the new layout, since it >> simply reads the 32bit 'flags' and zeroed 32bit 'pad' into a 64bit >> 'notify', ending with reading the same bits. > >Even on big endian? > Well I do not have access to a BE system right now to verify. But on a BE system, the lowest 3 bits (which are in use) of the first 32bit variable 'flags' shall be the lowest (leftmost) 3 bits of an 'overlayed' 64bit variable 'notify' as well...
On Tue, Aug 06, 2019 at 04:36:26PM +0000, Bernard Metzler wrote: > > >To: "Bernard Metzler" <BMT@zurich.ibm.com> > >From: "Jason Gunthorpe" <jgg@ziepe.ca> > >Date: 08/06/2019 05:31PM > >Cc: linux-rdma@vger.kernel.org > >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field > >32 bit size to remove 64 bit architecture dependency of siw. > > > >On Tue, Aug 06, 2019 at 02:53:49PM +0000, Bernard Metzler wrote: > > > >> >> index 7de68f1dc707..af735f55b291 100644 > >> >> +++ b/include/uapi/rdma/siw-abi.h > >> >> @@ -180,6 +180,7 @@ struct siw_cqe { > >> >> * to control CQ arming. > >> >> */ > >> >> struct siw_cq_ctrl { > >> >> - __aligned_u64 notify; > >> >> + __u32 flags; > >> >> + __u32 pad; > >> > > >> >The commit message needs to explain why this is compatible with > >> >existing user space, if it is even is safe.. > >> > > >> Old libsiw would remain compatible with the new layout, since it > >> simply reads the 32bit 'flags' and zeroed 32bit 'pad' into a 64bit > >> 'notify', ending with reading the same bits. > > > >Even on big endian? > > > Well I do not have access to a BE system right now to verify. > But on a BE system, the lowest 3 bits (which are in use) of the first > 32bit variable 'flags' shall be the lowest (leftmost) 3 bits of an > 'overlayed' 64bit variable 'notify' as well... One of LE or BE won't work with this scheme, it can't, the flag bit will end up in the pad. Jason
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: ----- >To: "Bernard Metzler" <BMT@zurich.ibm.com> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >Date: 08/06/2019 06:39PM >Cc: linux-rdma@vger.kernel.org >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field >32 bit size to remove 64 bit architecture dependency of siw. > >On Tue, Aug 06, 2019 at 04:36:26PM +0000, Bernard Metzler wrote: >> >> >To: "Bernard Metzler" <BMT@zurich.ibm.com> >> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >> >Date: 08/06/2019 05:31PM >> >Cc: linux-rdma@vger.kernel.org >> >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags >field >> >32 bit size to remove 64 bit architecture dependency of siw. >> > >> >On Tue, Aug 06, 2019 at 02:53:49PM +0000, Bernard Metzler wrote: >> > >> >> >> index 7de68f1dc707..af735f55b291 100644 >> >> >> +++ b/include/uapi/rdma/siw-abi.h >> >> >> @@ -180,6 +180,7 @@ struct siw_cqe { >> >> >> * to control CQ arming. >> >> >> */ >> >> >> struct siw_cq_ctrl { >> >> >> - __aligned_u64 notify; >> >> >> + __u32 flags; >> >> >> + __u32 pad; >> >> > >> >> >The commit message needs to explain why this is compatible with >> >> >existing user space, if it is even is safe.. >> >> > >> >> Old libsiw would remain compatible with the new layout, since it >> >> simply reads the 32bit 'flags' and zeroed 32bit 'pad' into a >64bit >> >> 'notify', ending with reading the same bits. >> > >> >Even on big endian? >> > >> Well I do not have access to a BE system right now to verify. >> But on a BE system, the lowest 3 bits (which are in use) of the >first >> 32bit variable 'flags' shall be the lowest (leftmost) 3 bits of an >> 'overlayed' 64bit variable 'notify' as well... > >One of LE or BE won't work with this scheme, it can't, the flag bit >will end up in the pad. > Sitting here on a x86 (LE), and it works. On a 64bits machine, two consecutive 32bits are obviously reordered in memory. Leaves 32bit LE broken, which is currently not supported. Anyway, what would you suggest as the best path forward? A new ABI version? If we move to test_and_clear_bit(), 'flags' size would become architecture dependent...and we would break the ABI as well, no? Best regards, Bernard.
On Tue, Aug 06, 2019 at 05:01:49PM +0000, Bernard Metzler wrote: > > >To: "Bernard Metzler" <BMT@zurich.ibm.com> > >From: "Jason Gunthorpe" <jgg@ziepe.ca> > >Date: 08/06/2019 06:39PM > >Cc: linux-rdma@vger.kernel.org > >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field > >32 bit size to remove 64 bit architecture dependency of siw. > > > >On Tue, Aug 06, 2019 at 04:36:26PM +0000, Bernard Metzler wrote: > >> > >> >To: "Bernard Metzler" <BMT@zurich.ibm.com> > >> >From: "Jason Gunthorpe" <jgg@ziepe.ca> > >> >Date: 08/06/2019 05:31PM > >> >Cc: linux-rdma@vger.kernel.org > >> >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags > >field > >> >32 bit size to remove 64 bit architecture dependency of siw. > >> > > >> >On Tue, Aug 06, 2019 at 02:53:49PM +0000, Bernard Metzler wrote: > >> > > >> >> >> index 7de68f1dc707..af735f55b291 100644 > >> >> >> +++ b/include/uapi/rdma/siw-abi.h > >> >> >> @@ -180,6 +180,7 @@ struct siw_cqe { > >> >> >> * to control CQ arming. > >> >> >> */ > >> >> >> struct siw_cq_ctrl { > >> >> >> - __aligned_u64 notify; > >> >> >> + __u32 flags; > >> >> >> + __u32 pad; > >> >> > > >> >> >The commit message needs to explain why this is compatible with > >> >> >existing user space, if it is even is safe.. > >> >> > > >> >> Old libsiw would remain compatible with the new layout, since it > >> >> simply reads the 32bit 'flags' and zeroed 32bit 'pad' into a > >64bit > >> >> 'notify', ending with reading the same bits. > >> > > >> >Even on big endian? > >> > > >> Well I do not have access to a BE system right now to verify. > >> But on a BE system, the lowest 3 bits (which are in use) of the > >first > >> 32bit variable 'flags' shall be the lowest (leftmost) 3 bits of an > >> 'overlayed' 64bit variable 'notify' as well... > > > >One of LE or BE won't work with this scheme, it can't, the flag bit > >will end up in the pad. > > > Sitting here on a x86 (LE), and it works. On a 64bits machine, > two consecutive 32bits are obviously reordered in memory. Leaves > 32bit LE broken, which is currently not supported. ? I still think 64 bit BE will be broken as the low two bits will overlap the pad, not the new flags. > Anyway, what would you suggest as the best path forward? A new ABI > version? If we move to test_and_clear_bit(), 'flags' size would > become architecture dependent...and we would break the ABI as well, > no? Maybe a #ifdef __big_endian__ and swap the order of the pad/flags ? Jason
fg-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: ----- >To: "Bernard Metzler" <BMT@zurich.ibm.com> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >Date: 08/06/2019 07:35PM >Cc: linux-rdma@vger.kernel.org >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field >32 bit size to remove 64 bit architecture dependency of siw. > >On Tue, Aug 06, 2019 at 05:01:49PM +0000, Bernard Metzler wrote: >> >> >To: "Bernard Metzler" <BMT@zurich.ibm.com> >> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >> >Date: 08/06/2019 06:39PM >> >Cc: linux-rdma@vger.kernel.org >> >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags >field >> >32 bit size to remove 64 bit architecture dependency of siw. >> > >> >On Tue, Aug 06, 2019 at 04:36:26PM +0000, Bernard Metzler wrote: >> >> >> >> >To: "Bernard Metzler" <BMT@zurich.ibm.com> >> >> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >> >> >Date: 08/06/2019 05:31PM >> >> >Cc: linux-rdma@vger.kernel.org >> >> >Subject: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags >> >field >> >> >32 bit size to remove 64 bit architecture dependency of siw. >> >> > >> >> >On Tue, Aug 06, 2019 at 02:53:49PM +0000, Bernard Metzler >wrote: >> >> > >> >> >> >> index 7de68f1dc707..af735f55b291 100644 >> >> >> >> +++ b/include/uapi/rdma/siw-abi.h >> >> >> >> @@ -180,6 +180,7 @@ struct siw_cqe { >> >> >> >> * to control CQ arming. >> >> >> >> */ >> >> >> >> struct siw_cq_ctrl { >> >> >> >> - __aligned_u64 notify; >> >> >> >> + __u32 flags; >> >> >> >> + __u32 pad; >> >> >> > >> >> >> >The commit message needs to explain why this is compatible >with >> >> >> >existing user space, if it is even is safe.. >> >> >> > >> >> >> Old libsiw would remain compatible with the new layout, since >it >> >> >> simply reads the 32bit 'flags' and zeroed 32bit 'pad' into a >> >64bit >> >> >> 'notify', ending with reading the same bits. >> >> > >> >> >Even on big endian? >> >> > >> >> Well I do not have access to a BE system right now to verify. >> >> But on a BE system, the lowest 3 bits (which are in use) of the >> >first >> >> 32bit variable 'flags' shall be the lowest (leftmost) 3 bits of >an >> >> 'overlayed' 64bit variable 'notify' as well... >> > >> >One of LE or BE won't work with this scheme, it can't, the flag >bit >> >will end up in the pad. >> > >> Sitting here on a x86 (LE), and it works. On a 64bits machine, >> two consecutive 32bits are obviously reordered in memory. Leaves >> 32bit LE broken, which is currently not supported. > >? I still think 64 bit BE will be broken as the low two bits will >overlap the pad, not the new flags. > It hurts, but I did finally setup qemu with a ppc image to check, and so you are right! ... #ifdef __BIG_ENDIAN__ seem to be available in both kernel and user land... But, general question: siw in its current shape isn't out for long, older versions from github are already broken with the abi. So, silently changing the abi at this stage of siw deployment is no option? It's a hassle to see an old mistake carried along forever with that #ifdef statement...no? Many thanks, Bernard. >> Anyway, what would you suggest as the best path forward? A new ABI >> version? If we move to test_and_clear_bit(), 'flags' size would >> become architecture dependent...and we would break the ABI as well, >> no? > >Maybe a #ifdef __big_endian__ and swap the order of the pad/flags ? > >Jason > >
On Wed, 2019-08-07 at 17:49 +0000, Bernard Metzler wrote: > > It hurts, but I did finally setup qemu with a ppc image to check, > and so you are right! > > ... > > #ifdef __BIG_ENDIAN__ > > seem to be available in both kernel and user land... > > But, general question: siw in its current shape isn't out > for long, older versions from github are already broken with > the abi. So, silently changing the abi at this stage of siw > deployment is no option? It's a hassle to see an old mistake > carried along forever with that #ifdef statement...no? I was thinking about that myself. This really hasn't been out long enough to completely tie our hands here. A point update to rdma-core will resolve any user side issues. Are they doing stable kernel patches for the last kernel? If so, we can fix it both places. No distros have picked up the original ABI in this short of a window and managed to get it into a shipped product, so we can notify any that might have picked up the broken version and get that updated too if we don't dilly dally and make the call quickly.
-----"Doug Ledford" <dledford@redhat.com> wrote: ----- >To: "Bernard Metzler" <BMT@zurich.ibm.com>, "Jason Gunthorpe" ><jgg@ziepe.ca> >From: "Doug Ledford" <dledford@redhat.com> >Date: 08/07/2019 08:53PM >Cc: linux-rdma@vger.kernel.org >Subject: [EXTERNAL] Re: Re: [PATCH 1/1] Make user mmapped CQ arming >flags field 32 bit size to remove 64 bit architecture dependency of >siw. > >On Wed, 2019-08-07 at 17:49 +0000, Bernard Metzler wrote: >> >> It hurts, but I did finally setup qemu with a ppc image to check, >> and so you are right! >> >> ... >> >> #ifdef __BIG_ENDIAN__ >> >> seem to be available in both kernel and user land... >> >> But, general question: siw in its current shape isn't out >> for long, older versions from github are already broken with >> the abi. So, silently changing the abi at this stage of siw >> deployment is no option? It's a hassle to see an old mistake >> carried along forever with that #ifdef statement...no? > >I was thinking about that myself. > >This really hasn't been out long enough to completely tie our hands >here. A point update to rdma-core will resolve any user side issues. > What we are aiming at is ensuring backward compatibility for 64bit-BE architectures, which are using siw since this year. The community is likely of size zero. I found it hard to find a machine, even in the ppc world, which let me test that BE thing. I ended up with an emulator. So I assume it is no real world issue to now just change the 64bits flag into 32bits and add a 32bit pad for ABI compliance. At the other hand, the change would be marginal (but awkward!): diff --git a/include/uapi/rdma/siw-abi.h b/include/uapi/rdma/siw-abi.h index af735f55b291..162b861ad2ac 100644 --- a/include/uapi/rdma/siw-abi.h +++ b/include/uapi/rdma/siw-abi.h @@ -180,7 +180,12 @@ struct siw_cqe { * to control CQ arming. */ struct siw_cq_ctrl { +#ifdef __BIG_ENDIAN__ + __u32 pad; + __u32 flags; +#else __u32 flags; __u32 pad; +#endif }; #endif I propose taking my initial patch (w/o conditional endianess code). But I can live with the ugly. Let me know. In any case, I will make a PR for the user lib, since we changed the variable to 32 bits... Thanks Bernard. >Are they doing stable kernel patches for the last kernel? If so, we >can >fix it both places. No distros have picked up the original ABI in >this >short of a window and managed to get it into a shipped product, so we >can notify any that might have picked up the broken version and get >that >updated too if we don't dilly dally and make the call quickly. > >-- >Doug Ledford <dledford@redhat.com> > GPG KeyID: B826A3330E572FDD > Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD > [attachment "signature.asc" removed by Bernard Metzler/Zurich/IBM]
On Thu, Aug 08, 2019 at 02:17:57PM +0000, Bernard Metzler wrote: > > >To: "Bernard Metzler" <BMT@zurich.ibm.com>, "Jason Gunthorpe" > ><jgg@ziepe.ca> > >From: "Doug Ledford" <dledford@redhat.com> > >Date: 08/07/2019 08:53PM > >Cc: linux-rdma@vger.kernel.org > >Subject: [EXTERNAL] Re: Re: [PATCH 1/1] Make user mmapped CQ arming > >flags field 32 bit size to remove 64 bit architecture dependency of > >siw. > > > >On Wed, 2019-08-07 at 17:49 +0000, Bernard Metzler wrote: > >> > >> It hurts, but I did finally setup qemu with a ppc image to check, > >> and so you are right! > >> > >> ... > >> > >> #ifdef __BIG_ENDIAN__ > >> > >> seem to be available in both kernel and user land... > >> > >> But, general question: siw in its current shape isn't out > >> for long, older versions from github are already broken with > >> the abi. So, silently changing the abi at this stage of siw > >> deployment is no option? It's a hassle to see an old mistake > >> carried along forever with that #ifdef statement...no? > > > >I was thinking about that myself. > > > >This really hasn't been out long enough to completely tie our hands > >here. A point update to rdma-core will resolve any user side issues. > > > > What we are aiming at is ensuring backward compatibility > for 64bit-BE architectures, which are using siw since this year. > The community is likely of size zero. > I found it hard to find a machine, even in the ppc world, which > let me test that BE thing. I ended up with an emulator. So I > assume it is no real world issue to now just change the 64bits > flag into 32bits and add a 32bit pad for ABI compliance. This seems reasonable to me, document all these details in the commit message. Jason
diff --git a/drivers/infiniband/sw/siw/Kconfig b/drivers/infiniband/sw/siw/Kconfig index dace276aea14..b622fc62f2cd 100644 --- a/drivers/infiniband/sw/siw/Kconfig +++ b/drivers/infiniband/sw/siw/Kconfig @@ -1,6 +1,6 @@ config RDMA_SIW tristate "Software RDMA over TCP/IP (iWARP) driver" - depends on INET && INFINIBAND && LIBCRC32C && 64BIT + depends on INET && INFINIBAND && LIBCRC32C select DMA_VIRT_OPS help This driver implements the iWARP RDMA transport over diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h index 03fd7b2f595f..77b1aabf6ff3 100644 --- a/drivers/infiniband/sw/siw/siw.h +++ b/drivers/infiniband/sw/siw/siw.h @@ -214,7 +214,7 @@ struct siw_wqe { struct siw_cq { struct ib_cq base_cq; spinlock_t lock; - u64 *notify; + struct siw_cq_ctrl *notify; struct siw_cqe *queue; u32 cq_put; u32 cq_get; diff --git a/drivers/infiniband/sw/siw/siw_qp.c b/drivers/infiniband/sw/siw/siw_qp.c index e27bd5b35b96..0990307c5d2c 100644 --- a/drivers/infiniband/sw/siw/siw_qp.c +++ b/drivers/infiniband/sw/siw/siw_qp.c @@ -1013,18 +1013,24 @@ int siw_activate_tx(struct siw_qp *qp) */ static bool siw_cq_notify_now(struct siw_cq *cq, u32 flags) { - u64 cq_notify; + u32 cq_notify; if (!cq->base_cq.comp_handler) return false; - cq_notify = READ_ONCE(*cq->notify); + /* Read application shared notification state */ + cq_notify = READ_ONCE(cq->notify->flags); if ((cq_notify & SIW_NOTIFY_NEXT_COMPLETION) || ((cq_notify & SIW_NOTIFY_SOLICITED) && (flags & SIW_WQE_SOLICITED))) { - /* dis-arm CQ */ - smp_store_mb(*cq->notify, SIW_NOTIFY_NOT); + /* + * CQ notification is one-shot: Since the + * current CQE causes user notification, + * the CQ gets dis-aremd and must be re-aremd + * by the user for a new notification. + */ + WRITE_ONCE(cq->notify->flags, SIW_NOTIFY_NOT); return true; } diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c index 32dc79d0e898..e7f3a2379d9d 100644 --- a/drivers/infiniband/sw/siw/siw_verbs.c +++ b/drivers/infiniband/sw/siw/siw_verbs.c @@ -1049,7 +1049,7 @@ int siw_create_cq(struct ib_cq *base_cq, const struct ib_cq_init_attr *attr, spin_lock_init(&cq->lock); - cq->notify = &((struct siw_cq_ctrl *)&cq->queue[size])->notify; + cq->notify = (struct siw_cq_ctrl *)&cq->queue[size]; if (udata) { struct siw_uresp_create_cq uresp = {}; @@ -1141,11 +1141,17 @@ int siw_req_notify_cq(struct ib_cq *base_cq, enum ib_cq_notify_flags flags) siw_dbg_cq(cq, "flags: 0x%02x\n", flags); if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_SOLICITED) - /* CQ event for next solicited completion */ - smp_store_mb(*cq->notify, SIW_NOTIFY_SOLICITED); + /* + * Enable CQ event for next solicited completion. + * and make it visible to all associated producers. + */ + smp_store_mb(cq->notify->flags, SIW_NOTIFY_SOLICITED); else - /* CQ event for any signalled completion */ - smp_store_mb(*cq->notify, SIW_NOTIFY_ALL); + /* + * Enable CQ event for any signalled completion. + * and make it visible to all associated producers. + */ + smp_store_mb(cq->notify->flags, SIW_NOTIFY_ALL); if (flags & IB_CQ_REPORT_MISSED_EVENTS) return cq->cq_put - cq->cq_get; diff --git a/include/uapi/rdma/siw-abi.h b/include/uapi/rdma/siw-abi.h index 7de68f1dc707..af735f55b291 100644 --- a/include/uapi/rdma/siw-abi.h +++ b/include/uapi/rdma/siw-abi.h @@ -180,6 +180,7 @@ struct siw_cqe { * to control CQ arming. */ struct siw_cq_ctrl { - __aligned_u64 notify; + __u32 flags; + __u32 pad; }; #endif
Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com> --- drivers/infiniband/sw/siw/Kconfig | 2 +- drivers/infiniband/sw/siw/siw.h | 2 +- drivers/infiniband/sw/siw/siw_qp.c | 14 ++++++++++---- drivers/infiniband/sw/siw/siw_verbs.c | 16 +++++++++++----- include/uapi/rdma/siw-abi.h | 3 ++- 5 files changed, 25 insertions(+), 12 deletions(-)