Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] Make user mmapped CQ arming flags field 32-bit size to remove 64-bit architecture dependency of siw.
@ 2019-08-09 15:18 Bernard Metzler
  2019-08-12 15:17 ` Doug Ledford
  0 siblings, 1 reply; 4+ messages in thread
From: Bernard Metzler @ 2019-08-09 15:18 UTC (permalink / raw)
  To: linux-rdma; +Cc: jgg, Bernard Metzler

This patch changes the driver/user shared (mmapped) CQ notification
flags field from unsigned 64-bits size to unsigned 32-bits size. This
enables building siw on 32-bit architectures.

This patch changes the siw-abi. On previously supported 64-bits
little-endian architectures, the old siw user library remains
usable, since the used 2 lowest bits of the new 32-bits field reside
at the same memory location as those of the old 64-bits field.
On 64-bits big-endian systems, the changes would break compatibility.
Given the very short time of availability of siw with the current ABI,
we do not expect current usage of siw on 64-bit big-endian systems.

An according patch to change the siw user library fitting the new ABI
will be provided to rdma-core.

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	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] Make user mmapped CQ arming flags field 32-bit size to remove 64-bit architecture dependency of siw.
  2019-08-09 15:18 [PATCH v2] Make user mmapped CQ arming flags field 32-bit size to remove 64-bit architecture dependency of siw Bernard Metzler
@ 2019-08-12 15:17 ` Doug Ledford
  2019-08-13 10:30   ` Leon Romanovsky
  2019-08-13 14:24   ` Bernard Metzler
  0 siblings, 2 replies; 4+ messages in thread
From: Doug Ledford @ 2019-08-12 15:17 UTC (permalink / raw)
  To: Bernard Metzler, linux-rdma; +Cc: jgg

[-- Attachment #1: Type: text/plain, Size: 5998 bytes --]

On Fri, 2019-08-09 at 17:18 +0200, Bernard Metzler wrote:
> This patch changes the driver/user shared (mmapped) CQ notification
> flags field from unsigned 64-bits size to unsigned 32-bits size. This
> enables building siw on 32-bit architectures.
> 
> This patch changes the siw-abi. On previously supported 64-bits
> little-endian architectures, the old siw user library remains
> usable, since the used 2 lowest bits of the new 32-bits field reside
> at the same memory location as those of the old 64-bits field.
> On 64-bits big-endian systems, the changes would break compatibility.
> Given the very short time of availability of siw with the current ABI,
> we do not expect current usage of siw on 64-bit big-endian systems.
> 
> An according patch to change the siw user library fitting the new ABI
> will be provided to rdma-core.

I changed the commit message somewhat.  The siw driver was just taken
into the upstream kernel this merge window, so there is no need to be
apologetic about abi breakage, there are *no* released kernels with a
prior abi.  We are only guaranteeing abi compatibility for the official
siw as taken into the upstream kernel and into rdma-core, and those will
be kept in sync starting with their first official release, which has
not yet happened.  Until this rc cycle is complete, we can fix up
anything that needs fixed up, so if there are any other abi issues you
think you would like to address, well, chop! chop! ;-)

With that said, thanks, applied to for-rc.

> 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

-- 
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] 4+ messages in thread

