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

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

[Gilad Naaman]

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.
---
 net/ipv6/addrconf.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

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)
+			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;
 
 	if (plen > 128) {
 		NL_SET_ERR_MSG_MOD(extack, "Invalid prefix length");
@@ -3124,12 +3132,17 @@ static int inet6_addr_del(struct net *net, int ifindex, u32 ifa_flags,
 			in6_ifa_hold(ifp);
 			read_unlock_bh(&idev->lock);
 
-			if (!(ifp->flags & IFA_F_TEMPORARY) &&
-			    (ifa_flags & IFA_F_MANAGETEMPADDR))
+			is_mgmt_tmp = (!(ifp->flags & IFA_F_TEMPORARY) &&
+				       (ifa_flags & IFA_F_MANAGETEMPADDR));
+
+			if (is_mgmt_tmp)
 				manage_tempaddrs(idev, ifp, 0, 0, false,
 						 jiffies);
 			ipv6_del_addr(ifp);
-			addrconf_verify_rtnl(net);
+
+			if (is_mgmt_tmp)
+				addrconf_verify_rtnl(net);
+
 			if (ipv6_addr_is_multicast(pfx)) {
 				ipv6_mc_config(net->ipv6.mc_autojoin_sk,
 					       false, pfx, dev->ifindex);
@@ -4962,7 +4975,10 @@ static int inet6_addr_modify(struct net *net, struct inet6_ifaddr *ifp,
 				 jiffies);
 	}
 
-	addrconf_verify_rtnl(net);
+	if (was_managetempaddr ||
+	    !(cfg->ifa_flags & IFA_F_PERMANENT) ||
+	    cfg->preferred_lft != INFINITY_LIFE_TIME)
+		addrconf_verify_rtnl(net);
 
 	return 0;
 }
-- 
2.34.1

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

[Jakub Kicinski]

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

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

[Gilad Naaman]

> 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.

No problem, will do :)
I thought that most of the text is a bit speculative and describes
changes I didn't end up making, but I see the value in including it.

> 
> > 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..

Good idea.

> > +			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".

Oh, whoopse, MANAGEDTEMP tricked me.
Thank you.

> >  
> >  	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 :(

No problem, thank you for time!
I'll resend a polished patch next week.