Re: IP fragmentation broken in 3.6-rc ?

Re: IP fragmentation broken in 3.6-rc ?

[Sylvain Munaut]

Hi,

I'm starting to wonder if it's not related to
46517008e1168dc926cf2c47d529efc07eca85c0 "ipv4: Kill
ip_rt_frag_needed()."

Cheers,

    Sylvain

Re: IP fragmentation broken in 3.6-rc ?

[Eric Dumazet]

On Tue, 2012-08-21 at 15:05 +0200, Sylvain Munaut wrote:
> Hi,
> 
> I'm starting to wonder if it's not related to
> 46517008e1168dc926cf2c47d529efc07eca85c0 "ipv4: Kill
> ip_rt_frag_needed()."
> 
> Cheers,
> 
>     Sylvain

Hmm, most probably problem came with commit
ceb3320610d6f15ff20dd4c042b36473d77de76f
(ipv4: Kill routes during PMTU/redirect updates.)

Re: IP fragmentation broken in 3.6-rc ?

[Eric Dumazet]

On Tue, 2012-08-21 at 15:22 +0200, Eric Dumazet wrote:
> On Tue, 2012-08-21 at 15:05 +0200, Sylvain Munaut wrote:
> > Hi,
> > 
> > I'm starting to wonder if it's not related to
> > 46517008e1168dc926cf2c47d529efc07eca85c0 "ipv4: Kill
> > ip_rt_frag_needed()."
> > 
> > Cheers,
> > 
> >     Sylvain
> 
> Hmm, most probably problem came with commit
> ceb3320610d6f15ff20dd4c042b36473d77de76f
> (ipv4: Kill routes during PMTU/redirect updates.)
> 
> 

Following patch should help :

diff --git a/include/net/dst.h b/include/net/dst.h
index 621e351..a04aa37 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -435,7 +435,7 @@ static inline void dst_set_expires(struct dst_entry *dst, int timeout)
 	if (expires == 0)
 		expires = 1;
 
-	if (dst->expires == 0 || time_before(expires, dst->expires))
+	if (dst->expires == 0 || time_after(expires, dst->expires))
 		dst->expires = expires;
 }
 

Re: IP fragmentation broken in 3.6-rc ?

[Julian Anastasov]

	Hello,

On Tue, 21 Aug 2012, Eric Dumazet wrote:

> Following patch should help :
> 
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 621e351..a04aa37 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -435,7 +435,7 @@ static inline void dst_set_expires(struct dst_entry *dst, int timeout)
>  	if (expires == 0)
>  		expires = 1;
>  

	In theory, restart of PMTUD should not lead to
fatal problems, we will get new MTUs. But with such
change should be better, not much, because all MTU
events will come at same time, later timer will expire
and we will get again events from routers. The gain
will be an increased period (with milliseconds to seconds)
between PMTUD restarts. Compared to the 600-second timer,
this should be gain below 1% in reduced traffic for PMTUD.
Before now we started timer from first router, now we
will start/update timer period after event from last router.

	But ipv4_link_failure and ip6_link_failure want to stop
this timer by setting it to NOW (0). May be we have to add
also a !timeout check here or to leave the code as before?

> -	if (dst->expires == 0 || time_before(expires, dst->expires))
> +	if (dst->expires == 0 || time_after(expires, dst->expires))
>  		dst->expires = expires;
>  }

	The original problem should be somewhere else, I think.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: IP fragmentation broken in 3.6-rc ?

[Eric Dumazet]

On Tue, 2012-08-21 at 19:34 +0300, Julian Anastasov wrote:
> 	Hello,
> 
> On Tue, 21 Aug 2012, Eric Dumazet wrote:
> 
> > Following patch should help :
> > 
> > diff --git a/include/net/dst.h b/include/net/dst.h
> > index 621e351..a04aa37 100644
> > --- a/include/net/dst.h
> > +++ b/include/net/dst.h
> > @@ -435,7 +435,7 @@ static inline void dst_set_expires(struct dst_entry *dst, int timeout)
> >  	if (expires == 0)
> >  		expires = 1;
> >  
> 
> 	In theory, restart of PMTUD should not lead to
> fatal problems, we will get new MTUs. But with such
> change should be better, not much, because all MTU
> events will come at same time, later timer will expire
> and we will get again events from routers. The gain
> will be an increased period (with milliseconds to seconds)
> between PMTUD restarts. Compared to the 600-second timer,
> this should be gain below 1% in reduced traffic for PMTUD.
> Before now we started timer from first router, now we
> will start/update timer period after event from last router.
> 

Sorry I dont really understand what you mean


