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

[Jakub Kicinski <kuba@kernel.org>]

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

This replaces the wait_event_interruptible_timeout() in
smc_wr_tx_get_free_slot() with an open-coded loop built on
prepare_to_wait_exclusive(), so a wake_up() on link->wr_tx_wait only wakes
one waiter per freed TX slot.  It also changes the return value on a
pending signal from -EPIPE to -ERESTARTSYS.

A few questions below about the new wait loop and about the changed return
code.

The commit message says:

  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.

[Severity: Low]

Should this part carry a Fixes: tag?  git blame on the baseline points the
wait_event_interruptible_timeout() call and the timeout-only "if (!rc)"
handling at f38ba179c6ca ("smc: work request (WR) base for use by LLC and
CDC"), with later touches in 15e1b99aadfb27 and 87523930a16eb5.  The
corrected value is user visible via smc_shutdown() -> smc_close_active()
-> smc_close_final() -> smc_wr_tx_get_free_slot().

Also, would it make sense to split the return-code correction from the
wait-queue rework?  As it stands both live in the same hunk, so the
corrected error code cannot be backported without the performance change.

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

[Severity: Medium]

Can a freed TX slot end up unclaimed while another waiter keeps sleeping?

prepare_to_wait_exclusive() queues the entry with WQ_FLAG_EXCLUSIVE and sets
TASK_INTERRUPTIBLE before the condition is evaluated, and the condition here
is not cheap - smc_wr_tx_get_free_slot_index() scans up to wr_tx_cnt bits:

net/smc/smc_wr.c:smc_wr_tx_get_free_slot_index() {
	...
	for_each_clear_bit(*idx, link->wr_tx_mask, link->wr_tx_cnt) {
		if (!test_and_set_bit(*idx, link->wr_tx_mask))
			return 0;
	}
	*idx = link->wr_tx_cnt;
	return -EBUSY;
}

Both slot producers use plain wake_up(), which is nr_exclusive = 1, so
exactly one exclusive waiter is woken per freed slot:

net/smc/smc_wr.c:smc_wr_tx_process_cqe() {
	...
	wake_up(&link->wr_tx_wait);
}

net/smc/smc_wr.c:smc_wr_tx_put_slot() {
	...
	test_and_clear_bit(idx, link->wr_tx_mask);
	wake_up(&link->wr_tx_wait);
}

The interleaving I have in mind:

  1) all slots busy, waiter W sleeps on link->wr_tx_wait
  2) a CQE frees slot i, wake_up() wakes W, autoremove_wake_function()
     dequeues W, the credit is consumed
  3) before W runs, a fresh process-context caller T enters the loop,
     prepare_to_wait_exclusive() queues T, T's first scan takes slot i
     (a bare test_and_set_bit(), no reservation) and T breaks out
  4) W re-checks, finds nothing free, requeues and sleeps again
  5) slot j is freed, wake_up() walks the queue, reaches T first (still
     queued and TASK_INTERRUPTIBLE), T absorbs the credit and returns

Slot j is now free while W sleeps.  Since the loop never issues another
wake_up(&link->wr_tx_wait) when it exits without consuming the announced
slot, is that credit simply dropped by finish_wait()?  Would a
wake_up(&link->wr_tx_wait) on the paths that leave the queue without
claiming the announced slot be appropriate here?

With the previous non-exclusive wait_event_interruptible_timeout(), every
wake_up() re-evaluated the condition for all waiters, so a free slot could
not coexist with sleeping waiters.

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

Related to the above: if a waiter misses its wakeup and no further slot is
released within SMC_WR_TX_WAIT_FREE_SLOT_TIME, does it reach this branch and
call smcr_link_down_cond_sched(), tearing down the link for the whole link
group even though a TX slot was actually available?

> +			if (signal_pending(current)) {
> +				rc = -ERESTARTSYS;
> +				break;
> +			}

[Severity: Medium]

Is returning raw -ERESTARTSYS to all callers of
smc_wr_tx_get_free_slot() safe?  The sibling blocking helper in the same
file converts it:

net/smc/smc_wr.c:smc_wr_reg_send() {
	...
	if (rc == -ERESTARTSYS)
		return -EINTR;
	...
}

The path I traced is shutdown(fd, SHUT_RDWR) on an SMC_ACTIVE socket:

  smc_shutdown() -> smc_close_active() -> smc_close_final() ->
  smc_cdc_get_slot_and_msg_send() -> smc_cdc_get_free_slot() ->
  smc_wr_tx_get_free_slot()

smc_close_active() advances the state machine before it looks at the return
value, setting local_tx_ctrl.conn_state_flags.peer_conn_closed = 1 and
sk->sk_state = SMC_PEERCLOSEWAIT1.  With all TX slots busy and a signal
pending, -ERESTARTSYS now reaches smc_shutdown(), which returns it verbatim.

With SA_RESTART the syscall is restarted transparently, re-enters
smc_shutdown() in state SMC_PEERCLOSEWAIT1 where smc_close_sent_any_close()
is already true, so nothing is sent and shutdown() returns 0.  Does this
report success to the application although the SMC close message was never
transmitted?  The baseline returned -EPIPE here, so the failure was visible.

The commit message states the new value is "matching the standard
interruptible-wait semantics and letting the syscall restart machinery
handle it" - does that hold for a call site that has already committed side
effects before the error is returned?

Other callers only special-case -EBUSY, for example:

net/smc/smc_tx.c:smcr_tx_sndbuf_nonempty() {
	...
	rc = smc_cdc_get_free_slot(conn, link, &wr_buf, &wr_rdma_buf, &pend);
	if (rc < 0) {
		smc_wr_tx_link_put(link);
		if (rc == -EBUSY) { ... }
		return rc;
	}
	...
}

so -ERESTARTSYS is propagated as a hard error with no retry queued.

For completeness on what I could not confirm: on the connect path every
error is converted to an SMC_CLC_DECL_* code before reaching
"smc->sk.sk_err = -rc", so -ERESTARTSYS does not appear to leak out through
SO_ERROR or poll, and I could not demonstrate signal_pending() becoming true
inside workqueue workers.

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

[ ... ]