Re: [PATCH net-next v8 08/13] net/rds: wait for connections to be freed on transport unload

[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] This patch fixes two real, pre-existing unload bugs in the IB
  transport, but carries no Fixes: tag.

--

net/rds: wait for connections to be freed on transport unload

This patch adds a per-transport t_conn_count of live connections. The IB,
TCP and loop exit paths now wait for that count to reach zero before
unloading. For IB, the wait also calls rds_ib_destroy_nodev_conns() on
every poll, and rds_ib_exit() flushes rds_wq again once the connections
are gone.

> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index d9879b6129e73..7b1f611c0a3e5 100644
> --- a/net/rds/ib.c
> +++ b/net/rds/ib.c
> @@ -540,7 +540,24 @@ void rds_ib_exit(void)
>  	rds_info_deregister_func(RDS6_INFO_IB_CONNECTIONS, rds6_ib_ic_info);
>  #endif
>  	rds_ib_unregister_client();
> +
> +	/* rds_ib_dev_shutdown() only dropped the connections still
> +	 * attached to a device; each moves itself to ib_nodev_conns
> +	 * from its shutdown work.  Destroy what is there now and keep
> +	 * sweeping the list while the wait sees connections outstanding,
> +	 * so a late arrival is destroyed rather than waited on forever.
> +	 */
>  	rds_ib_destroy_nodev_conns();
> +	rds_conn_wait_conns_freed(&rds_ib_transport,
> +				  rds_ib_destroy_nodev_conns);
> +
> +	/* Tearing down the last connection may have dropped the final
> +	 * reference on a device, deferring rds_ib_dev_free() to rds_wq.
> +	 * Drain it before the module goes away; it queues nothing
> +	 * further on rds_wq.
> +	 */
> +	flush_workqueue(rds_wq);
> +

[Severity: Low]

The resweep and the second flush_workqueue(rds_wq) seem to fix two IB
unload bugs that already exist in the tree before this patch. Should this
patch have a Fixes: tag?

The first bug starts in rds_ib_dev_shutdown(), which queues cp_down_w on
the per-path cp_wq:

	list_for_each_entry(ic, &rds_ibdev->conn_list, ib_node)
		rds_conn_path_drop(&ic->conn->c_path[0], true);

Since commit 4716af3897e9 ("net/rds: Give each connection path its own
workqueue"), cp_wq is a private ordered workqueue instead of rds_wq. So
the flush_workqueue(rds_wq) in rds_ib_unregister_client() does not wait
for those shutdown works.

If rds_ib_remove_conn() moves a connection to ib_nodev_conns after the
single rds_ib_destroy_nodev_conns() sweep, nothing destroys it. It
outlives the module and still points at rds_ib_transport.

The second bug follows from that. rds_ib_remove_conn() then calls
rds_ib_dev_put(), which does this when it drops the last device
reference:

	if (refcount_dec_and_test(&rds_ibdev->refcount))
		queue_work(rds_wq, &rds_ibdev->free_work);

That queues rds_ib_dev_free() after the only flush of rds_wq. It could
then run after the module text is gone.

The commit message already calls the first bug "an existing hole". Of
the second it says "nothing flushed rds_wq again after
rds_ib_unregister_client() before". Other fixes in this series carry a
Fixes: tag. Would a Fixes: 4716af3897e9 tag fit here as well?

The opening paragraph of the commit message also says:

  At this point in the series the initial reference is the only one
  and the wait returns at once; the guarantee becomes load-bearing
  with those patches.

Is that accurate for IB? A connection that reaches ib_nodev_conns late
keeps t_conn_count above zero. The loop in rds_conn_wait_conns_freed()
then has to call the resweep to destroy it, so the patch already changes
behavior at this point in the series.

Could the opening paragraph be reworded so backport readers don't take
the patch as a no-op?

It may also be worth splitting out a small standalone fix that doesn't
depend on the kref/t_conn_count infrastructure. One option is to flush
the per-connection shutdown works, or loop until the device lists and
ib_nodev_conns are empty, and then flush rds_wq.

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