[PATCH for-next 0/5] Misc update for RTRS

[PATCH for-next 0/5] Misc update for RTRS

[Md Haris Iqbal]

Hi Jason, hi Doug,

Please consider to include following changes to the next merge window.

The patchset is organized as:
- patch1, patch2, patch3 fixes warnings generated from checkpatch
- patch4 updates a comment.
- patch5 In case of error, performs failover earlier & avoids deadlock

Gioh Kim (3):
  RDMA/rtrs: fix CHECK:BRACES type warning
  RDMA/rtrs-clt: fix CHECK type warnings
  RDMA/rtrs-clt: fix CHECK type warnings

Jack Wang (2):
  RDMA/rtrs-clt: Update one outdated comment in path_it_deinit
  RDMA/rtrs-clt: Do stop and failover outside reconnect work.

 drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c |  3 +-
 drivers/infiniband/ulp/rtrs/rtrs-clt.c       | 91 ++++++++++----------
 drivers/infiniband/ulp/rtrs/rtrs-clt.h       |  1 +
 drivers/infiniband/ulp/rtrs/rtrs.c           |  1 -
 4 files changed, 47 insertions(+), 49 deletions(-)

-- 
2.25.1

[PATCH for-next 1/5] RDMA/rtrs: fix CHECK:BRACES type warning

[Md Haris Iqbal]

From: Gioh Kim <gi-oh.kim@ionos.com>

fix "CHECK:BRACES: Blank lines aren't necessary after an open
brace '{'" warning from checkpatch.pl

Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs.c b/drivers/infiniband/ulp/rtrs/rtrs.c
index 4da889103a5f..60fa0b0160f4 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs.c
@@ -479,7 +479,6 @@ static int rtrs_str_to_sockaddr(const char *addr, size_t len,
  */
 int sockaddr_to_str(const struct sockaddr *addr, char *buf, size_t len)
 {
-
 	switch (addr->sa_family) {
 	case AF_IB:
 		return scnprintf(buf, len, "gid:%pI6",
-- 
2.25.1

[PATCH for-next 2/5] RDMA/rtrs-clt: fix CHECK type warnings

[Md Haris Iqbal]

From: Gioh Kim <gi-oh.kim@ionos.com>

First this patch removes list_next_or_null_rr_rcu macro to
fix below warnings. That macro is used only twice.
CHECK:MACRO_ARG_REUSE: Macro argument reuse 'head' - possible side-effects?
CHECK:MACRO_ARG_REUSE: Macro argument reuse 'ptr' - possible side-effects?
CHECK:MACRO_ARG_REUSE: Macro argument reuse 'memb' - possible side-effects?

This patch also fixes below warning:
CHECK:SPACING: No space is necessary after a cast

Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 49 ++++++++++++--------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 7c3f98e57889..62ba0f17ac9d 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -748,26 +748,6 @@ struct path_it {
 	struct rtrs_clt_path *(*next_path)(struct path_it *it);
 };
 
-/**
- * list_next_or_null_rr_rcu - get next list element in round-robin fashion.
- * @head:	the head for the list.
- * @ptr:        the list head to take the next element from.
- * @type:       the type of the struct this is embedded in.
- * @memb:       the name of the list_head within the struct.
- *
- * Next element returned in round-robin fashion, i.e. head will be skipped,
- * but if list is observed as empty, NULL will be returned.
- *
- * This primitive may safely run concurrently with the _rcu list-mutation
- * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
- */
-#define list_next_or_null_rr_rcu(head, ptr, type, memb) \
-({ \
-	list_next_or_null_rcu(head, ptr, type, memb) ?: \
-		list_next_or_null_rcu(head, READ_ONCE((ptr)->next), \
-				      type, memb); \
-})
-
 /**
  * get_next_path_rr() - Returns path in round-robin fashion.
  * @it:	the path pointer
@@ -797,10 +777,20 @@ static struct rtrs_clt_path *get_next_path_rr(struct path_it *it)
 		path = list_first_or_null_rcu(&clt->paths_list,
 					      typeof(*path), s.entry);
 	else
-		path = list_next_or_null_rr_rcu(&clt->paths_list,
-						&path->s.entry,
-						typeof(*path),
-						s.entry);
+		/*
+		 * Next element returned in round-robin fashion, i.e. head will be skipped,
+		 * but if list is observed as empty, NULL will be returned.
+		 *
+		 * This primitive may safely run concurrently with the _rcu list-mutation
+		 * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
+		 */
+		path = list_next_or_null_rcu(&clt->paths_list,
+					     &path->s.entry,
+					     typeof(*path),
+					     s.entry) ?:
+			list_next_or_null_rcu(&clt->paths_list,
+					      READ_ONCE((&path->s.entry)->next),
+					      typeof(*path), s.entry);
 	rcu_assign_pointer(*ppcpu_path, path);
 
 	return path;
@@ -1863,7 +1853,7 @@ static int rtrs_rdma_conn_established(struct rtrs_clt_con *con,
 		}
 		clt_path->queue_depth = queue_depth;
 		clt_path->s.signal_interval = min_not_zero(queue_depth,
-						(unsigned short) SERVICE_CON_QUEUE_DEPTH);
+						(unsigned short)SERVICE_CON_QUEUE_DEPTH);
 		clt_path->max_hdr_size = le32_to_cpu(msg->max_hdr_size);
 		clt_path->max_io_size = le32_to_cpu(msg->max_io_size);
 		clt_path->flags = le32_to_cpu(msg->flags);
@@ -2268,8 +2258,13 @@ static void rtrs_clt_remove_path_from_arr(struct rtrs_clt_path *clt_path)
 	 * removed.  If @sess is the last element, then @next is NULL.
 	 */
 	rcu_read_lock();
-	next = list_next_or_null_rr_rcu(&clt->paths_list, &clt_path->s.entry,
-					typeof(*next), s.entry);
+	next = list_next_or_null_rcu(&clt->paths_list,
+				     &clt_path->s.entry,
+				     typeof(*next),
+				     s.entry) ?:
+		list_next_or_null_rcu(&clt->paths_list,
+				      READ_ONCE((&clt_path->s.entry)->next),
+				      typeof(*next), s.entry);
 	rcu_read_unlock();
 
 	/*
-- 
2.25.1

Re: [PATCH for-next 2/5] RDMA/rtrs-clt: fix CHECK type warnings

[Jason Gunthorpe]

On Fri, Jan 14, 2022 at 04:47:50PM +0100, Md Haris Iqbal wrote:

> -/**
> - * list_next_or_null_rr_rcu - get next list element in round-robin fashion.
> - * @head:	the head for the list.
> - * @ptr:        the list head to take the next element from.
> - * @type:       the type of the struct this is embedded in.
> - * @memb:       the name of the list_head within the struct.
> - *
> - * Next element returned in round-robin fashion, i.e. head will be skipped,
> - * but if list is observed as empty, NULL will be returned.
> - *
> - * This primitive may safely run concurrently with the _rcu list-mutation
> - * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
> - */
> -#define list_next_or_null_rr_rcu(head, ptr, type, memb) \
> -({ \
> -	list_next_or_null_rcu(head, ptr, type, memb) ?: \
> -		list_next_or_null_rcu(head, READ_ONCE((ptr)->next), \
> -				      type, memb); \
> -})

Why not put this in a static inline instead of open coding it? Type is
always the same for both usages, right?

Jason

Re: [PATCH for-next 2/5] RDMA/rtrs-clt: fix CHECK type warnings

[Haris Iqbal]

On Fri, Jan 28, 2022 at 3:55 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Fri, Jan 14, 2022 at 04:47:50PM +0100, Md Haris Iqbal wrote:
>
> > -/**
> > - * list_next_or_null_rr_rcu - get next list element in round-robin fashion.
> > - * @head:    the head for the list.
> > - * @ptr:        the list head to take the next element from.
> > - * @type:       the type of the struct this is embedded in.
> > - * @memb:       the name of the list_head within the struct.
> > - *
> > - * Next element returned in round-robin fashion, i.e. head will be skipped,
> > - * but if list is observed as empty, NULL will be returned.
> > - *
> > - * This primitive may safely run concurrently with the _rcu list-mutation
> > - * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
> > - */
> > -#define list_next_or_null_rr_rcu(head, ptr, type, memb) \
> > -({ \
> > -     list_next_or_null_rcu(head, ptr, type, memb) ?: \
> > -             list_next_or_null_rcu(head, READ_ONCE((ptr)->next), \
> > -                                   type, memb); \
> > -})
>
> Why not put this in a static inline instead of open coding it? Type is
> always the same for both usages, right?

Yes. Makes sense. I will send this change along with the next patchset.

Thanks for the review and comment.

>
> Jason

[PATCH for-next 3/5] RDMA/rtrs-clt: fix CHECK type warnings

[Md Haris Iqbal]

From: Gioh Kim <gi-oh.kim@ionos.com>

fix below warning
CHECK:OPEN_ENDED_LINE: Lines should not end with a '('

Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c b/drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c
index b4fa473b7888..d3c436ead694 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c
@@ -156,8 +156,7 @@ static DEVICE_ATTR_RW(mpath_policy);
 static ssize_t add_path_show(struct device *dev,
 			     struct device_attribute *attr, char *page)
 {
-	return sysfs_emit(
-		page,
+	return sysfs_emit(page,
 		"Usage: echo [<source addr>@]<destination addr> > %s\n\n*addr ::= [ ip:<ipv4|ipv6> | gid:<gid> ]\n",
 		attr->attr.name);
 }
-- 
2.25.1

[PATCH for-next 4/5] RDMA/rtrs-clt: Update one outdated comment in path_it_deinit

[Md Haris Iqbal]

From: Jack Wang <jinpu.wang@ionos.com>

The skip_list is used for both MIN_INFLIGHT and MIN_LATENCY.

Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 62ba0f17ac9d..05de9ec7c99a 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -907,7 +907,7 @@ static inline void path_it_deinit(struct path_it *it)
 {
 	struct list_head *skip, *tmp;
 	/*
-	 * The skip_list is used only for the MIN_INFLIGHT policy.
+	 * The skip_list is used only for the MIN_INFLIGHT and MIN_LATENCY policies.
 	 * We need to remove paths from it, so that next IO can insert
 	 * paths (->mp_skip_entry) into a skip_list again.
 	 */
-- 
2.25.1

[PATCH for-next 5/5] RDMA/rtrs-clt: Do stop and failover outside reconnect work.

[Md Haris Iqbal]

From: Jack Wang <jinpu.wang@ionos.com>

We can't do instant reconnect, not to DDoS server, but
we should stop and failover earlier, so there is less
service interruption.

To avoid deadlock, as error_recovery is called from different
callback like rdma event or hb error handler, add a new err
recovery_work.

Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
Reviewed-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 40 ++++++++++++++------------
 drivers/infiniband/ulp/rtrs/rtrs-clt.h |  1 +
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 05de9ec7c99a..b159471a8959 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -297,6 +297,7 @@ static bool rtrs_clt_change_state_from_to(struct rtrs_clt_path *clt_path,
 	return changed;
 }
 
+static void rtrs_clt_stop_and_destroy_conns(struct rtrs_clt_path *clt_path);
 static void rtrs_rdma_error_recovery(struct rtrs_clt_con *con)
 {
 	struct rtrs_clt_path *clt_path = to_clt_path(con->c.path);
@@ -304,16 +305,7 @@ static void rtrs_rdma_error_recovery(struct rtrs_clt_con *con)
 	if (rtrs_clt_change_state_from_to(clt_path,
 					   RTRS_CLT_CONNECTED,
 					   RTRS_CLT_RECONNECTING)) {
-		struct rtrs_clt_sess *clt = clt_path->clt;
-		unsigned int delay_ms;
-
-		/*
-		 * Normal scenario, reconnect if we were successfully connected
-		 */
-		delay_ms = clt->reconnect_delay_sec * 1000;
-		queue_delayed_work(rtrs_wq, &clt_path->reconnect_dwork,
-				   msecs_to_jiffies(delay_ms +
-						    prandom_u32() % RTRS_RECONNECT_SEED));
+		queue_work(rtrs_wq, &clt_path->err_recovery_work);
 	} else {
 		/*
 		 * Error can happen just on establishing new connection,
@@ -1501,6 +1493,22 @@ static void rtrs_clt_init_hb(struct rtrs_clt_path *clt_path)
 static void rtrs_clt_reconnect_work(struct work_struct *work);
 static void rtrs_clt_close_work(struct work_struct *work);
 
+static void rtrs_clt_err_recovery_work(struct work_struct *work)
+{
+	struct rtrs_clt_path *clt_path;
+	struct rtrs_clt_sess *clt;
+	int delay_ms;
+
+	clt_path = container_of(work, struct rtrs_clt_path, err_recovery_work);
+	clt = clt_path->clt;
+	delay_ms = clt->reconnect_delay_sec * 1000;
+	rtrs_clt_stop_and_destroy_conns(clt_path);
+	queue_delayed_work(rtrs_wq, &clt_path->reconnect_dwork,
+			   msecs_to_jiffies(delay_ms +
+					    prandom_u32() %
+					    RTRS_RECONNECT_SEED));
+}
+
 static struct rtrs_clt_path *alloc_path(struct rtrs_clt_sess *clt,
 					const struct rtrs_addr *path,
 					size_t con_num, u32 nr_poll_queues)
@@ -1552,6 +1560,7 @@ static struct rtrs_clt_path *alloc_path(struct rtrs_clt_sess *clt,
 	clt_path->state = RTRS_CLT_CONNECTING;
 	atomic_set(&clt_path->connected_cnt, 0);
 	INIT_WORK(&clt_path->close_work, rtrs_clt_close_work);
+	INIT_WORK(&clt_path->err_recovery_work, rtrs_clt_err_recovery_work);
 	INIT_DELAYED_WORK(&clt_path->reconnect_dwork, rtrs_clt_reconnect_work);
 	rtrs_clt_init_hb(clt_path);
 
@@ -2321,6 +2330,7 @@ static void rtrs_clt_close_work(struct work_struct *work)
 
 	clt_path = container_of(work, struct rtrs_clt_path, close_work);
 
+	cancel_work_sync(&clt_path->err_recovery_work);
 	cancel_delayed_work_sync(&clt_path->reconnect_dwork);
 	rtrs_clt_stop_and_destroy_conns(clt_path);
 	rtrs_clt_change_state_get_old(clt_path, RTRS_CLT_CLOSED, NULL);
@@ -2633,7 +2643,6 @@ static void rtrs_clt_reconnect_work(struct work_struct *work)
 {
 	struct rtrs_clt_path *clt_path;
 	struct rtrs_clt_sess *clt;
-	unsigned int delay_ms;
 	int err;
 
 	clt_path = container_of(to_delayed_work(work), struct rtrs_clt_path,
@@ -2650,8 +2659,6 @@ static void rtrs_clt_reconnect_work(struct work_struct *work)
 	}
 	clt_path->reconnect_attempts++;
 
-	/* Stop everything */
-	rtrs_clt_stop_and_destroy_conns(clt_path);
 	msleep(RTRS_RECONNECT_BACKOFF);
 	if (rtrs_clt_change_state_get_old(clt_path, RTRS_CLT_CONNECTING, NULL)) {
 		err = init_path(clt_path);
@@ -2664,11 +2671,7 @@ static void rtrs_clt_reconnect_work(struct work_struct *work)
 reconnect_again:
 	if (rtrs_clt_change_state_get_old(clt_path, RTRS_CLT_RECONNECTING, NULL)) {
 		clt_path->stats->reconnects.fail_cnt++;
-		delay_ms = clt->reconnect_delay_sec * 1000;
-		queue_delayed_work(rtrs_wq, &clt_path->reconnect_dwork,
-				   msecs_to_jiffies(delay_ms +
-						    prandom_u32() %
-						    RTRS_RECONNECT_SEED));
+		queue_work(rtrs_wq, &clt_path->err_recovery_work);
 	}
 }
 
@@ -2900,6 +2903,7 @@ int rtrs_clt_reconnect_from_sysfs(struct rtrs_clt_path *clt_path)
 						 &old_state);
 	if (changed) {
 		clt_path->reconnect_attempts = 0;
+		rtrs_clt_stop_and_destroy_conns(clt_path);
 		queue_delayed_work(rtrs_wq, &clt_path->reconnect_dwork, 0);
 	}
 	if (changed || old_state == RTRS_CLT_RECONNECTING) {
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.h b/drivers/infiniband/ulp/rtrs/rtrs-clt.h
index d1b18a154ae0..f848c0392d98 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.h
@@ -134,6 +134,7 @@ struct rtrs_clt_path {
 	struct rtrs_clt_io_req	*reqs;
 	struct delayed_work	reconnect_dwork;
 	struct work_struct	close_work;
+	struct work_struct	err_recovery_work;
 	unsigned int		reconnect_attempts;
 	bool			established;
 	struct rtrs_rbuf	*rbufs;
-- 
2.25.1

Re: [PATCH for-next 0/5] Misc update for RTRS

[Haris Iqbal]

On Fri, Jan 14, 2022 at 4:48 PM Md Haris Iqbal <haris.iqbal@ionos.com> wrote:
>
> Hi Jason, hi Doug,
>
> Please consider to include following changes to the next merge window.

Ping.

>
> The patchset is organized as:
> - patch1, patch2, patch3 fixes warnings generated from checkpatch
> - patch4 updates a comment.
> - patch5 In case of error, performs failover earlier & avoids deadlock
>
> Gioh Kim (3):
>   RDMA/rtrs: fix CHECK:BRACES type warning
>   RDMA/rtrs-clt: fix CHECK type warnings
>   RDMA/rtrs-clt: fix CHECK type warnings
>
> Jack Wang (2):
>   RDMA/rtrs-clt: Update one outdated comment in path_it_deinit
>   RDMA/rtrs-clt: Do stop and failover outside reconnect work.
>
>  drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c |  3 +-
>  drivers/infiniband/ulp/rtrs/rtrs-clt.c       | 91 ++++++++++----------
>  drivers/infiniband/ulp/rtrs/rtrs-clt.h       |  1 +
>  drivers/infiniband/ulp/rtrs/rtrs.c           |  1 -
>  4 files changed, 47 insertions(+), 49 deletions(-)
>
> --
> 2.25.1
>

Re: [PATCH for-next 0/5] Misc update for RTRS

[Jason Gunthorpe]

On Wed, Jan 26, 2022 at 02:56:31PM +0100, Haris Iqbal wrote:
> On Fri, Jan 14, 2022 at 4:48 PM Md Haris Iqbal <haris.iqbal@ionos.com> wrote:
> >
> > Hi Jason, hi Doug,
> >
> > Please consider to include following changes to the next merge window.
> 
> Ping.

I don't look at most patches during the merge window.

> >
> > The patchset is organized as:
> > - patch1, patch2, patch3 fixes warnings generated from checkpatch
> > - patch4 updates a comment.
> > - patch5 In case of error, performs failover earlier & avoids deadlock
> >
> > Gioh Kim (3):
> >   RDMA/rtrs: fix CHECK:BRACES type warning
> >   RDMA/rtrs-clt: fix CHECK type warnings
> >
> > Jack Wang (2):
> >   RDMA/rtrs-clt: Update one outdated comment in path_it_deinit
> >   RDMA/rtrs-clt: Do stop and failover outside reconnect work.

I took these ones to for-next

> >   RDMA/rtrs-clt: fix CHECK type warnings

This one needs some more thinking

I fixed all the commit messages too

Thanks,
Jason