From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julian Anastasov Subject: Re: [PATCH net] ipv4: fix fnhe usage by non-cached routes Date: Thu, 3 May 2018 08:32:53 +0300 (EEST) Message-ID: References: <20180502064119.4552-1-ja@ssi.bg> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Cc: David Miller , netdev@vger.kernel.org, Martin KaFai Lau , kernel-team@fb.com, Xin Long To: David Ahern Return-path: Received: from ja.ssi.bg ([178.16.129.10]:34994 "EHLO ja.ssi.bg" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751622AbeECFdF (ORCPT ); Thu, 3 May 2018 01:33:05 -0400 In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: Hello, On Wed, 2 May 2018, David Ahern wrote: > On 5/2/18 12:41 AM, Julian Anastasov wrote: > > Allow some non-cached routes to use non-expired fnhe: > > > > 1. ip_del_fnhe: moved above and now called by find_exception. > > The 4.5+ commit deed49df7390 expires fnhe only when caching > > routes. Change that to: > > > > 1.1. use fnhe for non-cached local output routes, with the help > > from (2) > > > > 1.2. allow __mkroute_input to detect expired fnhe (outdated > > fnhe_gw, for example) when do_cache is false, eg. when itag!=0 > > for unicast destinations. > > > > 2. __mkroute_output: keep fi to allow local routes with orig_oif != 0 > > to use fnhe info even when the new route will not be cached into fnhe. > > After commit 839da4d98960 ("net: ipv4: set orig_oif based on fib > > result for local traffic") it means all local routes will be affected > > because they are not cached. This change is used to solve a PMTU > > problem with IPVS (and probably Netfilter DNAT) setups that redirect > > local clients from target local IP (local route to Virtual IP) > > to new remote IP target, eg. IPVS TUN real server. Loopback has > > 64K MTU and we need to create fnhe on the local route that will > > keep the reduced PMTU for the Virtual IP. Without this change > > fnhe_pmtu is updated from ICMP but never exposed to non-cached > > local routes. This includes routes with flowi4_oif!=0 for 4.6+ and > > with flowi4_oif=any for 4.14+). > > Can you add a test case to tools/testing/selftests/net/pmtu.sh to cover > this situation? Sure, I'll give it a try. > > @@ -1310,8 +1340,14 @@ static struct fib_nh_exception *find_exception(struct fib_nh *nh, __be32 daddr) > > > > for (fnhe = rcu_dereference(hash[hval].chain); fnhe; > > fnhe = rcu_dereference(fnhe->fnhe_next)) { > > - if (fnhe->fnhe_daddr == daddr) > > + if (fnhe->fnhe_daddr == daddr) { > > + if (fnhe->fnhe_expires && > > + time_after(jiffies, fnhe->fnhe_expires)) { > > + ip_del_fnhe(nh, daddr); > > I'm surprised this is done in the fast path vs gc time. (the existing > code does as well; your change is only moving the call to make the input > and output paths the same) > > > The change looks correct to me and all of my functional tests passed. > > Acked-by: David Ahern Thanks for the review! Regards