| Message ID | 20241111171607.127691-1-gnaaman@drivenets.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] ipv6: Avoid invoking addrconf_verify_rtnl unnecessarily | expand |
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 :(
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; }
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(-)