Re: [PATCH net-next v11 04/12] net/rds: make rds_destroy_pending() report a connection's own destroy

[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 commit message gets the history of RDS_DESTROY_PENDING and
  c_destroy_in_prog wrong, and it leaves out that mainline already had a…

--

net/rds: make rds_destroy_pending() report a connection's own destroy

This patch adds a per-connection c_destroy_in_prog flag. The flag is set
in rds_conn_destroy() before the unhash and synchronize_rcu(), and
rds_destroy_pending() tests it first. The patch also drops the unused
RDS_DESTROY_PENDING cp_flags bit and its dead test in rds_ib_is_unloading().

> The cp_flags bit that once served this purpose, RDS_DESTROY_PENDING
> from commit c90ecbfaf50d2 ("rds: Use atomic flag to track connections
> being destroyed"), was only ever set on the IB protocol-version path
> and lost its last set_bit in commit cdc306a5c9cd3 ("rds: make v3.1 as
> compat version"); it never covered the loopback path above, which
> commit c809195f5523 ("rds: clean up loopback rds_connections on netns
> deletion") added.

[Severity: Low]

Is this history accurate? Commit c90ecbfaf50d2 set the bit
unconditionally at the top of rds_conn_path_destroy(). That function
runs on every connection destroy, for every transport:

net/rds/connection.c:rds_conn_path_destroy() {
	struct rds_message *rm, *rtmp;

+	set_bit(RDS_DESTROY_PENDING, &cp->cp_flags);
    ...
}

The same commit also removed a c_destroy_in_prog field from struct
rds_connection. rds_conn_destroy() used to set that field in the same
place where this patch now adds the WRITE_ONCE():

net/rds/connection.c:rds_conn_destroy() {
    ...
-	conn->c_destroy_in_prog = 1;
 	/* Ensure conn will not be scheduled for reconnect */
 	spin_lock_irq(&rds_conn_lock);
    ...
}

Commit ebeeb1ad9b8a ("rds: tcp: use rds_destroy_pending() to synchronize
netns/module teardown and rds connection/workq management") then removed
that generic set_bit and the synchronize_rcu() from
rds_conn_path_destroy(). It moved the checks into rds_destroy_pending(),
defined as !check_net() || t_unloading(). That is the commit that took
away the per-connection coverage.

After ebeeb1ad9b8a, only the IB connect-complete version-mismatch
set_bit remained, and cdc306a5c9cd3 removed it.

Could the commit message say that this patch brings back a
c_destroy_in_prog field that mainline once had? It would also help to
say that the patch partly reverses the consolidation done in
ebeeb1ad9b8a, and that the global predicate stopped being enough once
c809195f5523 made rds_conn_exit() run rds_loop_net_exit() before
rds_loop_exit().

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