Re: [PATCH net-next v2 8/9] net/rds: drop rds_conn_count in favor of t_conn_count

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] Changelog inaccuracy in "net/rds: drop rds_conn_count in favor of
  t_conn_count": the commit message states that removing the…

--

net/rds: drop rds_conn_count in favor of t_conn_count

This removes the global rds_conn_count now that each transport tracks its
own connections in t_conn_count, and seeds the per-path workqueue names
from t_conn_count instead.  The rds_conn_lock round-trip that existed only
to decrement rds_conn_count is dropped from rds_conn_destroy_fini().

> Removing the counter also removes the rds_conn_lock round-trip that
> rds_conn_destroy_fini() took solely to decrement it, leaving the
> free path lock-free.

[Severity: Low]
Is the free path really lock-free after this change?

rds_conn_destroy_fini() still calls rds_conn_path_free() for every path,
and that ends up in destroy_workqueue():

net/rds/connection.c:rds_conn_path_free() {
	if (cp->cp_wq != rds_wq) {
		destroy_workqueue(cp->cp_wq);
		cp->cp_wq = NULL;
	}
	...
}

destroy_workqueue() drains the queue and takes wq_pool_mutex, so it can
sleep.  The tail of rds_conn_destroy_fini() also takes the wait-queue
spinlock via wake_up_all(&rds_conn_freed_waitq).  What this patch removes
is the last rds_conn_lock acquisition, not all locking.

The concern with the current wording is that a later reader or a
backporter could take "lock-free" to mean the final rds_conn_put() may be
dropped from atomic, IRQ or softirq context, which this path cannot
tolerate because destroy_workqueue() may sleep.

The last patch of this series, "net/rds: hold a connection reference from
struct rds_incoming", states the opposite property:

  "The final rds_conn_put() runs the free path, which destroys the
   per-path workqueues and therefore may sleep, so the last reference has
   to be dropped from process context"

Would something like "removes the last rds_conn_lock round-trip from the
free path" be more accurate here?

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 1ec141d6faa38..11813f93961d1 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -656,7 +654,6 @@ static void rds_conn_destroy_fini(struct kref *kref)
>  						   c_refcount);
>  	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
>  	struct rds_transport *trans = conn->c_trans;
> -	unsigned long flags;
>  	int i;
>  
>  	for (i = 0; i < npaths; i++)
> @@ -665,10 +662,6 @@ static void rds_conn_destroy_fini(struct kref *kref)
>  	kfree(conn->c_path);
>  	kmem_cache_free(rds_conn_slab, conn);
>  
> -	spin_lock_irqsave(&rds_conn_lock, flags);
> -	rds_conn_count--;
> -	spin_unlock_irqrestore(&rds_conn_lock, flags);
> -
>  	/* only after everything the transport module owns has been
>  	 * freed above may its unload proceed
>  	 */

The code change itself looks fine; only the changelog wording seems to
overstate the result.

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