From: Timo Teras <timo.teras@iki•fi>
To: Julian Anastasov <ja@ssi•bg>
Cc: netdev@vger•kernel.org
Subject: Re: [PATCH net-next 2/3] ipv4: rate limit updating of next hop exceptions with same pmtu
Date: Wed, 29 May 2013 08:07:25 +0300 [thread overview]
Message-ID: <20130529080725.5de3477b@vostro> (raw)
In-Reply-To: <alpine.LFD.2.00.1305282245340.1713@ja.ssi.bg>
On Wed, 29 May 2013 00:04:47 +0300 (EEST)
Julian Anastasov <ja@ssi•bg> wrote:
>
> Hello,
>
> On Tue, 28 May 2013, Timo Teras wrote:
>
> > > > --- a/net/ipv4/route.c
> > > > +++ b/net/ipv4/route.c
> > > > @@ -947,6 +947,10 @@ static void __ip_rt_update_pmtu(struct
> > > > rtable *rt, struct flowi4 *fl4, u32 mtu) if (mtu <
> > > > ip_rt_min_pmtu) mtu = ip_rt_min_pmtu;
> > > >
> > > > + if (rt->rt_pmtu == mtu &&
> > > > + time_before(jiffies, dst->expires -
> > > > ip_rt_mtu_expires / 2))
> > > > + return;
> > > > +
> > >
> > > Can we also add logic in this patch in
> > > update_or_create_fnhe, so that we avoid invalidation for cached
> > > routes when only pmtu expiration is updated (same pmtu), i.e.:
> > >
> > > + if (gw || pmtu != fnhe->fnhe_pmtu) {
> > > + /* Exception created; mark the cached routes for
> > > the nexthop
> > > + ...
> > > + }
> > >
> > > BTW, I now see that previous patch should
> > > call for_each_possible_cpu for the both cases, not
> > > only when fnhe is created but also on update:
> >
> > Why would this be needed?
> >
> > The idea is that if there is no next hop exception, everyone is
> > using the next hop's dsts. If there is a next hop exception hashed,
> > they will be using those routes in the exception entry.
> >
> > When the exception is created, we need to invalidate the nexthop's
> > routes, to force relookup of these dst's if should go to the
> > exception. Basically it flushes the nexthop's 'default' dst.
> >
> > But if we have a valid exception, and we are just updating it.
> > Everyone is already using the right dst. The
> > update_or_create_fnhe() updates all exception's dst's that are
> > effected. Since the set of hosts to which that exception applies is
> > the same, I don't see why we would need to invalidate the nexthop's
> > 'default' dst.
>
> Agreed for the NH route. But there is another case:
> when fnhe exists for fnhe_pmtu!=0 and fnhe_gw=0 and the cached
> FNHE route has just rt_pmtu!=0 and rt_uses_gateway=0. In the
> event of redirect we should invalidate fnhe_rth.
> Users should come again to get the new gw from the
> same fnhe. As you note, nh_pcpu_rth_output does not need
> to be invalidated, it was already invalidated when
> fnhe was created.
>
> So, my idea is something like this. If cached
> route detects change in gw on redirect, it calls
> update_or_create_fnhe but FNHE can already know this gw,
> we should invalidate the fnhe_rth only if gw changes.
> As caller has some stale cache route, it should be
> invalidated as before, I assume 'kill_route' is properly
> provided.
>
> if (fnhe) {
> if (fnhe->fnhe_gw != gw && gw) {
> rt =
> rcu_dereference_protected(fnhe->fnhe_rth, 1); if (rt)
> rt->dst.obsolete = DST_OBSOLETE_KILL;
> fnhe->fnhe_gw = gw;
> }
> ...
> }
> ...
> }
This is already done by the caller of update_or_create_fnhe for the
case of gw change. It might be worth to put all this to the same place,
but would be worth of separate patch.
> Also, I don't know the XFRM code well but
> xfrm_bundle_ok() calls dst_check. Now ipv4_dst_check
> will not give any indication for recent change in
> rt_pmtu. I hope this is not a problem because I see
> some comparisons with cached pmtus. I.e. the main
> question is: are there any dst_check and ->check users
> that needs to know for changes in rt_pmtu or all
> of them just use dst_mtu().
In XFRM case, xfrm_bundle_ok() will propagate the pmtu by calling
dst_mtu() for each target. All caching users need to use dst_mtu() to
check it, so the route.c code is correct - this is how it worked before
my patches too.
- Timo
next prev parent reply other threads:[~2013-05-29 5:05 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-27 11:16 [PATCH net-next 0/6] ipv4 fixes for dmvpn Timo Teräs
2013-05-27 11:16 ` [PATCH net-next 1/6] net: inform NETDEV_CHANGE callbacks which flags were changed Timo Teräs
2013-05-27 11:18 ` Jiri Pirko
2013-05-27 11:16 ` [PATCH net-next 2/6] arp: flush arp cache on IFF_NOARP change Timo Teräs
2013-05-27 11:16 ` [PATCH net-next 3/6] ipv4: properly refresh rtable entries on pmtu/redirect events Timo Teräs
2013-05-27 11:16 ` [PATCH net-next 4/6] ipv4: rate limit updating of next hop exceptions with same pmtu Timo Teräs
2013-05-27 11:16 ` [PATCH net-next 5/6] ipv4: use separate genid for next hop exceptions Timo Teräs
2013-05-27 11:16 ` [PATCH RFC net-next 6/6] ipv4: use next hop exceptions also for input routes Timo Teräs
2013-05-28 6:33 ` [PATCH net-next 0/6] ipv4 fixes for dmvpn Timo Teras
2013-05-28 6:38 ` David Miller
2013-05-28 6:46 ` [PATCH net-next 1/3] ipv4: properly refresh rtable entries on pmtu/redirect events Timo Teräs
2013-05-28 6:46 ` [PATCH net-next 2/3] ipv4: rate limit updating of next hop exceptions with same pmtu Timo Teräs
2013-05-28 8:45 ` Julian Anastasov
2013-05-28 10:07 ` Timo Teras
2013-05-28 21:04 ` Julian Anastasov
2013-05-29 5:07 ` Timo Teras [this message]
2013-05-29 22:49 ` Julian Anastasov
2013-06-03 7:10 ` David Miller
2013-05-28 6:46 ` [PATCH net-next 3/3] ipv4: use separate genid for next hop exceptions Timo Teräs
2013-06-03 7:10 ` David Miller
2013-05-28 8:25 ` [PATCH net-next 1/3] ipv4: properly refresh rtable entries on pmtu/redirect events Julian Anastasov
2013-05-28 8:53 ` Timo Teras
2013-05-28 19:44 ` Julian Anastasov
2013-06-03 7:09 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130529080725.5de3477b@vostro \
--to=timo.teras@iki$(echo .)fi \
--cc=ja@ssi$(echo .)bg \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox