[MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Håkon Bugge]

The Dynamic Interrupt Moderation mechanism can only be used by ULPs
using ib_alloc_cq() and family. We extend DIM to also cover legacy
ULPs using ib_create_cq(). The last commit takes advantage of this end
uses DIM in RDS.

Håkon Bugge (2):
  RDMA/core: Enable legacy ULPs to use RDMA DIM
  rds: ib: Add Dynamic Interrupt Moderation to CQs

 drivers/infiniband/core/cq.c    |  7 +++++--
 drivers/infiniband/core/cq.h    | 16 ++++++++++++++++
 drivers/infiniband/core/verbs.c |  6 ++++++
 net/rds/ib_cm.c                 | 10 ++++++++++
 4 files changed, 37 insertions(+), 2 deletions(-)
 create mode 100644 drivers/infiniband/core/cq.h

--
2.43.5

[MAINLINE 1/2] RDMA/core: Enable legacy ULPs to use RDMA DIM

[Håkon Bugge]

The RDMA DIM mechanism is not available for legacy ULPs using
ib_create_cq(). Hence, enable it in ib_create_cq_user() if
IB_CQ_MODERATE is set in cq_attr.flags.

This way, legacy ULPs can take advantage of RDMA DIM.

Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
---
 drivers/infiniband/core/cq.c    |  7 +++++--
 drivers/infiniband/core/cq.h    | 16 ++++++++++++++++
 drivers/infiniband/core/verbs.c |  6 ++++++
 3 files changed, 27 insertions(+), 2 deletions(-)
 create mode 100644 drivers/infiniband/core/cq.h

diff --git a/drivers/infiniband/core/cq.c b/drivers/infiniband/core/cq.c
index a70876a0a2312..8e582eca6987f 100644
--- a/drivers/infiniband/core/cq.c
+++ b/drivers/infiniband/core/cq.c
@@ -7,6 +7,7 @@
 #include <rdma/ib_verbs.h>
 
 #include "core_priv.h"
+#include "cq.h"
 
 #include <trace/events/rdma_core.h>
 /* Max size for shared CQ, may require tuning */
@@ -50,7 +51,7 @@ static void ib_cq_rdma_dim_work(struct work_struct *w)
 	cq->device->ops.modify_cq(cq, comps, usec);
 }
 
-static void rdma_dim_init(struct ib_cq *cq)
+void rdma_dim_init(struct ib_cq *cq)
 {
 	struct dim *dim;
 
@@ -70,8 +71,9 @@ static void rdma_dim_init(struct ib_cq *cq)
 
 	INIT_WORK(&dim->work, ib_cq_rdma_dim_work);
 }
+EXPORT_SYMBOL(rdma_dim_init);
 
-static void rdma_dim_destroy(struct ib_cq *cq)
+void rdma_dim_destroy(struct ib_cq *cq)
 {
 	if (!cq->dim)
 		return;
@@ -79,6 +81,7 @@ static void rdma_dim_destroy(struct ib_cq *cq)
 	cancel_work_sync(&cq->dim->work);
 	kfree(cq->dim);
 }
+EXPORT_SYMBOL(rdma_dim_destroy);
 
 static int __poll_cq(struct ib_cq *cq, int num_entries, struct ib_wc *wc)
 {
diff --git a/drivers/infiniband/core/cq.h b/drivers/infiniband/core/cq.h
new file mode 100644
index 0000000000000..c55f7d3f1cbf4
--- /dev/null
+++ b/drivers/infiniband/core/cq.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Let other files use rdma_dim_{init,destroy}
+ *
+ * Author: Håkon Bugge <haakon.bugge@oracle.com>
+ *
+ * Copyright (c) 2024 Oracle and/or its affiliates.
+ */
+
+#ifndef CQ_H
+#define CQ_H
+
+void rdma_dim_init(struct ib_cq *cq);
+void rdma_dim_destroy(struct ib_cq *cq);
+
+#endif
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 473ee0831307c..60a64ea77ae25 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -53,6 +53,7 @@
 #include <rdma/lag.h>
 
 #include "core_priv.h"
+#include "cq.h"
 #include <trace/events/rdma_core.h>
 
 static int ib_resolve_eth_dmac(struct ib_device *device,
@@ -2161,6 +2162,9 @@ struct ib_cq *__ib_create_cq(struct ib_device *device,
 		return ERR_PTR(ret);
 	}
 
+	if (cq_attr->flags & IB_CQ_MODERATE)
+		rdma_dim_init(cq);
+
 	rdma_restrack_add(&cq->res);
 	return cq;
 }
@@ -2187,6 +2191,8 @@ int ib_destroy_cq_user(struct ib_cq *cq, struct ib_udata *udata)
 	if (atomic_read(&cq->usecnt))
 		return -EBUSY;
 
+	rdma_dim_destroy(cq);
+
 	ret = cq->device->ops.destroy_cq(cq, udata);
 	if (ret)
 		return ret;
-- 
2.43.5

[MAINLINE 2/2] rds: ib: Add Dynamic Interrupt Moderation to CQs

[Håkon Bugge]

With the support from ib_core to use Dynamic Interrupt Moderation
(DIM) from legacy ULPs, which uses ib_create_cq(), we enable that
feature for the receive and send CQs in RDS.

A set of rds-stress runs have been done. bcopy read + write for
payload 8448 and 16640 bytes and ack/req of 256 bytes. Number of QPs
varies from 8 to 128, number of threads (i.e. rds-stress processes)
from one to 16 and a depth of four. A limit has been applied such that
the number of processes times the number of QPs never exceeds 128. All
in all, 61 rds-stress runs.

For brevity, only the rows showing a +/- 3% deviation or larger from
base is listed. The geometric mean of the ratios (IOPS_test /
IOPS_base) is calculated for all 61 runs, and that gives the best
possible "average" impact of the commits.

In the following, "base" is v6.11-rc7. "test" is the same
kernel with the following two commits:

       * rds: ib: Add Dynamic Interrupt Moderation to CQs (this commit)
       * RDMA/core: Enable legacy ULPs to use RDMA DIM

This is executed between two X8-2 with CX-5 using fw 16.35.3502. These
BM systems were instantiated with one VF, which were used for the
test:

                                 base     test
   ACK    REQ  QPS  THR  DEP     IOPS     IOPS  Percent
   256   8448    8    1    4   634463   658162      3.7
   256   8448    8    2    4   862648   997358     15.6
   256   8448    8    4    4   950458  1113991     17.2
   256   8448    8    8    4   932120  1127024     20.9
   256   8448    8   16    4   944977  1133885     20.0
  8448    256    8    2    4   858663   975563     13.6
  8448    256    8    4    4   934884  1098854     17.5
  8448    256    8    8    4   928247  1116015     20.2
  8448    256    8   16    4   938864  1123455     19.7
   256   8448   64    1    4   965985   918445     -4.9
  8448    256   64    1    4   963280   918239     -4.7
   256  16640    8    2    4   544670   582330      6.9
   256  16640    8    4    4   554873   597553      7.7
   256  16640    8    8    4   551799   597479      8.3
   256  16640    8   16    4   553041   597898      8.1
 16640    256    8    2    4   544644   578331      6.2
 16640    256    8    4    4   553944   594627      7.3
 16640    256    8    8    4   551388   594737      7.9
 16640    256    8   16    4   552986   596581      7.9
Geometric mean of ratios: 1.03

Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
---
 net/rds/ib_cm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 26b069e1999df..79603d86b6c02 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -259,6 +259,7 @@ static void rds_ib_cq_comp_handler_recv(struct ib_cq *cq, void *context)
 static void poll_scq(struct rds_ib_connection *ic, struct ib_cq *cq,
 		     struct ib_wc *wcs)
 {
+	int ncompleted = 0;
 	int nr, i;
 	struct ib_wc *wc;
 
@@ -276,7 +277,10 @@ static void poll_scq(struct rds_ib_connection *ic, struct ib_cq *cq,
 				rds_ib_mr_cqe_handler(ic, wc);
 
 		}
+		ncompleted += nr;
 	}
+	if (cq->dim)
+		rdma_dim(cq->dim, ncompleted);
 }
 
 static void rds_ib_tasklet_fn_send(unsigned long data)
