* [PATCH 0/1] Fix siw CQ processing for 32 bit archtecture support
@ 2019-08-05 14:17 Bernard Metzler
2019-08-05 14:17 ` [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw Bernard Metzler
0 siblings, 1 reply; 17+ messages in thread
From: Bernard Metzler @ 2019-08-05 14:17 UTC (permalink / raw)
To: linux-rdma; +Cc: jgg, Bernard Metzler
Change driver/user shared (mmapped) CQ notification flags field
to unaligned 32-bits size. This enables building siw on 32-bit
architectures.
The original idea to introduce test_and_clear_bit() for testing CQ
arming during CQE processing was abandoned, since it would
require architecture spcific siw-abi notation: test_and_clear_bit()
expects an unsigned long field, which has an architecture specific
size.
This patch applies to 5.3-rc3.
Bernard Metzler (1):
Make user mmapped CQ arming flags field 32 bit size to remove 64 bit
architecture dependency of siw.
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(-)
--
2.17.2
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-05 14:17 [PATCH 0/1] Fix siw CQ processing for 32 bit archtecture support Bernard Metzler @ 2019-08-05 14:17 ` Bernard Metzler 2019-08-05 17:09 ` Leon Romanovsky 2019-08-06 12:10 ` Jason Gunthorpe 0 siblings, 2 replies; 17+ messages in thread From: Bernard Metzler @ 2019-08-05 14:17 UTC (permalink / raw) To: linux-rdma; +Cc: jgg, Bernard Metzler 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; }; #endif -- 2.17.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-05 14:17 ` [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw Bernard Metzler @ 2019-08-05 17:09 ` Leon Romanovsky 2019-08-06 11:58 ` Bernard Metzler 2019-08-06 12:10 ` Jason Gunthorpe 1 sibling, 1 reply; 17+ messages in thread From: Leon Romanovsky @ 2019-08-05 17:09 UTC (permalink / raw) To: Bernard Metzler; +Cc: linux-rdma, jgg 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-05 17:09 ` Leon Romanovsky @ 2019-08-06 11:58 ` Bernard Metzler 0 siblings, 0 replies; 17+ messages in thread From: Bernard Metzler @ 2019-08-06 11:58 UTC (permalink / raw) To: Leon Romanovsky; +Cc: linux-rdma, jgg -----"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) -- 2.17.2 >Thanks > >> }; >> #endif >> -- >> 2.17.2 >> > > ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-05 14:17 ` [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw Bernard Metzler 2019-08-05 17:09 ` Leon Romanovsky @ 2019-08-06 12:10 ` Jason Gunthorpe 2019-08-06 12:32 ` Doug Ledford 2019-08-06 14:53 ` Bernard Metzler 1 sibling, 2 replies; 17+ messages in thread From: Jason Gunthorpe @ 2019-08-06 12:10 UTC (permalink / raw) To: Bernard Metzler; +Cc: linux-rdma 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-06 12:10 ` Jason Gunthorpe @ 2019-08-06 12:32 ` Doug Ledford 2019-08-06 13:09 ` Bernard Metzler 2019-08-06 14:53 ` Bernard Metzler 1 sibling, 1 reply; 17+ messages in thread From: Doug Ledford @ 2019-08-06 12:32 UTC (permalink / raw) To: Jason Gunthorpe, Bernard Metzler; +Cc: linux-rdma [-- Attachment #1: Type: text/plain, Size: 901 bytes --] 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> GPG KeyID: B826A3330E572FDD Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-06 12:32 ` Doug Ledford @ 2019-08-06 13:09 ` Bernard Metzler 0 siblings, 0 replies; 17+ messages in thread From: Bernard Metzler @ 2019-08-06 13:09 UTC (permalink / raw) To: Doug Ledford; +Cc: Jason Gunthorpe, linux-rdma -----"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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-06 12:10 ` Jason Gunthorpe 2019-08-06 12:32 ` Doug Ledford @ 2019-08-06 14:53 ` Bernard Metzler 2019-08-06 15:31 ` Jason Gunthorpe 1 sibling, 1 reply; 17+ messages in thread From: Bernard Metzler @ 2019-08-06 14:53 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: linux-rdma -----"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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-06 14:53 ` Bernard Metzler @ 2019-08-06 15:31 ` Jason Gunthorpe 2019-08-06 16:36 ` Bernard Metzler 0 siblings, 1 reply; 17+ messages in thread From: Jason Gunthorpe @ 2019-08-06 15:31 UTC (permalink / raw) To: Bernard Metzler; +Cc: linux-rdma 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-06 15:31 ` Jason Gunthorpe @ 2019-08-06 16:36 ` Bernard Metzler 2019-08-06 16:39 ` Jason Gunthorpe 0 siblings, 1 reply; 17+ messages in thread From: Bernard Metzler @ 2019-08-06 16:36 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: linux-rdma -----"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... ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-06 16:36 ` Bernard Metzler @ 2019-08-06 16:39 ` Jason Gunthorpe 2019-08-06 17:01 ` Bernard Metzler 0 siblings, 1 reply; 17+ messages in thread From: Jason Gunthorpe @ 2019-08-06 16:39 UTC (permalink / raw) To: Bernard Metzler; +Cc: linux-rdma 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-06 16:39 ` Jason Gunthorpe @ 2019-08-06 17:01 ` Bernard Metzler 2019-08-06 17:35 ` Jason Gunthorpe 0 siblings, 1 reply; 17+ messages in thread From: Bernard Metzler @ 2019-08-06 17:01 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: linux-rdma -----"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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-06 17:01 ` Bernard Metzler @ 2019-08-06 17:35 ` Jason Gunthorpe 2019-08-07 17:49 ` Bernard Metzler 0 siblings, 1 reply; 17+ messages in thread From: Jason Gunthorpe @ 2019-08-06 17:35 UTC (permalink / raw) To: Bernard Metzler; +Cc: linux-rdma 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-06 17:35 ` Jason Gunthorpe @ 2019-08-07 17:49 ` Bernard Metzler 2019-08-07 18:53 ` Doug Ledford 0 siblings, 1 reply; 17+ messages in thread From: Bernard Metzler @ 2019-08-07 17:49 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: linux-rdma 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 > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-07 17:49 ` Bernard Metzler @ 2019-08-07 18:53 ` Doug Ledford 2019-08-08 14:17 ` Bernard Metzler 0 siblings, 1 reply; 17+ messages in thread From: Doug Ledford @ 2019-08-07 18:53 UTC (permalink / raw) To: Bernard Metzler, Jason Gunthorpe; +Cc: linux-rdma [-- Attachment #1: Type: text/plain, Size: 1253 bytes --] 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> GPG KeyID: B826A3330E572FDD Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-07 18:53 ` Doug Ledford @ 2019-08-08 14:17 ` Bernard Metzler 2019-08-08 15:39 ` Jason Gunthorpe 0 siblings, 1 reply; 17+ messages in thread From: Bernard Metzler @ 2019-08-08 14:17 UTC (permalink / raw) To: Doug Ledford; +Cc: Jason Gunthorpe, linux-rdma -----"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] ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw. 2019-08-08 14:17 ` Bernard Metzler @ 2019-08-08 15:39 ` Jason Gunthorpe 0 siblings, 0 replies; 17+ messages in thread From: Jason Gunthorpe @ 2019-08-08 15:39 UTC (permalink / raw) To: Bernard Metzler; +Cc: Doug Ledford, linux-rdma 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-08-08 15:39 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-08-05 14:17 [PATCH 0/1] Fix siw CQ processing for 32 bit archtecture support Bernard Metzler 2019-08-05 14:17 ` [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw Bernard Metzler 2019-08-05 17:09 ` Leon Romanovsky 2019-08-06 11:58 ` Bernard Metzler 2019-08-06 12:10 ` Jason Gunthorpe 2019-08-06 12:32 ` Doug Ledford 2019-08-06 13:09 ` Bernard Metzler 2019-08-06 14:53 ` Bernard Metzler 2019-08-06 15:31 ` Jason Gunthorpe 2019-08-06 16:36 ` Bernard Metzler 2019-08-06 16:39 ` Jason Gunthorpe 2019-08-06 17:01 ` Bernard Metzler 2019-08-06 17:35 ` Jason Gunthorpe 2019-08-07 17:49 ` Bernard Metzler 2019-08-07 18:53 ` Doug Ledford 2019-08-08 14:17 ` Bernard Metzler 2019-08-08 15:39 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox