[PATCH net-next v1 0/2] net/smc: fix v2 slot clearing and reduce TX slot contention

[PATCH net-next v1 0/2] net/smc: fix v2 slot clearing and reduce TX slot contention

[D. Wythe]

This series contains the two reviewed patches from the previously posted
"net/smc: transition to RDMA core CQ pooling" series (v4), reposted as a
standalone series so they can land independently. The remaining CQ pooling
patch will be reposted separately after these two are merged.

Patch 1 fixes smc_wr_tx_put_slot() to clear the v2 pending slot and buffer
structures instead of the pointer variables, the memset targets were the
8-byte pointers themselves so the structures were never actually cleared.
Patch 2 reduces TX slot contention by switching TX slot allocation from
non-exclusive wait_event() to prepare_to_wait_exclusive(), avoiding
thundering-herd wakes when slots are scarce.

Link: https://lore.kernel.org/netdev/20260721175309.321b6503@kernel.org/
Link: https://lore.kernel.org/netdev/20260716113745.65234-1-alibuda@linux.alibaba.com/

---
This is a repost of patch 1/2 and 2/2 from the v4 "net/smc: transition to
RDMA core CQ pooling" series, with no code changes. Version numbering is
restarted at v1 since the series is now standalone.

D. Wythe (2):
  net/smc: clear the correct v2 slot and buffer in smc_wr_tx_put_slot()
  net/smc: reduce TX slot contention with exclusive wait

 net/smc/smc_wr.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

-- 
2.45.0

[PATCH net-next v1 1/2] net/smc: clear the correct v2 slot and buffer in smc_wr_tx_put_slot()

[D. Wythe]

smc_wr_tx_put_slot() tries to reset the v2 pending slot and buffer with
memset(&link->wr_tx_v2_pend, 0, sizeof(link->wr_tx_v2_pend)) and the
equivalent for wr_tx_buf_v2. Both are pointers, so this zeroes the 8-byte
pointer variable instead of the structure it points to. The pending slot
and buffer are therefore never actually cleared, and the pointers get
overwritten with NULL.

Pass the pointers directly and use sizeof(*pointer) so the intended
structures are cleared.

Fixes: 8799e310fb3f ("net/smc: add v2 support to the work request layer")
Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
---
 net/smc/smc_wr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
index 59c92b46945c..6b5add922993 100644
--- a/net/smc/smc_wr.c
+++ b/net/smc/smc_wr.c
@@ -288,10 +288,10 @@ int smc_wr_tx_put_slot(struct smc_link *link,
 	} else if (link->lgr->smc_version == SMC_V2 &&
 		   pend->idx == link->wr_tx_cnt) {
 		/* Large v2 buffer */
-		memset(&link->wr_tx_v2_pend, 0,
-		       sizeof(link->wr_tx_v2_pend));
-		memset(&link->lgr->wr_tx_buf_v2, 0,
-		       sizeof(link->lgr->wr_tx_buf_v2));
+		memset(link->wr_tx_v2_pend, 0,
+		       sizeof(*link->wr_tx_v2_pend));
+		memset(link->lgr->wr_tx_buf_v2, 0,
+		       sizeof(*link->lgr->wr_tx_buf_v2));
 		return 1;
 	}
 
-- 
2.45.0

Re: [PATCH net-next v1 1/2] net/smc: clear the correct v2 slot and buffer in smc_wr_tx_put_slot()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Non-atomic clearing of `wr_tx_v2_pend` before `wr_tx_buf_v2` creates a TOCTOU race that prematurely signals slot availability, corrupting concurrent V2 messages and control data.
- [High] An error completion for a V1 work request unconditionally zeroes out the link group-wide V2 buffer, corrupting concurrent V2 messages on other links.
--

--- Patch [1]: [PATCH net-next v1 1/2] net/smc: clear the correct v2 slot and buffer in smc_wr_tx_put_slot() ---
commit 4fe2881d4ba7f681ee22c596087bf040cfe5c6f0
Author: D. Wythe <alibuda@linux.alibaba.com>