@@ -304,6 +308,7 @@ static void poll_rcq(struct rds_ib_connection *ic, struct ib_cq *cq,
 		     struct ib_wc *wcs,
 		     struct rds_ib_ack_state *ack_state)
 {
+	int ncompleted = 0;
 	int nr, i;
 	struct ib_wc *wc;
 
@@ -316,7 +321,10 @@ static void poll_rcq(struct rds_ib_connection *ic, struct ib_cq *cq,
 
 			rds_ib_recv_cqe_handler(ic, wc, ack_state);
 		}
+		ncompleted += nr;
 	}
+	if (cq->dim)
+		rdma_dim(cq->dim, ncompleted);
 }
 
 static void rds_ib_tasklet_fn_recv(unsigned long data)
@@ -542,6 +550,7 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
 	ic->i_scq_vector = ibdev_get_unused_vector(rds_ibdev);
 	cq_attr.cqe = ic->i_send_ring.w_nr + fr_queue_space + 1;
 	cq_attr.comp_vector = ic->i_scq_vector;
+	cq_attr.flags |= IB_CQ_MODERATE;
 	ic->i_send_cq = ib_create_cq(dev, rds_ib_cq_comp_handler_send,
 				     rds_ib_cq_event_handler, conn,
 				     &cq_attr);
@@ -556,6 +565,7 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
 	ic->i_rcq_vector = ibdev_get_unused_vector(rds_ibdev);
 	cq_attr.cqe = ic->i_recv_ring.w_nr;
 	cq_attr.comp_vector = ic->i_rcq_vector;
+	cq_attr.flags |= IB_CQ_MODERATE;
 	ic->i_recv_cq = ib_create_cq(dev, rds_ib_cq_comp_handler_recv,
 				     rds_ib_cq_event_handler, conn,
 				     &cq_attr);
-- 
2.43.5

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Christoph Hellwig]

On Wed, Sep 18, 2024 at 10:35:50AM +0200, Håkon Bugge wrote:
> The Dynamic Interrupt Moderation mechanism can only be used by ULPs
> using ib_alloc_cq() and family. We extend DIM to also cover legacy
> ULPs using ib_create_cq(). The last commit takes advantage of this end
> uses DIM in RDS.

I would much prefer if you could move RDS off that horrible API finally
instead of investing more effort into it and making it more complicated.

Re: [MAINLINE 2/2] rds: ib: Add Dynamic Interrupt Moderation to CQs

[Zhu Yanjun]

在 2024/9/18 16:35, Håkon Bugge 写道:
> With the support from ib_core to use Dynamic Interrupt Moderation
> (DIM) from legacy ULPs, which uses ib_create_cq(), we enable that
> feature for the receive and send CQs in RDS.

Hi, Haakon

I am interested in this patch series. I just wonder if the performance 
of rds is increased after DIM is used in legacy ULPs?
That is, is there any benefit to legacy ULPs after DIM is used?

Do you have any test results about this DIM?

Thanks,
Zhu Yanjun

> 
> A set of rds-stress runs have been done. bcopy read + write for
> payload 8448 and 16640 bytes and ack/req of 256 bytes. Number of QPs
> varies from 8 to 128, number of threads (i.e. rds-stress processes)
> from one to 16 and a depth of four. A limit has been applied such that
> the number of processes times the number of QPs never exceeds 128. All
> in all, 61 rds-stress runs.
> 
> For brevity, only the rows showing a +/- 3% deviation or larger from
> base is listed. The geometric mean of the ratios (IOPS_test /
> IOPS_base) is calculated for all 61 runs, and that gives the best
> possible "average" impact of the commits.
> 
> In the following, "base" is v6.11-rc7. "test" is the same
> kernel with the following two commits:
> 
>         * rds: ib: Add Dynamic Interrupt Moderation to CQs (this commit)
>         * RDMA/core: Enable legacy ULPs to use RDMA DIM
> 
> This is executed between two X8-2 with CX-5 using fw 16.35.3502. These
> BM systems were instantiated with one VF, which were used for the
> test:
> 
>                                   base     test
>     ACK    REQ  QPS  THR  DEP     IOPS     IOPS  Percent
>     256   8448    8    1    4   634463   658162      3.7
>     256   8448    8    2    4   862648   997358     15.6
>     256   8448    8    4    4   950458  1113991     17.2
>     256   8448    8    8    4   932120  1127024     20.9
>     256   8448    8   16    4   944977  1133885     20.0
>    8448    256    8    2    4   858663   975563     13.6
>    8448    256    8    4    4   934884  1098854     17.5
>    8448    256    8    8    4   928247  1116015     20.2
>    8448    256    8   16    4   938864  1123455     19.7
>     256   8448   64    1    4   965985   918445     -4.9
>    8448    256   64    1    4   963280   918239     -4.7
>     256  16640    8    2    4   544670   582330      6.9
>     256  16640    8    4    4   554873   597553      7.7
>     256  16640    8    8    4   551799   597479      8.3
>     256  16640    8   16    4   553041   597898      8.1
>   16640    256    8    2    4   544644   578331      6.2
>   16640    256    8    4    4   553944   594627      7.3
>   16640    256    8    8    4   551388   594737      7.9
>   16640    256    8   16    4   552986   596581      7.9
> Geometric mean of ratios: 1.03
> 
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> ---
>   net/rds/ib_cm.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 26b069e1999df..79603d86b6c02 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -259,6 +259,7 @@ static void rds_ib_cq_comp_handler_recv(struct ib_cq *cq, void *context)
>   static void poll_scq(struct rds_ib_connection *ic, struct ib_cq *cq,
>   		     struct ib_wc *wcs)
>   {
> +	int ncompleted = 0;
>   	int nr, i;
>   	struct ib_wc *wc;
>   
> @@ -276,7 +277,10 @@ static void poll_scq(struct rds_ib_connection *ic, struct ib_cq *cq,
>   				rds_ib_mr_cqe_handler(ic, wc);
>   
>   		}
> +		ncompleted += nr;
>   	}
> +	if (cq->dim)
> +		rdma_dim(cq->dim, ncompleted);
>   }
>   
>   static void rds_ib_tasklet_fn_send(unsigned long data)
> @@ -304,6 +308,7 @@ static void poll_rcq(struct rds_ib_connection *ic, struct ib_cq *cq,
>   		     struct ib_wc *wcs,
>   		     struct rds_ib_ack_state *ack_state)
>   {
> +	int ncompleted = 0;
>   	int nr, i;
>   	struct ib_wc *wc;
>   
> @@ -316,7 +321,10 @@ static void poll_rcq(struct rds_ib_connection *ic, struct ib_cq *cq,
>   
>   			rds_ib_recv_cqe_handler(ic, wc, ack_state);
>   		}
> +		ncompleted += nr;
>   	}
> +	if (cq->dim)
> +		rdma_dim(cq->dim, ncompleted);
>   }
>   
>   static void rds_ib_tasklet_fn_recv(unsigned long data)
> @@ -542,6 +550,7 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
>   	ic->i_scq_vector = ibdev_get_unused_vector(rds_ibdev);
>   	cq_attr.cqe = ic->i_send_ring.w_nr + fr_queue_space + 1;
>   	cq_attr.comp_vector = ic->i_scq_vector;
> +	cq_attr.flags |= IB_CQ_MODERATE;
>   	ic->i_send_cq = ib_create_cq(dev, rds_ib_cq_comp_handler_send,
>   				     rds_ib_cq_event_handler, conn,
>   				     &cq_attr);
> @@ -556,6 +565,7 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
>   	ic->i_rcq_vector = ibdev_get_unused_vector(rds_ibdev);
>   	cq_attr.cqe = ic->i_recv_ring.w_nr;
>   	cq_attr.comp_vector = ic->i_rcq_vector;
> +	cq_attr.flags |= IB_CQ_MODERATE;
>   	ic->i_recv_cq = ib_create_cq(dev, rds_ib_cq_comp_handler_recv,
>   				     rds_ib_cq_event_handler, conn,
>   				     &cq_attr);

Re: [MAINLINE 2/2] rds: ib: Add Dynamic Interrupt Moderation to CQs

[Haakon Bugge]

> On 20 Sep 2024, at 09:47, Zhu Yanjun <yanjun.zhu@linux.dev> wrote:
> 
> 在 2024/9/18 16:35, Håkon Bugge 写道:
>> With the support from ib_core to use Dynamic Interrupt Moderation
>> (DIM) from legacy ULPs, which uses ib_create_cq(), we enable that
>> feature for the receive and send CQs in RDS.
> 
> Hi, Haakon
> 
> I am interested in this patch series. I just wonder if the performance of rds is increased after DIM is used in legacy ULPs?
> That is, is there any benefit to legacy ULPs after DIM is used?
> 
> Do you have any test results about this DIM?

Yes, please see the cover letter of this commit.


Thxs, Håkon

