Re: [PATCH v3 net-next 2/2] udp_tunnel: use static call for GRO hooks when possible

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Paolo Abeni wrote:
> On 3/11/25 3:51 AM, Willem de Bruijn wrote:
> > Paolo Abeni wrote:
> [...]
> >> +void udp_tunnel_update_gro_rcv(struct sock *sk, bool add)
> >> +{
> >> +	struct udp_tunnel_type_entry *cur = NULL, *avail = NULL;
> >> +	struct udp_sock *up = udp_sk(sk);
> >> +	int i, old_gro_type_nr;
> >> +
> >> +	if (!up->gro_receive)
> >> +		return;
> >> +
> >> +	mutex_lock(&udp_tunnel_gro_type_lock);
> >> +	for (i = 0; i < UDP_MAX_TUNNEL_TYPES; i++) {
> >> +		if (!refcount_read(&udp_tunnel_gro_types[i].count))
> > 
> > Optionally: && !avail, to fill the list from the front. And on delete
> > avoid gaps. For instance, like __fanout_link/__fanout_unlink.
> > 
> > Can stop sooner then. And list length is then implicit as i once found
> > the first [i].count == zero.
> > 
> > Then again, this list is always short. I can imagine you prefer to
> > leave as is.
> 
> I avoided optimizations for this slow path, to keep the code simpler.
> Thinking again about it, avoiding gaps will simplify/cleanup the code a
> bit (no need to lookup the enabled tunnel on deletion and to use `avail`
> on addition), so I'll do it.
> 
> Note that I'll still need to explicitly track the number of enabled
> tunnel types, as an easy way to disable the static call in the unlikely
> udp_tunnel_gro_type_nr == UDP_MAX_TUNNEL_TYPES event.
> 
> [...]
> >> +	if (udp_tunnel_gro_type_nr == 1) {
> >> +		for (i = 0; i < UDP_MAX_TUNNEL_TYPES; i++) {
> >> +			cur = &udp_tunnel_gro_types[i];
> >> +			if (refcount_read(&cur->count)) {
> >> +				static_call_update(udp_tunnel_gro_rcv,
> >> +						   cur->gro_receive);
> >> +				static_branch_enable(&udp_tunnel_static_call);
> >> +			}
> >> +		}
> >> +	} else if (old_gro_type_nr == 1) {
> >> +		static_branch_disable(&udp_tunnel_static_call);
> >> +		static_call_update(udp_tunnel_gro_rcv, dummy_gro_rcv);
> > 
> > These operations must not be reorderd, or dummy_gro_rcv might get hit.
> > 
> > If static calls are not configured, the last call is just a
> > WRITE_ONCE. Similar for static_branch_disable if !CONFIG_JUMP_LABEL.
> 
> When both construct are disabled, I think a wmb/rmb pair would be needed
> to ensure no reordering, and that in turn looks overkill. I think it
> would be better just drop the WARN_ONCE in dummy_gro_rcv().

SGTM, thanks.