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

[PATCH net-next 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.

For patch 2, uperf numbers are now included in the commit message, as
requested by Mahanta during v1 review. The short version is that the
gain tracks how often the TX slot wait path is actually taken: with the
default sysctl settings, where a link group multiplexes many connections
over a small send queue, throughput improves by 134% on the 200x1000
request/response workload and by 458% on the 1-byte ping-pong workload;
with a tuned configuration where slots are rarely exhausted, the change
is neutral (+3.4% and +1.0%, i.e. within noise). See patch 2 for the
full table.

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

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 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 97ba46893b17..def2ab84b0c7 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 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] DMA use-after-free/unmap during link teardown due to ignored v2 TX pending state.
- [High] TOCTOU data race condition in V2 slot management causes data corruption of outgoing LLC messages due to lockless memset.
--

commit 4dd2538a0566b239179ada14640f3f763de057d5
Author: D. Wythe <alibuda@linux.alibaba.com>

net/smc: clear the correct v2 slot and buffer in smc_wr_tx_put_slot()
    
This commit fixes a bug where pointers were being zeroed instead of the
structures they point to when clearing the v2 pending slot and buffer. By
passing the pointers directly and using sizeof(*pointer), it ensures the
intended structures are actually zeroed.

> diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
> index 97ba46893b17f..def2ab84b0c71 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));

[Severity: High]
This isn't a bug introduced by this patch, but since the memset is now
correctly clearing the struct, could this create a data race with
smc_wr_tx_get_v2_slot()?

The first memset clears the entire smc_wr_tx_pend structure, which includes
the idx field. Once idx becomes 0, a concurrent thread calling
smc_wr_tx_get_v2_slot() might locklessly see that the slot is available
since it checks:

    if (link->wr_tx_v2_pend->idx == link->wr_tx_cnt)
        return -EBUSY;
        
If the concurrent thread grabs the slot and starts writing data into the
payload buffer, the second memset here will then overwrite that newly claimed
buffer with zeroes. Could this cause the hardware to transmit a corrupted
or empty message?

[Severity: High]
This is also a pre-existing issue, but while reviewing the V2 TX pending 
state logic, could there be a DMA use-after-free during link teardown?

During teardown, smc_wr_free_link() waits for pending sends to finish by
calling smc_wr_tx_wait_no_pending_sends(), which relies on:

    static inline bool smc_wr_is_tx_pend(struct smc_link *link)
    {
        ...
        return !bitmap_empty(link->wr_tx_mask, link->wr_tx_cnt);
    }

This only checks the wr_tx_mask bitmap. However, the V2 TX slot does not use
the mask; its pending state is tracked by link->wr_tx_v2_pend->idx. 

If only a V2 TX request is in flight, would smc_wr_is_tx_pend() return false
and allow smc_wr_tx_wait_no_pending_sends() to return early? This could allow
subsequent DMA unmap operations to free memory while the hardware is still
transmitting the V2 message.

>  		return 1;
>  	}

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

[PATCH net-next 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.

Performance
===========

Measured with uperf between two peers over SMC-R. The benefit depends
on how often the TX slot wait path is actually taken.

With the default settings, where many connections share a link group
and the send queue is small, slots are scarce and the wait path is hot:

  net.smc.smcr_max_conns_per_lgr = 255
  net.smc.smcr_max_send_wr       = 16
  net.smc.smcr_max_recv_wr       = 48

  workload                 baseline      patched     delta
  ---------------------------------------------------------
  rr1c-200x1000-50.xml     655.06 Mb/s   1.53 Gb/s   +134%
  rr1c-1x1-250.xml         371.03 Kb/s   2.07 Mb/s   +458%

With a tuned configuration, where slots are mostly available and the
wait path is rarely entered, the change is neutral to slightly positive:

  net.smc.smcr_max_conns_per_lgr = 32
  net.smc.smcr_max_send_wr       = 64
  net.smc.smcr_max_recv_wr       = 64

  workload                 baseline      patched     delta
  ---------------------------------------------------------
  rr1c-200x1000-50.xml     1.74 Gb/s     1.80 Gb/s   +3.4%
  rr1c-1x1-250.xml         3.11 Mb/s     3.14 Mb/s   +1.0%

So the change does not regress the uncontended case, and recovers most
of the throughput lost to thundering-herd wakeups once slots become
scarce.

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
Reviewed-by: Wen Gu <guwen@linux.alibaba.com>
Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.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 def2ab84b0c7..64413008f9c1 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 2/2] net/smc: reduce TX slot contention with exclusive wait

[Mahanta Jambigi]

On 10/09/26 4:14 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.
> 
> 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.
> 
> Performance
> ===========
> 
> Measured with uperf between two peers over SMC-R. The benefit depends
> on how often the TX slot wait path is actually taken.
> 
> With the default settings, where many connections share a link group
> and the send queue is small, slots are scarce and the wait path is hot:
> 
>   net.smc.smcr_max_conns_per_lgr = 255
>   net.smc.smcr_max_send_wr       = 16
>   net.smc.smcr_max_recv_wr       = 48
> 
>   workload                 baseline      patched     delta
>   ---------------------------------------------------------
>   rr1c-200x1000-50.xml     655.06 Mb/s   1.53 Gb/s   +134%
>   rr1c-1x1-250.xml         371.03 Kb/s   2.07 Mb/s   +458%
> 
> With a tuned configuration, where slots are mostly available and the
> wait path is rarely entered, the change is neutral to slightly positive:
> 
>   net.smc.smcr_max_conns_per_lgr = 32
>   net.smc.smcr_max_send_wr       = 64
>   net.smc.smcr_max_recv_wr       = 64
> 
>   workload                 baseline      patched     delta
>   ---------------------------------------------------------
>   rr1c-200x1000-50.xml     1.74 Gb/s     1.80 Gb/s   +3.4%
>   rr1c-1x1-250.xml         3.11 Mb/s     3.14 Mb/s   +1.0%
> 
> So the change does not regress the uncontended case, and recovers most
> of the throughput lost to thundering-herd wakeups once slots become
> scarce.
> 
> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
> Reviewed-by: Wen Gu <guwen@linux.alibaba.com>
> Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>

I don't see a changelog describing what has changed since the previous
version. Any Sashiko comments were addressed? Could you explain why this
is being reposted?

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

[sashiko-bot]

> 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
> [ ... ]
> So the change does not regress the uncontended case, and recovers most
> of the throughput lost to thundering-herd wakeups once slots become
> scarce.
> 
> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH net-next 0/2] net/smc: transition to RDMA core CQ pooling

[D. Wythe]

This series transitions SMC-R completion handling to RDMA core CQ pooling
via the ib_cqe API. The new completion model improves scalability by
allowing per-link completion processing across multiple cores and enables
DIM-based interrupt moderation.

As a side effect, the increased concurrency can amplify contention for TX
slots on the shared wait queue. Patch 2 addresses this by switching TX slot
allocation from non-exclusive wait_event() to prepare_to_wait_exclusive(),
which avoids thundering-herd wakeups under contention.

Patch 1 replaces the global per-device CQ and manual tasklet polling model
with RDMA core CQ pooling.
Patch 2 reduces TX slot contention by using exclusive wait queue entries
during allocation.

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

D. Wythe (2):
  net/smc: transition to RDMA core CQ pooling
  net/smc: reduce TX slot contention with exclusive wait

 net/smc/smc_core.c |   9 +-
 net/smc/smc_core.h |  28 ++--
 net/smc/smc_ib.c   | 113 +++++----------
 net/smc/smc_ib.h   |   7 -
 net/smc/smc_tx.c   |   1 -
 net/smc/smc_wr.c   | 344 ++++++++++++++++++++-------------------------
 net/smc/smc_wr.h   |  40 ++----
 7 files changed, 215 insertions(+), 327 deletions(-)

-- 
2.45.0

[PATCH net-next 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.

Performance measured with netperf TCP_RR (63 flows, 200B write /
1000B read, 60s duration):

+-------------------------------+---------------+---------------+
| smcr_max_conns_per_lgr        | 32            | 255           |
|-------------------------------+---------------+---------------|
| before                        | 4.85 Gb/s     | 657.95 Mb/s   |
|-------------------------------+---------------+---------------|
| after                         | 5.01 Gb/s     | 2.2 Gb/s      |
+-------------------------------+---------------+---------------+

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
 net/smc/smc_wr.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
index 48037a3d97a3..0a6f2befb0e2 100644
--- a/net/smc/smc_wr.c
+++ b/net/smc/smc_wr.c
@@ -159,9 +159,11 @@ 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;
+	DEFINE_WAIT(wait);
 	int rc;
 
 	*wr_buf = NULL;
@@ -171,17 +173,27 @@ 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);
+			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 2/2] net/smc: reduce TX slot contention with exclusive wait

[Paolo Abeni]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net/smc: reduce TX slot contention with exclusive wait

This patch replaces the non-exclusive wait_event_interruptible_timeout()
in smc_wr_tx_get_free_slot() with an open-coded exclusive wait loop via
prepare_to_wait_exclusive(), so wake_up() on a freed TX slot wakes only
a single waiter instead of the entire wait queue.

> diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
> index 48037a3d97a3..0a6f2befb0e2 100644
> --- a/net/smc/smc_wr.c
> +++ b/net/smc/smc_wr.c
> @@ -171,17 +173,27 @@ 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);
> +			if (!timeout) {
> +				/* timeout - terminate link */
> +				smcr_link_down_cond_sched(link);
> +				break;
> +			}
> +			if (signal_pending(current)) {
> +				rc = -ERESTARTSYS;
> +				break;
> +			}