> 	But ipv4_link_failure and ip6_link_failure want to stop
> this timer by setting it to NOW (0). May be we have to add
> also a !timeout check here or to leave the code as before?
> 
> > -	if (dst->expires == 0 || time_before(expires, dst->expires))
> > +	if (dst->expires == 0 || time_after(expires, dst->expires))
> >  		dst->expires = expires;
> >  }
> 
> 	The original problem should be somewhere else, I think.

This patch fixed the problem for me.

My first patch was :

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index e4ba974..9858714 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -956,6 +956,7 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
 		dst->obsolete = DST_OBSOLETE_KILL;
 	} else {
 		rt->rt_pmtu = mtu;
+		rt->dst.expires = 0;
 		dst_set_expires(&rt->dst, ip_rt_mtu_expires);
 	}
 }

Re: IP fragmentation broken in 3.6-rc ?

[Julian Anastasov]

	Hello,

On Tue, 21 Aug 2012, Eric Dumazet wrote:

> On Tue, 2012-08-21 at 19:34 +0300, Julian Anastasov wrote:
> > 	Hello,
> > 
> > On Tue, 21 Aug 2012, Eric Dumazet wrote:
> > 
> > > Following patch should help :
> > > 
> > > diff --git a/include/net/dst.h b/include/net/dst.h
> > > index 621e351..a04aa37 100644
> > > --- a/include/net/dst.h
> > > +++ b/include/net/dst.h
> > > @@ -435,7 +435,7 @@ static inline void dst_set_expires(struct dst_entry *dst, int timeout)
> > >  	if (expires == 0)
> > >  		expires = 1;
> > >  
> > 
> > 	In theory, restart of PMTUD should not lead to
> > fatal problems, we will get new MTUs. But with such
> > change should be better, not much, because all MTU
> > events will come at same time, later timer will expire
> > and we will get again events from routers. The gain
> > will be an increased period (with milliseconds to seconds)
> > between PMTUD restarts. Compared to the 600-second timer,
> > this should be gain below 1% in reduced traffic for PMTUD.
> > Before now we started timer from first router, now we
> > will start/update timer period after event from last router.
> > 
> 
> Sorry I dont really understand what you mean

	This timer is used only for PMTU, right?
RFC 1191 6.3. Purging stale PMTU information.

> > 	But ipv4_link_failure and ip6_link_failure want to stop
> > this timer by setting it to NOW (0). May be we have to add
> > also a !timeout check here or to leave the code as before?
> > 
> > > -	if (dst->expires == 0 || time_before(expires, dst->expires))
> > > +	if (dst->expires == 0 || time_after(expires, dst->expires))
> > >  		dst->expires = expires;
> > >  }
> > 
> > 	The original problem should be somewhere else, I think.
> 
> This patch fixed the problem for me.

	OK, then I'll wait for the patch description
to understand what this change actually does. For me,
it just updates the timer on every MTU event, while
before the change, PMTU timer was started only once and
during the 600-second period it was not updated and also
there was the ability to invalidate the PMTU on link failure
by setting timer to jiffies, i.e. to stop this period
and to start new one with default MTU and possibly
new discovery procedure.

