[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241118182004.5d38fac2@kernel.org>
Date: Mon, 18 Nov 2024 18:20:04 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Gilad Naaman <gnaaman@...venets.com>
Cc: "David S. Miller" <davem@...emloft.net>, David Ahern
<dsahern@...nel.org>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
<pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
netdev@...r.kernel.org
Subject: Re: [PATCH net-next] ipv6: Avoid invoking addrconf_verify_rtnl
unnecessarily
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@...venets.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
Powered by blists - more mailing lists