[PATCH 0/1] Fix siw CQ processing for 32 bit archtecture support

[PATCH 0/1] Fix siw CQ processing for 32 bit archtecture support

[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

[PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[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

Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Leon Romanovsky]

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
>

Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Bernard Metzler]

-----"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
>>
>
>

Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Jason Gunthorpe]

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

Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Doug Ledford]

[-- 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 --]

RE: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Bernard Metzler]

-----"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.

Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Bernard Metzler]

-----"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.

Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Jason Gunthorpe]

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

Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Bernard Metzler]

-----"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...

Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Jason Gunthorpe]

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

Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Bernard Metzler]

-----"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.

Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Jason Gunthorpe]

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

Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Bernard Metzler]

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
>
>

Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Doug Ledford]

[-- 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 --]

Re: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Bernard Metzler]

-----"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]

Re: Re: Re: [PATCH 1/1] Make user mmapped CQ arming flags field 32 bit size to remove 64 bit architecture dependency of siw.

[Jason Gunthorpe]

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