Re: [PATCH net-next v6 06/12] net/rds: wait for connections to be freed on transport unload
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-26 09:48:40
Also in:
linux-rdma
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider. Critical: 0 · High: 0 · Medium: 1 · Low: 0 If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [Medium] At this commit only, the combination of `list_splice_init()` in net/rds/ib_rdma.c:rds_ib_destroy_nodev_conns() (line 168) and the… -- net/rds: wait for connections to be freed on transport unload This adds a per-transport live connection count (t_conn_count) that is incremented when a connection is published in __rds_conn_create() and decremented at the end of rds_conn_destroy_fini(). The transport exit paths then poll that count until it reaches zero, with an optional resweep callback used by the IB transport for connections that are still detaching from a device. [ ... ]
quoted hunk ↗ jump to hunk
diff --git a/net/rds/ib.c b/net/rds/ib.c index 9fe3b9951bd32..3fc2de9d19d55 100644 --- a/net/rds/ib.c +++ b/net/rds/ib.c@@ -537,7 +537,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);
[Severity: Medium]
Passing rds_ib_destroy_nodev_conns as the resweep means the list is
emptied again on every 100 ms poll for the whole duration of the unload
wait, rather than just once. Combined with the list_splice_init()
change below, can that widen a window where rds_ib_add_conn() takes
BUG()?
At this commit rds_ib_add_conn() still asserts that the list head is
non-empty:
net/rds/ib_rdma.c:rds_ib_add_conn() {
/* conn was previously on the nodev_conns_list */
spin_lock_irq(&ib_nodev_conns_lock);
BUG_ON(list_empty(&ib_nodev_conns));
BUG_ON(list_empty(&ic->ib_node));
list_del(&ic->ib_node);
...
}
A connect that was already in flight when the unload started can reach
that assert while the sweep still holds the claimed nodes on its
stack-local tmp_list:
rds_ib_cm_handle_event() (ADDR_RESOLVED/ROUTE_RESOLVED)
rds_ib_cm_initiate_connect()
rds_ib_setup_qp()
rds_ib_add_conn() <- list_empty(&ib_nodev_conns) == true
The sweep itself calls rds_conn_destroy(), which unhashes the connection
and then blocks in synchronize_rcu() before driving the path to
RDS_CONN_DISCONNECTING, so there is real time spent with the head empty
and the connections still alive.
quoted hunk ↗ jump to hunk
+ + /* 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); + rds_ib_sysctl_exit(); rds_ib_recv_exit(); rds_trans_unregister(&rds_ib_transport);diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c index db7e92e7bd29f..a9b27f06cbfcf 100644 --- a/net/rds/ib_rdma.c +++ b/net/rds/ib_rdma.c@@ -165,7 +165,7 @@ void rds_ib_destroy_nodev_conns(void) /* avoid calling conn_destroy with irqs off */ spin_lock_irq(&ib_nodev_conns_lock); - list_splice(&ib_nodev_conns, &tmp_list); + list_splice_init(&ib_nodev_conns, &tmp_list); spin_unlock_irq(&ib_nodev_conns_lock); list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
[Severity: Medium] With the old plain list_splice(), ib_nodev_conns was left stale and non-empty, so the BUG_ON(list_empty(&ib_nodev_conns)) in rds_ib_add_conn() could not fire during a sweep. Does re-initializing the head here make that assert reachable for an ic whose node the sweep has already moved onto tmp_list? Worth noting: the next patch in this series, "net/rds: unlink transport nodes before a possibly deferred connection free", removes both BUG_ONs from rds_ib_add_conn() and replaces the splice with a per-node i_ib_node_detached claim, so the end state of the series does not have this window - it exists only at this intermediate commit, which makes it a bisection hazard. Could the assert removal be folded in here, or the list_splice_init() change moved to the later patch, so no commit in the series has the reachable BUG()? [ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922085410.391323-1-achender%40kernel.org