> 
> Thanks,
> Zhu Yanjun
> 
>> A set of rds-stress runs have been done. bcopy read + write for
>> payload 8448 and 16640 bytes and ack/req of 256 bytes. Number of QPs
>> varies from 8 to 128, number of threads (i.e. rds-stress processes)
>> from one to 16 and a depth of four. A limit has been applied such that
>> the number of processes times the number of QPs never exceeds 128. All
>> in all, 61 rds-stress runs.
>> For brevity, only the rows showing a +/- 3% deviation or larger from
>> base is listed. The geometric mean of the ratios (IOPS_test /
>> IOPS_base) is calculated for all 61 runs, and that gives the best
>> possible "average" impact of the commits.
>> In the following, "base" is v6.11-rc7. "test" is the same
>> kernel with the following two commits:
>>        * rds: ib: Add Dynamic Interrupt Moderation to CQs (this commit)
>>        * RDMA/core: Enable legacy ULPs to use RDMA DIM
>> This is executed between two X8-2 with CX-5 using fw 16.35.3502. These
>> BM systems were instantiated with one VF, which were used for the
>> test:
>>                                  base     test
>>    ACK    REQ  QPS  THR  DEP     IOPS     IOPS  Percent
>>    256   8448    8    1    4   634463   658162      3.7
>>    256   8448    8    2    4   862648   997358     15.6
>>    256   8448    8    4    4   950458  1113991     17.2
>>    256   8448    8    8    4   932120  1127024     20.9
>>    256   8448    8   16    4   944977  1133885     20.0
>>   8448    256    8    2    4   858663   975563     13.6
>>   8448    256    8    4    4   934884  1098854     17.5
>>   8448    256    8    8    4   928247  1116015     20.2
>>   8448    256    8   16    4   938864  1123455     19.7
>>    256   8448   64    1    4   965985   918445     -4.9
>>   8448    256   64    1    4   963280   918239     -4.7
>>    256  16640    8    2    4   544670   582330      6.9
>>    256  16640    8    4    4   554873   597553      7.7
>>    256  16640    8    8    4   551799   597479      8.3
>>    256  16640    8   16    4   553041   597898      8.1
>>  16640    256    8    2    4   544644   578331      6.2
>>  16640    256    8    4    4   553944   594627      7.3
>>  16640    256    8    8    4   551388   594737      7.9
>>  16640    256    8   16    4   552986   596581      7.9
>> Geometric mean of ratios: 1.03
>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>> ---
>>  net/rds/ib_cm.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
>> index 26b069e1999df..79603d86b6c02 100644
>> --- a/net/rds/ib_cm.c
>> +++ b/net/rds/ib_cm.c
>> @@ -259,6 +259,7 @@ static void rds_ib_cq_comp_handler_recv(struct ib_cq *cq, void *context)
>>  static void poll_scq(struct rds_ib_connection *ic, struct ib_cq *cq,
>>       struct ib_wc *wcs)
>>  {
>> + int ncompleted = 0;
>>   int nr, i;
>>   struct ib_wc *wc;
>>  @@ -276,7 +277,10 @@ static void poll_scq(struct rds_ib_connection *ic, struct ib_cq *cq,
>>   rds_ib_mr_cqe_handler(ic, wc);
>>     }
>> + ncompleted += nr;
>>   }
>> + if (cq->dim)
>> + rdma_dim(cq->dim, ncompleted);
>>  }
>>    static void rds_ib_tasklet_fn_send(unsigned long data)
>> @@ -304,6 +308,7 @@ static void poll_rcq(struct rds_ib_connection *ic, struct ib_cq *cq,
>>       struct ib_wc *wcs,
>>       struct rds_ib_ack_state *ack_state)
>>  {
>> + int ncompleted = 0;
>>   int nr, i;
>>   struct ib_wc *wc;
>>  @@ -316,7 +321,10 @@ static void poll_rcq(struct rds_ib_connection *ic, struct ib_cq *cq,
>>     rds_ib_recv_cqe_handler(ic, wc, ack_state);
>>   }
>> + ncompleted += nr;
>>   }
>> + if (cq->dim)
>> + rdma_dim(cq->dim, ncompleted);
>>  }
>>    static void rds_ib_tasklet_fn_recv(unsigned long data)
>> @@ -542,6 +550,7 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
>>   ic->i_scq_vector = ibdev_get_unused_vector(rds_ibdev);
>>   cq_attr.cqe = ic->i_send_ring.w_nr + fr_queue_space + 1;
>>   cq_attr.comp_vector = ic->i_scq_vector;
>> + cq_attr.flags |= IB_CQ_MODERATE;
>>   ic->i_send_cq = ib_create_cq(dev, rds_ib_cq_comp_handler_send,
>>       rds_ib_cq_event_handler, conn,
>>       &cq_attr);
>> @@ -556,6 +565,7 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
>>   ic->i_rcq_vector = ibdev_get_unused_vector(rds_ibdev);
>>   cq_attr.cqe = ic->i_recv_ring.w_nr;
>>   cq_attr.comp_vector = ic->i_rcq_vector;
>> + cq_attr.flags |= IB_CQ_MODERATE;
>>   ic->i_recv_cq = ib_create_cq(dev, rds_ib_cq_comp_handler_recv,
>>       rds_ib_cq_event_handler, conn,
>>       &cq_attr);
> 

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Haakon Bugge]

Hi Christoph,

> On 19 Sep 2024, at 16:17, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Wed, Sep 18, 2024 at 10:35:50AM +0200, Håkon Bugge wrote:
>> The Dynamic Interrupt Moderation mechanism can only be used by ULPs
>> using ib_alloc_cq() and family. We extend DIM to also cover legacy
>> ULPs using ib_create_cq(). The last commit takes advantage of this end
>> uses DIM in RDS.
> 
> I would much prefer if you could move RDS off that horrible API finally
> instead of investing more effort into it and making it more complicated.

ib_alloc_cq() and family does not support arming the CQ with the IB_CQ_SOLICITED flag, which RDS uses.


Thxs, Håkon

Re: [MAINLINE 2/2] rds: ib: Add Dynamic Interrupt Moderation to CQs

[Zhu Yanjun]

在 2024/9/20 17:42, Haakon Bugge 写道:
> 
> 
>> On 20 Sep 2024, at 09:47, Zhu Yanjun <yanjun.zhu@linux.dev> wrote:
>>
>> 在 2024/9/18 16:35, Håkon Bugge 写道:
>>> With the support from ib_core to use Dynamic Interrupt Moderation
>>> (DIM) from legacy ULPs, which uses ib_create_cq(), we enable that
>>> feature for the receive and send CQs in RDS.
>>
>> Hi, Haakon
>>
>> I am interested in this patch series. I just wonder if the performance of rds is increased after DIM is used in legacy ULPs?
>> That is, is there any benefit to legacy ULPs after DIM is used?
>>
>> Do you have any test results about this DIM?
> 
> Yes, please see the cover letter of this commit.

Which Oracle Linux distro includes this feature?
And what is the kernel version?

Zhu Yanjun

