-----"Leon Romanovsky" <leon@xxxxxxxxxx> wrote: -----
>To: "Bernard Metzler" <bmt@xxxxxxxxxxxxxx>
>From: "Leon Romanovsky" <leon@xxxxxxxxxx>
>Date: 08/05/2019 07:09PM
>Cc: linux-rdma@xxxxxxxxxxxxxxx, jgg@xxxxxxxx
>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@xxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx>
---
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
>>
>
>
