Re: [PATCH net-next v7 05/12] net/rds: split connection destroy into quiesce and kref-governed free

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The comment added to rds_conn_destroy()
  (net/rds/connection.c:647-656) and this commit's message justify the…

--

net/rds: split connection destroy into quiesce and kref-governed free

This commit adds a kref to struct rds_connection. It splits
rds_conn_destroy() into a synchronous quiesce step and a deferred free in
rds_conn_destroy_fini(), which runs on the final rds_conn_put(). It also
adds a first-caller-wins guard on c_destroy_in_prog, so a second
rds_conn_destroy() of the same connection returns at once.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index cbc49426ba085..638e9f3140e28 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -590,11 +643,26 @@ void rds_conn_destroy(struct rds_connection *conn)
>  	 * sites (which all test rds_destroy_pending() under
>  	 * rcu_read_lock()) from queueing new work on the path
>  	 * workqueues once we start cancelling and destroying them.
> +	 *
> +	 * Now that the transport state stays discoverable (e.g. on the
> +	 * transports' connection lists) until the final rds_conn_put(),
> +	 * a conn can be handed to rds_conn_destroy() more than once -
> +	 * the IB unload path re-sweeps its connection list until every
> +	 * connection is gone.  Only the first caller proceeds; the
> +	 * unhash also happens under rds_conn_lock, so a looked-up conn
> +	 * can never be quiesced twice.  (With the initial reference the
> +	 * only one, as it is at this point in the series, nothing can
> +	 * reach a second call yet; this is the contract the following
> +	 * patches rely on.)
>  	 */
> +	spin_lock_irq(&rds_conn_lock);
> +	if (conn->c_destroy_in_prog) {
> +		spin_unlock_irq(&rds_conn_lock);
> +		return;
> +	}

[Severity: Low]
Is this comment accurate? At this commit, rds_ib_exit() calls
rds_ib_destroy_nodev_conns() once, and that function makes a single pass
over its list:

net/rds/ib_rdma.c:rds_ib_destroy_nodev_conns() {
    ...
	list_splice(&ib_nodev_conns, &tmp_list);
	spin_unlock_irq(&ib_nodev_conns_lock);

	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
		rds_conn_destroy(ic->conn);
    ...
}

So nothing re-sweeps the list and passes the same conn to
rds_conn_destroy() a second time.

Later in the series, rds_conn_wait_conns_freed() does call
rds_ib_destroy_nodev_conns() again on each poll. However, "net/rds: unlink
transport nodes before a possibly deferred connection free" makes the sweep
claim each node under ib_nodev_conns_lock before the destroy:

	ic->i_ib_node_detached = true;
	list_move_tail(&ic->ib_node, &tmp_list);

Nothing ever clears i_ib_node_detached. rds_ib_add_conn(),
rds_ib_remove_conn() and rds_ib_conn_free() all skip a detached node, so a
gathered conn can't show up on ib_nodev_conns again for a later sweep to
find. The TCP and loop sweeps also unlink and detach each node before
destroying it.

That seems to contradict "the transport state stays discoverable (e.g. on
the transports' connection lists) until the final rds_conn_put()". The
commit message makes the same claim:

  so that a connection handed to it a second time - which the IB unload
  re-sweep added later in the series does - is not quiesced twice

If no caller can reach a second rds_conn_destroy() on the same conn,
should the c_destroy_in_prog guard be dropped? If it stays, could this
comment, the rds_conn_destroy() header comment, the c_destroy_in_prog
comment in rds.h ("so a second destroy of the same connection returns at
once") and the commit message be reworded? As written, they describe a
double destroy that no path produces.

>  	WRITE_ONCE(conn->c_destroy_in_prog, true);
>  
>  	/* Ensure conn will not be scheduled for reconnect */
> -	spin_lock_irq(&rds_conn_lock);
>  	hlist_del_init_rcu(&conn->c_hash_node);
>  	spin_unlock_irq(&rds_conn_lock);
>  	synchronize_rcu();

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927061448.167862-1-achender%40kernel.org