> 
> 
> Thxs, Håkon
> 
>>
>> Thanks,
>> Zhu Yanjun
>>
>>> A set of rds-stress runs have been done. bcopy read + write for
>>> payload 8448 and 16640 bytes and ack/req of 256 bytes. Number of QPs
>>> varies from 8 to 128, number of threads (i.e. rds-stress processes)
>>> from one to 16 and a depth of four. A limit has been applied such that
>>> the number of processes times the number of QPs never exceeds 128. All
>>> in all, 61 rds-stress runs.
>>> For brevity, only the rows showing a +/- 3% deviation or larger from
>>> base is listed. The geometric mean of the ratios (IOPS_test /
>>> IOPS_base) is calculated for all 61 runs, and that gives the best
>>> possible "average" impact of the commits.
>>> In the following, "base" is v6.11-rc7. "test" is the same
>>> kernel with the following two commits:
>>>         * rds: ib: Add Dynamic Interrupt Moderation to CQs (this commit)
>>>         * RDMA/core: Enable legacy ULPs to use RDMA DIM
>>> This is executed between two X8-2 with CX-5 using fw 16.35.3502. These
>>> BM systems were instantiated with one VF, which were used for the
>>> test:
>>>                                   base     test
>>>     ACK    REQ  QPS  THR  DEP     IOPS     IOPS  Percent
>>>     256   8448    8    1    4   634463   658162      3.7
>>>     256   8448    8    2    4   862648   997358     15.6
>>>     256   8448    8    4    4   950458  1113991     17.2
>>>     256   8448    8    8    4   932120  1127024     20.9
>>>     256   8448    8   16    4   944977  1133885     20.0
>>>    8448    256    8    2    4   858663   975563     13.6
>>>    8448    256    8    4    4   934884  1098854     17.5
>>>    8448    256    8    8    4   928247  1116015     20.2
>>>    8448    256    8   16    4   938864  1123455     19.7
>>>     256   8448   64    1    4   965985   918445     -4.9
>>>    8448    256   64    1    4   963280   918239     -4.7
>>>     256  16640    8    2    4   544670   582330      6.9
>>>     256  16640    8    4    4   554873   597553      7.7
>>>     256  16640    8    8    4   551799   597479      8.3
>>>     256  16640    8   16    4   553041   597898      8.1
>>>   16640    256    8    2    4   544644   578331      6.2
>>>   16640    256    8    4    4   553944   594627      7.3
>>>   16640    256    8    8    4   551388   594737      7.9
>>>   16640    256    8   16    4   552986   596581      7.9
>>> Geometric mean of ratios: 1.03
>>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>>> ---
>>>   net/rds/ib_cm.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
>>> index 26b069e1999df..79603d86b6c02 100644
>>> --- a/net/rds/ib_cm.c
>>> +++ b/net/rds/ib_cm.c
>>> @@ -259,6 +259,7 @@ static void rds_ib_cq_comp_handler_recv(struct ib_cq *cq, void *context)
>>>   static void poll_scq(struct rds_ib_connection *ic, struct ib_cq *cq,
>>>        struct ib_wc *wcs)
>>>   {
>>> + int ncompleted = 0;
>>>    int nr, i;
>>>    struct ib_wc *wc;
>>>   @@ -276,7 +277,10 @@ static void poll_scq(struct rds_ib_connection *ic, struct ib_cq *cq,
>>>    rds_ib_mr_cqe_handler(ic, wc);
>>>      }
>>> + ncompleted += nr;
>>>    }
>>> + if (cq->dim)
>>> + rdma_dim(cq->dim, ncompleted);
>>>   }
>>>     static void rds_ib_tasklet_fn_send(unsigned long data)
>>> @@ -304,6 +308,7 @@ static void poll_rcq(struct rds_ib_connection *ic, struct ib_cq *cq,
>>>        struct ib_wc *wcs,
>>>        struct rds_ib_ack_state *ack_state)
>>>   {
>>> + int ncompleted = 0;
>>>    int nr, i;
>>>    struct ib_wc *wc;
>>>   @@ -316,7 +321,10 @@ static void poll_rcq(struct rds_ib_connection *ic, struct ib_cq *cq,
>>>      rds_ib_recv_cqe_handler(ic, wc, ack_state);
>>>    }
>>> + ncompleted += nr;
>>>    }
>>> + if (cq->dim)
>>> + rdma_dim(cq->dim, ncompleted);
>>>   }
>>>     static void rds_ib_tasklet_fn_recv(unsigned long data)
>>> @@ -542,6 +550,7 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
>>>    ic->i_scq_vector = ibdev_get_unused_vector(rds_ibdev);
>>>    cq_attr.cqe = ic->i_send_ring.w_nr + fr_queue_space + 1;
>>>    cq_attr.comp_vector = ic->i_scq_vector;
>>> + cq_attr.flags |= IB_CQ_MODERATE;
>>>    ic->i_send_cq = ib_create_cq(dev, rds_ib_cq_comp_handler_send,
>>>        rds_ib_cq_event_handler, conn,
>>>        &cq_attr);
>>> @@ -556,6 +565,7 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
>>>    ic->i_rcq_vector = ibdev_get_unused_vector(rds_ibdev);
>>>    cq_attr.cqe = ic->i_recv_ring.w_nr;
>>>    cq_attr.comp_vector = ic->i_rcq_vector;
>>> + cq_attr.flags |= IB_CQ_MODERATE;
>>>    ic->i_recv_cq = ib_create_cq(dev, rds_ib_cq_comp_handler_recv,
>>>        rds_ib_cq_event_handler, conn,
>>>        &cq_attr);
>>
> 

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Christoph Hellwig]

On Fri, Sep 20, 2024 at 09:46:06AM +0000, Haakon Bugge wrote:
> > I would much prefer if you could move RDS off that horrible API finally
> > instead of investing more effort into it and making it more complicated.
> 
> ib_alloc_cq() and family does not support arming the CQ with the IB_CQ_SOLICITED flag, which RDS uses.

Then work on supporting it.  RDS and SMC are the only users, so one
of the maintainers needs to drive it.

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Zhu Yanjun]

在 2024/9/20 21:51, Christoph Hellwig 写道:
> On Fri, Sep 20, 2024 at 09:46:06AM +0000, Haakon Bugge wrote:
>>> I would much prefer if you could move RDS off that horrible API finally
>>> instead of investing more effort into it and making it more complicated.
>>
>> ib_alloc_cq() and family does not support arming the CQ with the IB_CQ_SOLICITED flag, which RDS uses.
> 
> Then work on supporting it.  RDS and SMC are the only users, so one

Some other open source projects are also the users.

Zhu Yanjun

> of the maintainers needs to drive it.
> 

Re: [MAINLINE 2/2] rds: ib: Add Dynamic Interrupt Moderation to CQs

[Zhu Yanjun]

