| Message ID | 20130116063645.GC18940@secunet.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello, On Wed, 16 Jan 2013, Steffen Klassert wrote: > The output route check was introduced with git commit 261663b0 > (ipv4: Don't use the cached pmtu informations for input routes) > during times when we cached the pmtu informations on the > inetpeer. Now the pmtu informations are back in the routes, > so this check is obsolete. It also had some unwanted side effects, > as reported by Timo Teras and Lukas Tribus. > > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> > --- > net/ipv4/route.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > index 844a9ef..6e4a89c 100644 > --- a/net/ipv4/route.c > +++ b/net/ipv4/route.c > @@ -1120,7 +1120,7 @@ static unsigned int ipv4_mtu(const struct dst_entry *dst) > if (!mtu || time_after_eq(jiffies, rt->dst.expires)) > mtu = dst_metric_raw(dst, RTAX_MTU); > > - if (mtu && rt_is_output_route(rt)) > + if (mtu) > return mtu; This fix looks good to me. But I see that we have another problem here. doc/ip-cref.tex in iproute2 claims that a locked MTU value has priority and PMTU should not be considered. IIRC, rt_pmtu is valid only for output routes but we do not check the lock flag here. What about such variant: static unsigned int ipv4_mtu(const struct dst_entry *dst) { const struct rtable *rt = (const struct rtable *) dst; unsigned int mtu = rt->rt_pmtu; if (unlikely(dst_metric_locked(dst, RTAX_MTU))) { mtu = dst_metric_raw(dst, RTAX_MTU); if (!mtu) { mtu = dst->dev->mtu; if (rt->rt_uses_gateway && mtu > 576) mtu = 576; } } else if (!mtu || time_after_eq(jiffies, rt->dst.expires)) { mtu = dst_metric_raw(dst, RTAX_MTU); if (!mtu) mtu = dst->dev->mtu; } if (mtu > IP_MAX_MTU) mtu = IP_MAX_MTU; return mtu; } I.e. order becomes: mtu lock non-zero => fixed fib_mtu mtu lock 0 => device MTU, up to 576 if via GW, ignore PMTU PMTU (output routes) mtu non-zero => fib_mtu device MTU Also, it seems __ip_rt_update_pmtu should start with such dst_metric_locked(dst, RTAX_MTU) check? > mtu = dst->dev->mtu; > -- > 1.7.9.5 Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 16, 2013 at 10:58:13AM +0200, Julian Anastasov wrote: > > This fix looks good to me. But I see that we > have another problem here. doc/ip-cref.tex in iproute2 > claims that a locked MTU value has priority and > PMTU should not be considered. > > IIRC, rt_pmtu is valid only for output routes but > we do not check the lock flag here. What about such > variant: > > static unsigned int ipv4_mtu(const struct dst_entry *dst) > { > const struct rtable *rt = (const struct rtable *) dst; > unsigned int mtu = rt->rt_pmtu; > > if (unlikely(dst_metric_locked(dst, RTAX_MTU))) { > mtu = dst_metric_raw(dst, RTAX_MTU); > if (!mtu) { > mtu = dst->dev->mtu; > if (rt->rt_uses_gateway && mtu > 576) > mtu = 576; > } > } else if (!mtu || time_after_eq(jiffies, rt->dst.expires)) { > mtu = dst_metric_raw(dst, RTAX_MTU); > if (!mtu) > mtu = dst->dev->mtu; > } > if (mtu > IP_MAX_MTU) > mtu = IP_MAX_MTU; > > return mtu; > } > > I.e. order becomes: > > mtu lock non-zero => fixed fib_mtu > mtu lock 0 => device MTU, up to 576 if via GW, ignore PMTU > PMTU (output routes) > mtu non-zero => fib_mtu > device MTU If this is the desired order, I'm fine with the above variant. > > Also, it seems __ip_rt_update_pmtu should start > with such dst_metric_locked(dst, RTAX_MTU) check? > Not absolutely sure what you want to do if the mtu is locked. But if you don't want to genetate a nh exception and don't update rt->rt_pmtu in this case, rt->rt_pmtu is never set on routes with locked mtu. We probably would not need to change ipv4_mtu() to the above variant. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, 16 Jan 2013, Steffen Klassert wrote: > On Wed, Jan 16, 2013 at 10:58:13AM +0200, Julian Anastasov wrote: > > > > This fix looks good to me. But I see that we > > have another problem here. doc/ip-cref.tex in iproute2 > > claims that a locked MTU value has priority and > > PMTU should not be considered. > > > > IIRC, rt_pmtu is valid only for output routes but > > we do not check the lock flag here. What about such > > variant: > > > > static unsigned int ipv4_mtu(const struct dst_entry *dst) > > { > > const struct rtable *rt = (const struct rtable *) dst; > > unsigned int mtu = rt->rt_pmtu; > > > > if (unlikely(dst_metric_locked(dst, RTAX_MTU))) { > > mtu = dst_metric_raw(dst, RTAX_MTU); > > if (!mtu) { > > mtu = dst->dev->mtu; > > if (rt->rt_uses_gateway && mtu > 576) > > mtu = 576; > > } > > } else if (!mtu || time_after_eq(jiffies, rt->dst.expires)) { > > mtu = dst_metric_raw(dst, RTAX_MTU); > > if (!mtu) > > mtu = dst->dev->mtu; > > } > > if (mtu > IP_MAX_MTU) > > mtu = IP_MAX_MTU; > > > > return mtu; > > } > > > > I.e. order becomes: > > > > mtu lock non-zero => fixed fib_mtu > > mtu lock 0 => device MTU, up to 576 if via GW, ignore PMTU > > PMTU (output routes) > > mtu non-zero => fib_mtu > > device MTU > > If this is the desired order, I'm fine with the above variant. > > > > > Also, it seems __ip_rt_update_pmtu should start > > with such dst_metric_locked(dst, RTAX_MTU) check? > > > > Not absolutely sure what you want to do if the mtu is locked. > But if you don't want to genetate a nh exception and don't update > rt->rt_pmtu in this case, rt->rt_pmtu is never set on routes with > locked mtu. We probably would not need to change ipv4_mtu() to the > above variant. Yes, we have to exit __ip_rt_update_pmtu if fib_mtu is locked. Then rt_pmtu should stay 0 and your variant of ipv4_mtu should be ok. I hope you can simply extend your patch with such additional check in __ip_rt_update_pmtu: if (dst_metric_locked(dst, RTAX_MTU)) return; Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 16, 2013 at 11:01:39PM +0200, Julian Anastasov wrote: > > On Wed, 16 Jan 2013, Steffen Klassert wrote: > > > > Not absolutely sure what you want to do if the mtu is locked. > > But if you don't want to genetate a nh exception and don't update > > rt->rt_pmtu in this case, rt->rt_pmtu is never set on routes with > > locked mtu. We probably would not need to change ipv4_mtu() to the > > above variant. > > Yes, we have to exit __ip_rt_update_pmtu if > fib_mtu is locked. Then rt_pmtu should stay 0 and > your variant of ipv4_mtu should be ok. I hope you > can simply extend your patch with such additional > check in __ip_rt_update_pmtu: > > if (dst_metric_locked(dst, RTAX_MTU)) > return; > That's what I had in mind. I'll do that with an additional patch, it's a another issue. Thanks for the input! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/route.c b/net/ipv4/route.c index 844a9ef..6e4a89c 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -1120,7 +1120,7 @@ static unsigned int ipv4_mtu(const struct dst_entry *dst) if (!mtu || time_after_eq(jiffies, rt->dst.expires)) mtu = dst_metric_raw(dst, RTAX_MTU); - if (mtu && rt_is_output_route(rt)) + if (mtu) return mtu; mtu = dst->dev->mtu;
The output route check was introduced with git commit 261663b0 (ipv4: Don't use the cached pmtu informations for input routes) during times when we cached the pmtu informations on the inetpeer. Now the pmtu informations are back in the routes, so this check is obsolete. It also had some unwanted side effects, as reported by Timo Teras and Lukas Tribus. Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> --- net/ipv4/route.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)