Re: [PATCH net-next] ipv6: Avoid invoking addrconf_verify_rtnl unnecessarily

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 11 Nov 2024 17:16:07 +0000 Gilad Naaman wrote:
> Do not invoke costly `addrconf_verify_rtnl` if the added address
> wouldn't need it, or affect the delayed_work timer.
> 
> Signed-off-by: Gilad Naaman <gnaaman@drivenets.com>
> ---
> addrconf_verify_rtnl() deals with either management/temporary (Security)
> addresses, or with addresses that have some kind of lifetime.
> 
> This patches makes it so that ops on addresses that are permanent would
> not trigger this function.
> 
> This does wonders in our use-case of modifying a lot of (~24K) static
> addresses, since it turns the addition or deletion of addresses to an
> amortized O(1), instead of O(N).
> 
> Modification of management addresses or "non-permanent" (not sure what
> is the correct jargon) addresses are still slow.
> 
> We can improve those in the future, depending on the case:
> 
> If the function is called only to handle cases where the scheduled work should
> be called earlier, I think this would be better served by saving the next
> expiration and equating to it, since it would save iteration of the
> table.
> 
> If some upkeep *is* needed (e.g. creating a temporary address)
> I Think it is possible in theory make these modifications faster as
> well, if we only iterate `idev->if_addrs` as a response for a
> modification, since it doesn't seem to me like there are any
> cross-device effects.
> 
> I opted to keep this patch simple and not solve this, on the assumption
> that there aren't many users that need this scale.

I'd rather you put too much in the commit message than too little.
Move more (all?) of this above the --- please.

> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index d0a99710d65d..12fdabb1deba 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -3072,8 +3072,7 @@ static int inet6_addr_add(struct net *net, int ifindex,
>  		 */
>  		if (!(ifp->flags & (IFA_F_OPTIMISTIC | IFA_F_NODAD)))
>  			ipv6_ifa_notify(0, ifp);
> -		/*
> -		 * Note that section 3.1 of RFC 4429 indicates
> +		/* Note that section 3.1 of RFC 4429 indicates
>  		 * that the Optimistic flag should not be set for
>  		 * manually configured addresses
>  		 */
> @@ -3082,7 +3081,15 @@ static int inet6_addr_add(struct net *net, int ifindex,
>  			manage_tempaddrs(idev, ifp, cfg->valid_lft,
>  					 cfg->preferred_lft, true, jiffies);
>  		in6_ifa_put(ifp);
> -		addrconf_verify_rtnl(net);
> +
> +		/* Verify only if this address is perishable or has temporary
> +		 * offshoots, as this function is too expansive.
> +		 */
> +		if ((cfg->ifa_flags & IFA_F_MANAGETEMPADDR) ||
> +		    !(cfg->ifa_flags & IFA_F_PERMANENT) ||
> +		    cfg->preferred_lft != INFINITY_LIFE_TIME)

Would be very useful for readability to extract the condition into 
some helper. If addrconf_verify_rtnl() also used that same helper
reviewing this patch would be trivial..

> +			addrconf_verify_rtnl(net);
> +
>  		return 0;
>  	} else if (cfg->ifa_flags & IFA_F_MCAUTOJOIN) {
>  		ipv6_mc_config(net->ipv6.mc_autojoin_sk, false,
> @@ -3099,6 +3106,7 @@ static int inet6_addr_del(struct net *net, int ifindex, u32 ifa_flags,
>  	struct inet6_ifaddr *ifp;
>  	struct inet6_dev *idev;
>  	struct net_device *dev;
> +	int is_mgmt_tmp;

The flag naming isn't super clear, but it's manageD, not manageMENT,
as in "managed by the kernel".

>  
>  	if (plen > 128) {
>  		NL_SET_ERR_MSG_MOD(extack, "Invalid prefix length");

I think this change will need to wait until after the merge window
(Dec 2nd), sorry nobody reviewed it in time for 6.13 :(
-- 
pw-bot: defer