Re: [PATCH net 14/15] ipvs: wait grace period before reusing destinations

[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 for "ipvs: wait grace period before reusing
  destinations" describes the change against an earlier revision of the…

--

ipvs: wait grace period before reusing destinations

This patch adds a conditional RCU grace-period wait in ip_vs_add_dest()
right after a destination is taken from dest_trash, before its estimator
is restarted. Trash insertions are batched so that one rcu_state cookie
covers each removal. A need_gp flag makes ip_vs_rs_hash() wait before it
rehashes a destination that was unhashed during an edit.

> When a destination is retrieved from dest_trash, ip_vs_add_dest() currently
> restarts its estimator before waiting for the grace period.

[ ... ]

> ip_vs_start_estimator() rewrites the node on its temporary list on success,
> or initializes it on failure, before the current wait. A reader can then
> follow the changed link or stop traversing the chain early.

[Severity: Low]
Does this wording describe the change against an earlier revision of the
patch rather than the tree it applies to?

In the parent tree, the trash-reuse path in ip_vs_add_dest() has no
grace-period wait. ip_vs_trash_get_dest() is followed directly by
ip_vs_start_estimator() and __ip_vs_update_dest(). There is also no
rcu_state, need_gp or cond_synchronize_rcu() anywhere under
net/netfilter/ipvs/ in the parent tree.

So "currently restarts its estimator before waiting" and "before the
current wait" point to a wait that does not exist in the baseline. The
patch adds the wait rather than moving it.

> The grace-period cookie is also saved when a live destination is unhashed
> for an edit and consumed when it is rehashed. Describe rcu_state as the
> cookie for an unlink, covering both trash and rehash cases.

The sentence "Describe rcu_state as the cookie for an unlink" reads like a
note between patch versions about rewording the comment on the new
struct ip_vs_dest::rcu_state field. It does not describe a change against
the baseline.

The patch is tagged for stable. Could the message say plainly that before
this patch n_list, d_list and est->list were reused with no grace period?
It could then say that the patch adds cond_synchronize_rcu() right after
the dest is taken from the trash, and a need_gp wait in ip_vs_rs_hash()
for the edit/rehash case.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113011447.2357709-2-pablo%40netfilter.org