[High]
With exclusive waiters, wake_up() on link->wr_tx_wait wakes only one
task per freed slot. If that waiter is about to exit via the
signal_pending() branch, can the freed slot be lost to the remaining
sleepers?

Consider two exclusive waiters A and B asleep in schedule_timeout():

  1. smc_wr_tx_put_slot() -> wake_up(&link->wr_tx_wait) is effectively
     __wake_up(..., nr_exclusive=1).  default_wake_function makes A
     runnable, autoremove_wake_function pulls A off the queue, and
     __wake_up_common stops -- B is not woken.

  2. A signal is delivered to A before it actually runs.  Because A is
     already TASK_RUNNING, signal_wake_up only sets TIF_SIGPENDING.

  3. A returns from schedule_timeout() with timeout > 0, so the
     !timeout branch is skipped.  signal_pending(current) is true, so
     the loop sets rc = -ERESTARTSYS and breaks without re-testing
     smc_wr_tx_get_free_slot_index() and without issuing any wake_up()
     on the way out.

  4. B stays asleep on the wait queue even though the wr_tx_mask bit
     for the freed slot is still available.  With no further
     completions, B sleeps until the full SMC_WR_TX_WAIT_FREE_SLOT_TIME
     expires, at which point smcr_link_down_cond_sched() tears down an
     otherwise healthy link.

The previous wait_event_interruptible_timeout() was non-exclusive, so
every waiter was woken on each wake_up() and re-evaluated the
condition, which avoided this class of lost-wakeup.

Should the signal path re-check the slot condition before returning,
or issue a wake_up() on exit so another exclusive waiter gets a chance
at the slot?

[Medium]
The commit message says this is a contention optimization and that
"teardown behavior is unchanged", but does it also change the errno
returned to callers on signal delivery?

Before this patch, wait_event_interruptible_timeout() returned
-ERESTARTSYS on signal; that value is non-zero, so the old if (!rc)
branch was skipped, control fell through to the trailing
if (idx == link->wr_tx_cnt) return -EPIPE, and callers saw -EPIPE.

After the patch, the signal path does:

    if (signal_pending(current)) {
        rc = -ERESTARTSYS;
        break;
    }
    ...
    finish_wait(&link->wr_tx_wait, &wait);
    if (rc)
        return rc;

so -ERESTARTSYS propagates up through smc_cdc_get_free_slot() (which
only remaps rc to -EPIPE when conn->killed is set),
smcr_cdc_get_slot_and_msg_send(), smc_cdc_get_slot_and_msg_send() and
smc_tx_sendmsg(), meaning userspace sees syscall restart / -EINTR
instead of -EPIPE.

Note that the sibling smc_wr_reg_send() in the same file explicitly
converts -ERESTARTSYS to -EINTR.  Should this return-code change be
called out in the commit message (and possibly split out), given that
the stated intent of the patch is purely to reduce contention?

[Low]
On timeout expiry the new loop takes the teardown path without
re-testing whether a slot became available:

> +			timeout = schedule_timeout(timeout);
> +			if (!timeout) {
> +				/* timeout - terminate link */
> +				smcr_link_down_cond_sched(link);
> +				break;
> +			}

The old wait_event_interruptible_timeout() expansion re-evaluated the
condition via ___wait_cond_timeout after schedule_timeout() returned
0, so a wake_up() racing with timer expiry could still report success
and let smc_wr_tx_get_free_slot_index() claim the slot.

In the narrow race where a slot is freed at the same instant the
timer fires, does this now tear the link down even though a slot was
actually available?

>  		}
> +		finish_wait(&link->wr_tx_wait, &wait);
> +		if (rc)
> +			return rc;
>  		if (idx == link->wr_tx_cnt)
>  			return -EPIPE;
>  	}
-- 
This is an AI-generated review.

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

[D. Wythe]