net/smc: clear the correct v2 slot and buffer in smc_wr_tx_put_slot()

This patch fixes an issue where 8-byte pointer variables were being zeroed
instead of the structures they point to. It passes the pointers directly
and uses sizeof(*pointer) to properly clear the intended structures.

> diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
> --- a/net/smc/smc_wr.c
> +++ b/net/smc/smc_wr.c
> @@ -288,10 +288,10 @@ int smc_wr_tx_put_slot(struct smc_link *link,
>  	} else if (link->lgr->smc_version == SMC_V2 &&
>  		   pend->idx == link->wr_tx_cnt) {
>  		/* Large v2 buffer */
> -		memset(&link->wr_tx_v2_pend, 0,
> -		       sizeof(link->wr_tx_v2_pend));
> -		memset(&link->lgr->wr_tx_buf_v2, 0,
> -		       sizeof(link->lgr->wr_tx_buf_v2));
> +		memset(link->wr_tx_v2_pend, 0,
> +		       sizeof(*link->wr_tx_v2_pend));
> +		memset(link->lgr->wr_tx_buf_v2, 0,
> +		       sizeof(*link->lgr->wr_tx_buf_v2));

[Severity: High]
This isn't a bug introduced by this patch, but it looks like this lockless
synchronization pattern might have a race condition.

Because memset() is not atomic, could this zero out the state tracking struct
(which contains idx) before the data buffer is fully cleared?

If a concurrent thread calls smc_wr_tx_get_v2_slot():

    if (link->wr_tx_v2_pend->idx == link->wr_tx_cnt)
        return -EBUSY;

It checks the idx to locklessly determine slot availability. If idx gets
zeroed early by the first memset(), could the concurrent thread claim the
slot and write new data, only for it to be destroyed by the remainder of
the memset() operations?

For V1 buffers, this appears to be safely handled using a dedicated
link->wr_tx_mask bitmask that is cleared only after the memsets.

[Severity: High]
This is another pre-existing issue in the same file, but could the error
logic in smc_wr_tx_process_cqe() corrupt shared V2 payloads?

smc_wr_tx_process_cqe() {
    ...
    if (wc->status) {
        ...
        if (link->lgr->smc_version == SMC_V2) {
            memset(link->wr_tx_v2_pend, 0,
                   sizeof(*link->wr_tx_v2_pend));
            memset(link->lgr->wr_tx_buf_v2, 0,
                   sizeof(*link->lgr->wr_tx_buf_v2));
        }
    ...
}

If a V1 work request completes with an error, the error path unconditionally
zeroes out link->lgr->wr_tx_buf_v2 just because the link group version is
SMC_V2.

Since this buffer is shared across all links within the link group, wouldn't
zeroing it out from a failing link silently corrupt the payload of any
concurrent V2 message on healthy, surviving links in the same group?

>  		return 1;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806064252.39180-1-alibuda@linux.alibaba.com?part=1

[PATCH net-next v1 2/2] net/smc: reduce TX slot contention with exclusive wait

[D. Wythe]

smc_wr_tx_get_free_slot() waits for a free TX slot with
wait_event_interruptible_timeout(). Since the wait_event family
enqueues waiters as non-exclusive, wake_up() may wake multiple
waiters even though only one can use the slot, causing
thundering-herd contention when slots are scarce.

Use an exclusive wait loop with prepare_to_wait_exclusive() so
wake_up() wakes only one waiter per freed slot.
smc_wr_wakeup_tx_wait() still uses wake_up_all() during link
teardown, so teardown behavior is unchanged.

This also corrects the return value on a pending signal: the previous
wait_event_interruptible_timeout() path fell through to the "no free
slot" case and returned -EPIPE, masking the signal as a connection
error. The open-coded loop now returns -ERESTARTSYS, matching the
standard interruptible-wait semantics and letting the syscall restart
machinery handle it.

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
Reviewed-by: Wen Gu <guwen@linux.alibaba.com>
---
 net/smc/smc_wr.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
index 6b5add922993..2cdd4063c13d 100644
--- a/net/smc/smc_wr.c
+++ b/net/smc/smc_wr.c
@@ -198,11 +198,13 @@ int smc_wr_tx_get_free_slot(struct smc_link *link,
 			    struct smc_rdma_wr **wr_rdma_buf,
 			    struct smc_wr_tx_pend_priv **wr_pend_priv)
 {
+	unsigned long timeout = SMC_WR_TX_WAIT_FREE_SLOT_TIME;
 	struct smc_link_group *lgr = smc_get_lgr(link);
 	struct smc_wr_tx_pend *wr_pend;
 	u32 idx = link->wr_tx_cnt;
 	struct ib_send_wr *wr_ib;
 	u64 wr_id;
+	DEFINE_WAIT(wait);
 	int rc;
 
 	*wr_buf = NULL;
@@ -212,17 +214,31 @@ int smc_wr_tx_get_free_slot(struct smc_link *link,
 		if (rc)
 			return rc;
 	} else {
-		rc = wait_event_interruptible_timeout(
-			link->wr_tx_wait,
-			!smc_link_sendable(link) ||
-			lgr->terminating ||
-			(smc_wr_tx_get_free_slot_index(link, &idx) != -EBUSY),
-			SMC_WR_TX_WAIT_FREE_SLOT_TIME);
-		if (!rc) {
-			/* timeout - terminate link */
-			smcr_link_down_cond_sched(link);
-			return -EPIPE;
+		rc = 0;
+		for (;;) {
+			prepare_to_wait_exclusive(&link->wr_tx_wait, &wait,
+						  TASK_INTERRUPTIBLE);
+			if (!smc_link_sendable(link) || lgr->terminating ||
+			    smc_wr_tx_get_free_slot_index(link, &idx) != -EBUSY)
+				break;
+			timeout = schedule_timeout(timeout);
+			/* re-check */
+			if (!smc_link_sendable(link) || lgr->terminating ||
+			    smc_wr_tx_get_free_slot_index(link, &idx) != -EBUSY)
+				break;
+			if (!timeout) {
+				/* timeout - terminate link */
+				smcr_link_down_cond_sched(link);
+				break;
+			}
+			if (signal_pending(current)) {
+				rc = -ERESTARTSYS;
+				break;
+			}
 		}
+		finish_wait(&link->wr_tx_wait, &wait);
+		if (rc)
+			return rc;
 		if (idx == link->wr_tx_cnt)
 			return -EPIPE;
 	}
-- 
2.45.0

Re: [PATCH net-next v1 2/2] net/smc: reduce TX slot contention with exclusive wait

[Mahanta Jambigi]

On 06/08/26 12:12 pm, D. Wythe wrote:
> smc_wr_tx_get_free_slot() waits for a free TX slot with
> wait_event_interruptible_timeout(). Since the wait_event family
> enqueues waiters as non-exclusive, wake_up() may wake multiple
> waiters even though only one can use the slot, causing
> thundering-herd contention when slots are scarce.

Have you run any performance benchmarks (e.g. redis-benchmark or uperf)
to quantify the benefit of this change?

Since the motivation is to reduce contention caused by multiple waiters
being awakened for a single available TX slot, it would be helpful to
see measurable results. Please share the baseline and patched throughput
figures, and any observed throughput differences.

Re: [PATCH net-next v1 2/2] net/smc: reduce TX slot contention with exclusive wait

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The open-coded exclusive wait loop introduces a lost wakeup vulnerability by failing to check the condition before enqueuing.

Pre-existing issues:
- [High] Returning -ERESTARTSYS from smc_wr_tx_get_free_slot causes silent data stalling because the caller does not schedule tx_work to retry, and the error is ignored by sendmsg.
--

--- Patch [2]: [PATCH net-next v1 2/2] net/smc: reduce TX slot contention with exclusive wait ---
commit bcc5b21bf03b53f401940337c34d2ab052835d45
Author: D. Wythe <alibuda@linux.alibaba.com>

net/smc: reduce TX slot contention with exclusive wait

This patch modifies smc_wr_tx_get_free_slot() to use an exclusive wait loop 
using prepare_to_wait_exclusive(). It also updates the return value on a 
pending signal from -EPIPE to -ERESTARTSYS to match standard wait semantics.

> diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
> index 6b5add9229937..2cdd4063c13d6 100644
> --- a/net/smc/smc_wr.c
> +++ b/net/smc/smc_wr.c
[ ... ]
> @@ -212,17 +214,31 @@ int smc_wr_tx_get_free_slot(struct smc_link *link,
>  		if (rc)
>  			return rc;
>  	} else {
> -		rc = wait_event_interruptible_timeout(
> -			link->wr_tx_wait,
> -			!smc_link_sendable(link) ||
> -			lgr->terminating ||
> -			(smc_wr_tx_get_free_slot_index(link, &idx) != -EBUSY),
> -			SMC_WR_TX_WAIT_FREE_SLOT_TIME);
> -		if (!rc) {
> -			/* timeout - terminate link */
> -			smcr_link_down_cond_sched(link);
> -			return -EPIPE;
> +		rc = 0;
> +		for (;;) {
> +			prepare_to_wait_exclusive(&link->wr_tx_wait, &wait,
> +						  TASK_INTERRUPTIBLE);
> +			if (!smc_link_sendable(link) || lgr->terminating ||
> +			    smc_wr_tx_get_free_slot_index(link, &idx) != -EBUSY)
> +				break;

[Severity: High]
Does this open-coded exclusive wait loop introduce a lost wakeup regression?

By calling prepare_to_wait_exclusive() before checking if a slot is available,
the task is unconditionally added to the waitqueue. If it then finds a free
slot immediately and breaks, it remains on the waitqueue until finish_wait() is
called.

If another slot is freed concurrently, an exclusive wake_up() could wake this
exiting task and stop iterating. This absorbs the wakeup and starves other
threads that are actually sleeping on the queue.

This could cause a 10-second stall for the starved thread, leading to a link
termination via smcr_link_down_cond_sched().

> +			timeout = schedule_timeout(timeout);
> +			/* re-check */
> +			if (!smc_link_sendable(link) || lgr->terminating ||
> +			    smc_wr_tx_get_free_slot_index(link, &idx) != -EBUSY)
> +				break;
> +			if (!timeout) {
> +				/* timeout - terminate link */
> +				smcr_link_down_cond_sched(link);
> +				break;
> +			}
> +			if (signal_pending(current)) {
> +				rc = -ERESTARTSYS;
> +				break;
> +			}

[Severity: High]
This is a pre-existing issue, but does returning -ERESTARTSYS
here cause silent data stalling if the caller does not schedule a retry?

If we look at smcr_tx_sndbuf_nonempty() in net/smc/smc_tx.c, it only schedules
tx_work if the return value is -EBUSY:

	if (rc == -EBUSY) {
		...
		mod_delayed_work(conn->lgr->tx_wq, &conn->tx_work,
                                 SMC_TX_WORK_DELAY);
	}
	return rc;

And in smc_tx_sendmsg() in net/smc/smc_tx.c, the return value is ignored:

	if (!smc_tx_should_cork(smc, msg))
		smc_tx_sndbuf_nonempty(conn);

If the syscall is interrupted by a signal, and the data was already copied to
sndbuf, smc_tx_sendmsg() ignores the return value and returns success to
userspace. Since tx_work is not scheduled for -ERESTARTSYS, the data could be
permanently stuck in the buffer.

>  		}
> +		finish_wait(&link->wr_tx_wait, &wait);
> +		if (rc)
> +			return rc;
>  		if (idx == link->wr_tx_cnt)
>  			return -EPIPE;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806064252.39180-1-alibuda@linux.alibaba.com?part=2