在 2024/9/18 16:35, Håkon Bugge 写道:
> With the support from ib_core to use Dynamic Interrupt Moderation
> (DIM) from legacy ULPs, which uses ib_create_cq(), we enable that
> feature for the receive and send CQs in RDS.
> 
> A set of rds-stress runs have been done. bcopy read + write for
> payload 8448 and 16640 bytes and ack/req of 256 bytes. Number of QPs
> varies from 8 to 128, number of threads (i.e. rds-stress processes)
> from one to 16 and a depth of four. A limit has been applied such that
> the number of processes times the number of QPs never exceeds 128. All
> in all, 61 rds-stress runs.
> 
> For brevity, only the rows showing a +/- 3% deviation or larger from
> base is listed. The geometric mean of the ratios (IOPS_test /
> IOPS_base) is calculated for all 61 runs, and that gives the best
> possible "average" impact of the commits.
> 
> In the following, "base" is v6.11-rc7. "test" is the same
> kernel with the following two commits:
> 
>         * rds: ib: Add Dynamic Interrupt Moderation to CQs (this commit)
>         * RDMA/core: Enable legacy ULPs to use RDMA DIM
> 
> This is executed between two X8-2 with CX-5 using fw 16.35.3502. These
> BM systems were instantiated with one VF, which were used for the
> test:
> 
>                                   base     test
>     ACK    REQ  QPS  THR  DEP     IOPS     IOPS  Percent
>     256   8448    8    1    4   634463   658162      3.7
>     256   8448    8    2    4   862648   997358     15.6
>     256   8448    8    4    4   950458  1113991     17.2
>     256   8448    8    8    4   932120  1127024     20.9
>     256   8448    8   16    4   944977  1133885     20.0
>    8448    256    8    2    4   858663   975563     13.6
>    8448    256    8    4    4   934884  1098854     17.5
>    8448    256    8    8    4   928247  1116015     20.2
>    8448    256    8   16    4   938864  1123455     19.7
>     256   8448   64    1    4   965985   918445     -4.9
>    8448    256   64    1    4   963280   918239     -4.7
>     256  16640    8    2    4   544670   582330      6.9
>     256  16640    8    4    4   554873   597553      7.7
>     256  16640    8    8    4   551799   597479      8.3
>     256  16640    8   16    4   553041   597898      8.1
>   16640    256    8    2    4   544644   578331      6.2
>   16640    256    8    4    4   553944   594627      7.3
>   16640    256    8    8    4   551388   594737      7.9
>   16640    256    8   16    4   552986   596581      7.9
> Geometric mean of ratios: 1.03
> 
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> ---
>   net/rds/ib_cm.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 26b069e1999df..79603d86b6c02 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -259,6 +259,7 @@ static void rds_ib_cq_comp_handler_recv(struct ib_cq *cq, void *context)
>   static void poll_scq(struct rds_ib_connection *ic, struct ib_cq *cq,
>   		     struct ib_wc *wcs)
>   {
> +	int ncompleted = 0;
>   	int nr, i;
>   	struct ib_wc *wc;
>   
> @@ -276,7 +277,10 @@ static void poll_scq(struct rds_ib_connection *ic, struct ib_cq *cq,
>   				rds_ib_mr_cqe_handler(ic, wc);
>   
>   		}
> +		ncompleted += nr;
>   	}
> +	if (cq->dim)
> +		rdma_dim(cq->dim, ncompleted);
>   }
>   
>   static void rds_ib_tasklet_fn_send(unsigned long data)
> @@ -304,6 +308,7 @@ static void poll_rcq(struct rds_ib_connection *ic, struct ib_cq *cq,
>   		     struct ib_wc *wcs,
>   		     struct rds_ib_ack_state *ack_state)
>   {
> +	int ncompleted = 0;
>   	int nr, i;
>   	struct ib_wc *wc;
>   
> @@ -316,7 +321,10 @@ static void poll_rcq(struct rds_ib_connection *ic, struct ib_cq *cq,
>   
>   			rds_ib_recv_cqe_handler(ic, wc, ack_state);
>   		}
> +		ncompleted += nr;
>   	}
> +	if (cq->dim)
> +		rdma_dim(cq->dim, ncompleted);
>   }
>   
>   static void rds_ib_tasklet_fn_recv(unsigned long data)
> @@ -542,6 +550,7 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
>   	ic->i_scq_vector = ibdev_get_unused_vector(rds_ibdev);
>   	cq_attr.cqe = ic->i_send_ring.w_nr + fr_queue_space + 1;
>   	cq_attr.comp_vector = ic->i_scq_vector;
> +	cq_attr.flags |= IB_CQ_MODERATE;

cq_attr.flags is added IB_CQ_MODERATE here.

>   	ic->i_send_cq = ib_create_cq(dev, rds_ib_cq_comp_handler_send,
>   				     rds_ib_cq_event_handler, conn,
>   				     &cq_attr);
> @@ -556,6 +565,7 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
>   	ic->i_rcq_vector = ibdev_get_unused_vector(rds_ibdev);
>   	cq_attr.cqe = ic->i_recv_ring.w_nr;
>   	cq_attr.comp_vector = ic->i_rcq_vector;
> +	cq_attr.flags |= IB_CQ_MODERATE;

Why is cq_attr.flags add IB_CQ_MODERATE again?
Is this cq_attr.flags changed before this line?

Zhu Yanjun

>   	ic->i_recv_cq = ib_create_cq(dev, rds_ib_cq_comp_handler_recv,
>   				     rds_ib_cq_event_handler, conn,
>   				     &cq_attr);

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Leon Romanovsky]

On Sat, Sep 21, 2024 at 10:28:48AM +0800, Zhu Yanjun wrote:
> 在 2024/9/20 21:51, Christoph Hellwig 写道:
> > On Fri, Sep 20, 2024 at 09:46:06AM +0000, Haakon Bugge wrote:
> > > > I would much prefer if you could move RDS off that horrible API finally
> > > > instead of investing more effort into it and making it more complicated.
> > > 
> > > ib_alloc_cq() and family does not support arming the CQ with the IB_CQ_SOLICITED flag, which RDS uses.
> > 
> > Then work on supporting it.  RDS and SMC are the only users, so one
> 
> Some other open source projects are also the users.