On Tue, May 12, 2026 at 10:26:43AM +0200, Paolo Abeni wrote:
> From: AI Reviewer <ai@example.com>
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> net/smc: reduce TX slot contention with exclusive wait
> 
> This patch replaces the non-exclusive wait_event_interruptible_timeout()
> in smc_wr_tx_get_free_slot() with an open-coded exclusive wait loop via
> prepare_to_wait_exclusive(), so wake_up() on a freed TX slot wakes only
> a single waiter instead of the entire wait queue.
> 
> > diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
> > index 48037a3d97a3..0a6f2befb0e2 100644
> > --- a/net/smc/smc_wr.c
> > +++ b/net/smc/smc_wr.c
> > @@ -171,17 +173,27 @@ 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);
> > +			if (!timeout) {
> > +				/* timeout - terminate link */
> > +				smcr_link_down_cond_sched(link);
> > +				break;
> > +			}
> > +			if (signal_pending(current)) {
> > +				rc = -ERESTARTSYS;
> > +				break;
> > +			}
> 
> [High]
> With exclusive waiters, wake_up() on link->wr_tx_wait wakes only one
> task per freed slot. If that waiter is about to exit via the
> signal_pending() branch, can the freed slot be lost to the remaining
> sleepers?
> 
> Consider two exclusive waiters A and B asleep in schedule_timeout():
> 
>   1. smc_wr_tx_put_slot() -> wake_up(&link->wr_tx_wait) is effectively
>      __wake_up(..., nr_exclusive=1).  default_wake_function makes A
>      runnable, autoremove_wake_function pulls A off the queue, and
>      __wake_up_common stops -- B is not woken.
> 
>   2. A signal is delivered to A before it actually runs.  Because A is
>      already TASK_RUNNING, signal_wake_up only sets TIF_SIGPENDING.
> 
>   3. A returns from schedule_timeout() with timeout > 0, so the
>      !timeout branch is skipped.  signal_pending(current) is true, so
>      the loop sets rc = -ERESTARTSYS and breaks without re-testing
>      smc_wr_tx_get_free_slot_index() and without issuing any wake_up()
>      on the way out.
> 
>   4. B stays asleep on the wait queue even though the wr_tx_mask bit
>      for the freed slot is still available.  With no further
>      completions, B sleeps until the full SMC_WR_TX_WAIT_FREE_SLOT_TIME
>      expires, at which point smcr_link_down_cond_sched() tears down an
>      otherwise healthy link.
> 
> The previous wait_event_interruptible_timeout() was non-exclusive, so
> every waiter was woken on each wake_up() and re-evaluated the
> condition, which avoided this class of lost-wakeup.
> 
> Should the signal path re-check the slot condition before returning,
> or issue a wake_up() on exit so another exclusive waiter gets a chance
> at the slot?
> 

Thanks for the review, all three points are valid.

[High] & [Low]: Both resolved by a unified re-check after finish_wait():
if no slot was acquired in the loop (due to signal or timeout),
re-evaluate the condition one final time. If a slot is now available,
claim it and proceed. No wake_up() is needed on exit — the teardown path
already does wake_up_all when the link goes down, and waking the next
waiter when no slot exists is pointless.

> [Medium]
> The commit message says this is a contention optimization and that
> "teardown behavior is unchanged", but does it also change the errno
> returned to callers on signal delivery?
> 
> Before this patch, wait_event_interruptible_timeout() returned
> -ERESTARTSYS on signal; that value is non-zero, so the old if (!rc)
> branch was skipped, control fell through to the trailing
> if (idx == link->wr_tx_cnt) return -EPIPE, and callers saw -EPIPE.
> 
> After the patch, the signal path does:
> 
>     if (signal_pending(current)) {
>         rc = -ERESTARTSYS;
>         break;
>     }
>     ...
>     finish_wait(&link->wr_tx_wait, &wait);
>     if (rc)
>         return rc;
> 
> so -ERESTARTSYS propagates up through smc_cdc_get_free_slot() (which
> only remaps rc to -EPIPE when conn->killed is set),
> smcr_cdc_get_slot_and_msg_send(), smc_cdc_get_slot_and_msg_send() and
> smc_tx_sendmsg(), meaning userspace sees syscall restart / -EINTR
> instead of -EPIPE.
> 
> Note that the sibling smc_wr_reg_send() in the same file explicitly
> converts -ERESTARTSYS to -EINTR.  Should this return-code change be
> called out in the commit message (and possibly split out), given that
> the stated intent of the patch is purely to reduce contention?

Agreed. I'll keep the return code as -EPIPE to match the original behavior, so
this patch remains a pure contention optimization with no semantic
change.


> [Low]
> On timeout expiry the new loop takes the teardown path without
> re-testing whether a slot became available:
> 
> > +			timeout = schedule_timeout(timeout);
> > +			if (!timeout) {
> > +				/* timeout - terminate link */
> > +				smcr_link_down_cond_sched(link);
> > +				break;
> > +			}
> 
> The old wait_event_interruptible_timeout() expansion re-evaluated the
> condition via ___wait_cond_timeout after schedule_timeout() returned
> 0, so a wake_up() racing with timer expiry could still report success
> and let smc_wr_tx_get_free_slot_index() claim the slot.
> 
> In the narrow race where a slot is freed at the same instant the
> timer fires, does this now tear the link down even though a slot was
> actually available?
> 
> >  		}
> > +		finish_wait(&link->wr_tx_wait, &wait);
> > +		if (rc)
> > +			return rc;
> >  		if (idx == link->wr_tx_cnt)
> >  			return -EPIPE;
> >  	}

D. Wythe

> -- 
> This is an AI-generated review.