> My first patch was :
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index e4ba974..9858714 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -956,6 +956,7 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
>  		dst->obsolete = DST_OBSOLETE_KILL;
>  	} else {
>  		rt->rt_pmtu = mtu;
> +		rt->dst.expires = 0;
>  		dst_set_expires(&rt->dst, ip_rt_mtu_expires);

	This is better, does not break ipv4_link_failure.
There is a little race some ipv4_mtu() user to see
rt_pmtu != 0 and dst.expires = 0 and to fail in time_after_eq
test. May be that is why dst.expires is never set to 0.
But I still don't understand what both changes fix.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: IP fragmentation broken in 3.6-rc ?

[Eric Dumazet]

On Tue, 2012-08-21 at 20:18 +0300, Julian Anastasov wrote:
> 	Hello,
> 
> On Tue, 21 Aug 2012, Eric Dumazet wrote:

> > My first patch was :
> > 
> > diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> > index e4ba974..9858714 100644
> > --- a/net/ipv4/route.c
> > +++ b/net/ipv4/route.c
> > @@ -956,6 +956,7 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
> >  		dst->obsolete = DST_OBSOLETE_KILL;
> >  	} else {
> >  		rt->rt_pmtu = mtu;
> > +		rt->dst.expires = 0;
> >  		dst_set_expires(&rt->dst, ip_rt_mtu_expires);
> 
> 	This is better, does not break ipv4_link_failure.
> There is a little race some ipv4_mtu() user to see
> rt_pmtu != 0 and dst.expires = 0 and to fail in time_after_eq
> test. May be that is why dst.expires is never set to 0.
> But I still don't understand what both changes fix.

In fact we re-enter ip_rt_update_pmtu() if we receive a second ICMP,
and rt->rt_pmtu is already set, but dst is expired.

Thats why Sylvain said it was not happening in the 10 minutes following
boot.

So calling again dst_set_expires(&rt->dst, ip_rt_mtu_expires) does
nothing : rt_pmtu is ignored because dst.expires is too old.

Maybe we should just do :

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index e4ba974..d0181e2 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -952,7 +952,7 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
 	ip_rt_build_flow_key(&fl4, sk, skb);
 	mtu = __ip_rt_update_pmtu(rt, &fl4, mtu);
 
-	if (!rt->rt_pmtu) {
+	if (!rt->rt_pmtu || time_after_eq(jiffies, rt->dst.expires)) {
 		dst->obsolete = DST_OBSOLETE_KILL;
 	} else {
 		rt->rt_pmtu = mtu;

Re: IP fragmentation broken in 3.6-rc ?

[Julian Anastasov]

	Hello,

On Tue, 21 Aug 2012, Eric Dumazet wrote:

> In fact we re-enter ip_rt_update_pmtu() if we receive a second ICMP,
> and rt->rt_pmtu is already set, but dst is expired.
> 
> Thats why Sylvain said it was not happening in the 10 minutes following
> boot.
> 
> So calling again dst_set_expires(&rt->dst, ip_rt_mtu_expires) does
> nothing : rt_pmtu is ignored because dst.expires is too old.

	Oh, well. I thought dst_set_expires was used
before for IPv4, it was not. I didn't expected function
that was old to cause problem. So, you are right that
MTU is not updated second time. But we must check if
such change will harm IPv6, dst_set_expires was/is
used there.

> Maybe we should just do :
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index e4ba974..d0181e2 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -952,7 +952,7 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
>  	ip_rt_build_flow_key(&fl4, sk, skb);
>  	mtu = __ip_rt_update_pmtu(rt, &fl4, mtu);
>  
> -	if (!rt->rt_pmtu) {
> +	if (!rt->rt_pmtu || time_after_eq(jiffies, rt->dst.expires)) {

	At first look it should not be needed to cause route
relookup here. tcp_v4_mtu_reduced is prepared to use the new
dst_mtu for mss after calling inet_csk_update_pmtu. It seems the
new code expects that every socket that performs PMTU Discovery
will get ICMP error, so tcp_v4_mtu_reduced should be called
for every socket with every new ICMP. If there are 100 sockets
we will get 100 ICMPs. If we set DST_OBSOLETE_KILL as you
propose, all sockets will notice the need to relookup
route and will learn the new PMTU from fnhe exception with less
chances for other ICMP events. At first look, this variant looks
better to me in case if we want single PMTU to be propagated
to all sockets immediately. Can it cause other problems?
Now the rt life with PMTU will be limited to 600 secs.

	OTOH, tcp_current_mss() is called often, it will
notice the new dst_mtu, so may be there is no need for
route relookup by setting DST_OBSOLETE_KILL?

	Not sure if we still have to support this second
option, to change dst_set_expires to check for timeout=0:

	if (dst->expires == 0 || time_after(expires, dst->expires) ||
	    !timeout)

	In IPv4, ip_rt_update_pmtu was the only place that
can extend dst->expires, for the rt_bind_exception case
I assume expires is still 0 before the checks. Not sure if
IPv6 needs to use time_after, may be it prefers the
time_before variant? IPv6 updates MTU in rt6_update_expires
and we call dst_set_expires where the timer will not be
extended. But may be the intention is that MTU only
can reduce the expiration, not to extend it after timer
was set when route was added.

	So, another option is to create new function
dst_update_expires and to use it just for IPv4, it will
use time_after because IPv4 uses the timer just for
PMTU while IPv6 uses it in ip6_route_add() to limit
route lifetime due to lft values.

	dst_update_expires will be like dst_set_expires
but with this difference (time_after):

	if (dst->expires == 0 || time_after(expires, dst->expires) ||
	    !timeout)

	while dst_set_expires will be as before, used by IPv6.

>  		dst->obsolete = DST_OBSOLETE_KILL;
>  	} else {
>  		rt->rt_pmtu = mtu;

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: IP fragmentation broken in 3.6-rc ?

[Eric Dumazet]

On Tue, 2012-08-21 at 23:00 +0300, Julian Anastasov wrote:
> 	Hello,
> 
> On Tue, 21 Aug 2012, Eric Dumazet wrote:
> 
> > In fact we re-enter ip_rt_update_pmtu() if we receive a second ICMP,
> > and rt->rt_pmtu is already set, but dst is expired.
> > 
> > Thats why Sylvain said it was not happening in the 10 minutes following
> > boot.
> > 
> > So calling again dst_set_expires(&rt->dst, ip_rt_mtu_expires) does
> > nothing : rt_pmtu is ignored because dst.expires is too old.
> 
> 	Oh, well. I thought dst_set_expires was used
> before for IPv4, it was not. I didn't expected function
> that was old to cause problem. So, you are right that
> MTU is not updated second time. But we must check if
> such change will harm IPv6, dst_set_expires was/is
> used there.
> 
> > Maybe we should just do :
> > 
> > diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> > index e4ba974..d0181e2 100644
> > --- a/net/ipv4/route.c
> > +++ b/net/ipv4/route.c
> > @@ -952,7 +952,7 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
> >  	ip_rt_build_flow_key(&fl4, sk, skb);
> >  	mtu = __ip_rt_update_pmtu(rt, &fl4, mtu);
> >  
> > -	if (!rt->rt_pmtu) {
> > +	if (!rt->rt_pmtu || time_after_eq(jiffies, rt->dst.expires)) {
> 
> 	At first look it should not be needed to cause route
> relookup here. tcp_v4_mtu_reduced is prepared to use the new
> dst_mtu for mss after calling inet_csk_update_pmtu. It seems the
> new code expects that every socket that performs PMTU Discovery
> will get ICMP error, so tcp_v4_mtu_reduced should be called
> for every socket with every new ICMP. If there are 100 sockets
> we will get 100 ICMPs. If we set DST_OBSOLETE_KILL as you
> propose, all sockets will notice the need to relookup
> route and will learn the new PMTU from fnhe exception with less
> chances for other ICMP events. At first look, this variant looks
> better to me in case if we want single PMTU to be propagated
> to all sockets immediately. Can it cause other problems?
> Now the rt life with PMTU will be limited to 600 secs.
> 
> 	OTOH, tcp_current_mss() is called often, it will
> notice the new dst_mtu, so may be there is no need for
> route relookup by setting DST_OBSOLETE_KILL?
> 
> 	Not sure if we still have to support this second
> option, to change dst_set_expires to check for timeout=0:
> 
> 	if (dst->expires == 0 || time_after(expires, dst->expires) ||
> 	    !timeout)
> 
> 	In IPv4, ip_rt_update_pmtu was the only place that
> can extend dst->expires, for the rt_bind_exception case
> I assume expires is still 0 before the checks. Not sure if
> IPv6 needs to use time_after, may be it prefers the
> time_before variant? IPv6 updates MTU in rt6_update_expires
> and we call dst_set_expires where the timer will not be
> extended. But may be the intention is that MTU only
> can reduce the expiration, not to extend it after timer
> was set when route was added.
> 
> 	So, another option is to create new function
> dst_update_expires and to use it just for IPv4, it will
> use time_after because IPv4 uses the timer just for
> PMTU while IPv6 uses it in ip6_route_add() to limit
> route lifetime due to lft values.
> 
> 	dst_update_expires will be like dst_set_expires
> but with this difference (time_after):
> 
> 	if (dst->expires == 0 || time_after(expires, dst->expires) ||
> 	    !timeout)

Hmm, all these tests are not really needed, what about :

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index e4ba974..8d6d320 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -956,7 +956,7 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
 		dst->obsolete = DST_OBSOLETE_KILL;
 	} else {
 		rt->rt_pmtu = mtu;
-		dst_set_expires(&rt->dst, ip_rt_mtu_expires);
+		rt->dst.expires = max(1UL, jiffies + ip_rt_mtu_expires);
 	}
 }
 

Re: IP fragmentation broken in 3.6-rc ?

[Julian Anastasov]

	Hello,

On Tue, 21 Aug 2012, Eric Dumazet wrote:

> > 	dst_update_expires will be like dst_set_expires
> > but with this difference (time_after):
> > 
> > 	if (dst->expires == 0 || time_after(expires, dst->expires) ||
> > 	    !timeout)
> 
> Hmm, all these tests are not really needed, what about :
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index e4ba974..8d6d320 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -956,7 +956,7 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
>  		dst->obsolete = DST_OBSOLETE_KILL;
>  	} else {
>  		rt->rt_pmtu = mtu;
> -		dst_set_expires(&rt->dst, ip_rt_mtu_expires);
> +		rt->dst.expires = max(1UL, jiffies + ip_rt_mtu_expires);

	The best solution, I think.

>  	}
>  }

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: IP fragmentation broken in 3.6-rc ?

[Sylvain Munaut]

Hi,

> Following patch should help :
>
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 621e351..a04aa37 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -435,7 +435,7 @@ static inline void dst_set_expires(struct dst_entry *dst, int timeout)
>         if (expires == 0)
>                 expires = 1;
>
> -       if (dst->expires == 0 || time_before(expires, dst->expires))
> +       if (dst->expires == 0 || time_after(expires, dst->expires))
>                 dst->expires = expires;
>  }

Yes, this fixes it for me too . ( 2h without issues on 2 different machines )

But that line has been in the kernel for _years_ ... so I'm not sure
what's going on there ...


Cheers,

    Sylvain