* Re: [PATCH v2] Make user mmapped CQ arming flags field 32-bit size to remove 64-bit architecture dependency of siw.
  2019-08-12 15:17 ` Doug Ledford
@ 2019-08-13 10:30   ` Leon Romanovsky
  2019-08-13 14:24   ` Bernard Metzler
  1 sibling, 0 replies; 4+ messages in thread
From: Leon Romanovsky @ 2019-08-13 10:30 UTC (permalink / raw)
  To: Doug Ledford; +Cc: Bernard Metzler, linux-rdma, jgg

On Mon, Aug 12, 2019 at 11:17:00AM -0400, Doug Ledford wrote:
> On Fri, 2019-08-09 at 17:18 +0200, Bernard Metzler wrote:
> > This patch changes the driver/user shared (mmapped) CQ notification
> > flags field from unsigned 64-bits size to unsigned 32-bits size. This
> > enables building siw on 32-bit architectures.
> >
> > This patch changes the siw-abi. On previously supported 64-bits
> > little-endian architectures, the old siw user library remains
> > usable, since the used 2 lowest bits of the new 32-bits field reside
> > at the same memory location as those of the old 64-bits field.
> > On 64-bits big-endian systems, the changes would break compatibility.
> > Given the very short time of availability of siw with the current ABI,
> > we do not expect current usage of siw on 64-bit big-endian systems.
> >
> > An according patch to change the siw user library fitting the new ABI
> > will be provided to rdma-core.
>
> I changed the commit message somewhat.  The siw driver was just taken
> into the upstream kernel this merge window, so there is no need to be
> apologetic about abi breakage, there are *no* released kernels with a
> prior abi.  We are only guaranteeing abi compatibility for the official
> siw as taken into the upstream kernel and into rdma-core, and those will
> be kept in sync starting with their first official release, which has
> not yet happened.  Until this rc cycle is complete, we can fix up
> anything that needs fixed up, so if there are any other abi issues you
> think you would like to address, well, chop! chop! ;-)
>
> With that said, thanks, applied to for-rc.
>

Please send relevant change to rdma-core too.

Thanks

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Re: [PATCH v2] Make user mmapped CQ arming flags field 32-bit size to remove 64-bit architecture dependency of siw.
  2019-08-12 15:17 ` Doug Ledford
  2019-08-13 10:30   ` Leon Romanovsky
@ 2019-08-13 14:24   ` Bernard Metzler
  1 sibling, 0 replies; 4+ messages in thread
From: Bernard Metzler @ 2019-08-13 14:24 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: Doug Ledford, linux-rdma, jgg

-----"Leon Romanovsky" <leon@kernel.org> wrote: -----

>To: "Doug Ledford" <dledford@redhat.com>
>From: "Leon Romanovsky" <leon@kernel.org>
>Date: 08/13/2019 12:30PM
>Cc: "Bernard Metzler" <bmt@zurich.ibm.com>,
>linux-rdma@vger.kernel.org, jgg@ziepe.ca
>Subject: [EXTERNAL] Re: [PATCH v2] Make user mmapped CQ arming flags
>field 32-bit size to remove 64-bit architecture dependency of siw.
>
>On Mon, Aug 12, 2019 at 11:17:00AM -0400, Doug Ledford wrote:
>> On Fri, 2019-08-09 at 17:18 +0200, Bernard Metzler wrote:
>> > This patch changes the driver/user shared (mmapped) CQ
>notification
>> > flags field from unsigned 64-bits size to unsigned 32-bits size.
>This
>> > enables building siw on 32-bit architectures.
>> >
>> > This patch changes the siw-abi. On previously supported 64-bits
>> > little-endian architectures, the old siw user library remains
>> > usable, since the used 2 lowest bits of the new 32-bits field
>reside
>> > at the same memory location as those of the old 64-bits field.
>> > On 64-bits big-endian systems, the changes would break
>compatibility.
>> > Given the very short time of availability of siw with the current
>ABI,
>> > we do not expect current usage of siw on 64-bit big-endian
>systems.
>> >
>> > An according patch to change the siw user library fitting the new
>ABI
>> > will be provided to rdma-core.
>>
>> I changed the commit message somewhat.  The siw driver was just
>taken
>> into the upstream kernel this merge window, so there is no need to
>be
>> apologetic about abi breakage, there are *no* released kernels with
>a
>> prior abi.  We are only guaranteeing abi compatibility for the
>official
>> siw as taken into the upstream kernel and into rdma-core, and those
>will
>> be kept in sync starting with their first official release, which
>has
>> not yet happened.  Until this rc cycle is complete, we can fix up
>> anything that needs fixed up, so if there are any other abi issues
>you
>> think you would like to address, well, chop! chop! ;-)
>>
>> With that said, thanks, applied to for-rc.
>>
>
>Please send relevant change to rdma-core too.
>

Thanks for the reminder, absolutely!

I created a PR#564.

Thanks very much,
Bernard.


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-09 15:18 [PATCH v2] Make user mmapped CQ arming flags field 32-bit size to remove 64-bit architecture dependency of siw Bernard Metzler
2019-08-12 15:17 ` Doug Ledford
2019-08-13 10:30   ` Leon Romanovsky
2019-08-13 14:24   ` Bernard Metzler

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org linux-rdma@archiver.kernel.org
	public-inbox-index linux-rdma


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/ public-inbox