What are these projects? I see only RDS and SMC.

Thanks

> 
> Zhu Yanjun
> 
> > of the maintainers needs to drive it.
> > 
> 
> 

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Christoph Hellwig]

On Sat, Sep 21, 2024 at 10:28:48AM +0800, Zhu Yanjun wrote:
> 在 2024/9/20 21:51, Christoph Hellwig 写道:
> > On Fri, Sep 20, 2024 at 09:46:06AM +0000, Haakon Bugge wrote:
> > > > I would much prefer if you could move RDS off that horrible API finally
> > > > instead of investing more effort into it and making it more complicated.
> > > 
> > > ib_alloc_cq() and family does not support arming the CQ with the IB_CQ_SOLICITED flag, which RDS uses.
> > 
> > Then work on supporting it.  RDS and SMC are the only users, so one
> 
> Some other open source projects are also the users.

I'm not sure what you mean with "open source projects", but the only
thing that matters is users in the kernel tree.

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Zhu Yanjun]

在 2024/9/23 20:03, Christoph Hellwig 写道:
> On Sat, Sep 21, 2024 at 10:28:48AM +0800, Zhu Yanjun wrote:
>> 在 2024/9/20 21:51, Christoph Hellwig 写道:
>>> On Fri, Sep 20, 2024 at 09:46:06AM +0000, Haakon Bugge wrote:
>>>>> I would much prefer if you could move RDS off that horrible API finally
>>>>> instead of investing more effort into it and making it more complicated.
>>>>
>>>> ib_alloc_cq() and family does not support arming the CQ with the IB_CQ_SOLICITED flag, which RDS uses.
>>>
>>> Then work on supporting it.  RDS and SMC are the only users, so one
>>
>> Some other open source projects are also the users.
> 
> I'm not sure what you mean with "open source projects", but the only
> thing that matters is users in the kernel tree.

The users that I mentioned is not in the kernel tree.

Best Regards,
Zhu Yanjun
> 

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Christoph Hellwig]

On Tue, Sep 24, 2024 at 09:58:24AM +0800, Zhu Yanjun wrote:
> The users that I mentioned is not in the kernel tree.

And why do you think that would matter the slightest?

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Christoph Hellwig]

On Fri, Sep 20, 2024 at 06:51:18AM -0700, Christoph Hellwig wrote:
> On Fri, Sep 20, 2024 at 09:46:06AM +0000, Haakon Bugge wrote:
> > > I would much prefer if you could move RDS off that horrible API finally
> > > instead of investing more effort into it and making it more complicated.
> > 
> > ib_alloc_cq() and family does not support arming the CQ with the IB_CQ_SOLICITED flag, which RDS uses.
> 
> Then work on supporting it.  RDS and SMC are the only users, so one
> of the maintainers needs to drive it.

I took a quick look at what it would take, and adding IB_CQ_SOLICITED
support to the cq API looks pretty trivial, you'll just need to pass
it to ib_cq_pool_get by adding a new argument and the pass it
down to __ib_alloc_cq.  So yes, please get started at moving RDS out
of the stone age.

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Haakon Bugge]

> On 24 Sep 2024, at 09:01, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Fri, Sep 20, 2024 at 06:51:18AM -0700, Christoph Hellwig wrote:
>> On Fri, Sep 20, 2024 at 09:46:06AM +0000, Haakon Bugge wrote:
>>>> I would much prefer if you could move RDS off that horrible API finally
>>>> instead of investing more effort into it and making it more complicated.
>>> 
>>> ib_alloc_cq() and family does not support arming the CQ with the IB_CQ_SOLICITED flag, which RDS uses.
>> 
>> Then work on supporting it.  RDS and SMC are the only users, so one
>> of the maintainers needs to drive it.
> 
> I took a quick look at what it would take, and adding IB_CQ_SOLICITED
> support to the cq API looks pretty trivial, you'll just need to pass
> it to ib_cq_pool_get by adding a new argument and the pass it
> down to __ib_alloc_cq.  So yes, please get started at moving RDS out
> of the stone age.

Agree. I'll work on that. I am also inclined to improve it by having designated CPUs where the worker threads polling the CQs execute, as that often improves cache hit rates. But that will come as another commit.


Thxs, Håkon

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Zhu Yanjun]

在 2024/9/24 14:54, Christoph Hellwig 写道:
> On Tue, Sep 24, 2024 at 09:58:24AM +0800, Zhu Yanjun wrote:
>> The users that I mentioned is not in the kernel tree.
> 
> And why do you think that would matter the slightest?

I noticed that the same cq functions are used. And I also made tests 
with this patch series. Without this patch series, dim mechanism will 
not be invoked.

Zhu Yanjun

> 

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Haakon Bugge]

> On 24 Sep 2024, at 15:59, Zhu Yanjun <yanjun.zhu@linux.dev> wrote:
> 
> 在 2024/9/24 14:54, Christoph Hellwig 写道:
>> On Tue, Sep 24, 2024 at 09:58:24AM +0800, Zhu Yanjun wrote:
>>> The users that I mentioned is not in the kernel tree.
>> And why do you think that would matter the slightest?
> 
> I noticed that the same cq functions are used. And I also made tests with this patch series. Without this patch series, dim mechanism will not be invoked.

Christoph alluded to say: Do not modify the old cq_create_cq() code in order to support DIM, it is better to change the ULP to use ib_alloc_cq(), and get DIM enabled that way.


Thxs, Håkon

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Allison Henderson]

On Tue, 2024-09-24 at 08:59 +0000, Haakon Bugge wrote:
> 
> 
> > On 24 Sep 2024, at 09:01, Christoph Hellwig <hch@infradead.org>
> > wrote:
> > 
> > On Fri, Sep 20, 2024 at 06:51:18AM -0700, Christoph Hellwig wrote:
> > > On Fri, Sep 20, 2024 at 09:46:06AM +0000, Haakon Bugge wrote:
> > > > > I would much prefer if you could move RDS off that horrible
> > > > > API finally
> > > > > instead of investing more effort into it and making it more
> > > > > complicated.
> > > > 
> > > > ib_alloc_cq() and family does not support arming the CQ with
> > > > the IB_CQ_SOLICITED flag, which RDS uses.
> > > 
> > > Then work on supporting it.  RDS and SMC are the only users, so
> > > one
> > > of the maintainers needs to drive it.
> > 
> > I took a quick look at what it would take, and adding
> > IB_CQ_SOLICITED
> > support to the cq API looks pretty trivial, you'll just need to
> > pass
> > it to ib_cq_pool_get by adding a new argument and the pass it
> > down to __ib_alloc_cq.  So yes, please get started at moving RDS
> > out
> > of the stone age.
> 
> Agree. I'll work on that. I am also inclined to improve it by having
> designated CPUs where the worker threads polling the CQs execute, as
> that often improves cache hit rates. But that will come as another
> commit.
> 
> 
> Thxs, Håkon
> 

Ok, sounds like a good plan then.  Thanks for working on this Haakon!

Allison

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Zhu Yanjun]

在 2024/9/24 23:16, Haakon Bugge 写道:
> 
> 
>> On 24 Sep 2024, at 15:59, Zhu Yanjun <yanjun.zhu@linux.dev> wrote:
>>
>> 在 2024/9/24 14:54, Christoph Hellwig 写道:
>>> On Tue, Sep 24, 2024 at 09:58:24AM +0800, Zhu Yanjun wrote:
>>>> The users that I mentioned is not in the kernel tree.
>>> And why do you think that would matter the slightest?
>>
>> I noticed that the same cq functions are used. And I also made tests with this patch series. Without this patch series, dim mechanism will not be invoked.
> 
> Christoph alluded to say: Do not modify the old cq_create_cq() code in order to support DIM, it is better to change the ULP to use ib_alloc_cq(), and get DIM enabled that way.

Hi, Haakon

To be honest, I like your original commit that enable DIM for legacy 
ULPs because this can fix this problem once for all and improve the old 
ib_create_cq function.

The idea from Christoph will cause a lot of changes in ULPs. I am not 
very sure if these changes cause risks or not.

Thus, I prefer to your original commit. But I will follow the advice 
from Leon and Jason.

Zhu Yanjun

> 
> 
> Thxs, Håkon
> 

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Leon Romanovsky]

On Wed, Sep 25, 2024 at 10:04:21AM +0800, Zhu Yanjun wrote:
> 在 2024/9/24 23:16, Haakon Bugge 写道:
> > 
> > 
> > > On 24 Sep 2024, at 15:59, Zhu Yanjun <yanjun.zhu@linux.dev> wrote:
> > > 
> > > 在 2024/9/24 14:54, Christoph Hellwig 写道:
> > > > On Tue, Sep 24, 2024 at 09:58:24AM +0800, Zhu Yanjun wrote:
> > > > > The users that I mentioned is not in the kernel tree.
> > > > And why do you think that would matter the slightest?
> > > 
> > > I noticed that the same cq functions are used. And I also made tests with this patch series. Without this patch series, dim mechanism will not be invoked.
> > 
> > Christoph alluded to say: Do not modify the old cq_create_cq() code in order to support DIM, it is better to change the ULP to use ib_alloc_cq(), and get DIM enabled that way.
> 
> Hi, Haakon
> 
> To be honest, I like your original commit that enable DIM for legacy ULPs
> because this can fix this problem once for all and improve the old
> ib_create_cq function.
> 
> The idea from Christoph will cause a lot of changes in ULPs. I am not very
> sure if these changes cause risks or not.
> 
> Thus, I prefer to your original commit. But I will follow the advice from
> Leon and Jason.

Christoph was very clear and he summarized our position very well. We
said similar thing to SMC folks in 2022 [1] and RDS is no different here.

So no, "old ib_create_cq" shouldn't be used by ULPs.

Thanks

[1] https://lore.kernel.org/netdev/YePesYRnrKCh1vFy@unreal/

> 
> Zhu Yanjun
> 
> > 
> > 
> > Thxs, Håkon
> > 
> 
> 

Re: [MAINLINE 0/2] Enable DIM for legacy ULPs and use it in RDS

[Zhu Yanjun]

在 2024/9/25 17:26, Leon Romanovsky 写道:
> On Wed, Sep 25, 2024 at 10:04:21AM +0800, Zhu Yanjun wrote:
>> 在 2024/9/24 23:16, Haakon Bugge 写道:
>>>
>>>
>>>> On 24 Sep 2024, at 15:59, Zhu Yanjun <yanjun.zhu@linux.dev> wrote:
>>>>
>>>> 在 2024/9/24 14:54, Christoph Hellwig 写道:
>>>>> On Tue, Sep 24, 2024 at 09:58:24AM +0800, Zhu Yanjun wrote:
>>>>>> The users that I mentioned is not in the kernel tree.
>>>>> And why do you think that would matter the slightest?
>>>>
>>>> I noticed that the same cq functions are used. And I also made tests with this patch series. Without this patch series, dim mechanism will not be invoked.
>>>
>>> Christoph alluded to say: Do not modify the old cq_create_cq() code in order to support DIM, it is better to change the ULP to use ib_alloc_cq(), and get DIM enabled that way.
>>
>> Hi, Haakon
>>
>> To be honest, I like your original commit that enable DIM for legacy ULPs
>> because this can fix this problem once for all and improve the old
>> ib_create_cq function.
>>
>> The idea from Christoph will cause a lot of changes in ULPs. I am not very
>> sure if these changes cause risks or not.
>>
>> Thus, I prefer to your original commit. But I will follow the advice from
>> Leon and Jason.
> 
> Christoph was very clear and he summarized our position very well. We
> said similar thing to SMC folks in 2022 [1] and RDS is no different here.

Thanks, Leon. I will read this link carefully.

> 
> So no, "old ib_create_cq" shouldn't be used by ULPs.

Got it. I have replaced ib_create_cq with ib cq pool APIs. Perhaps 
drivers/infiniband/ulp/srpt/ib_srpt.c is a good example to use ib cq 
pool APIs.

Best Regards,
Zhu Yanjun

> 
> Thanks
> 
> [1] https://lore.kernel.org/netdev/YePesYRnrKCh1vFy@unreal/
> 
>>
>> Zhu Yanjun
>>
>>>
>>>
>>> Thxs, Håkon
>>>
>>
>>