Patchwork [1/3] gre: used time_before for comparing jiffies

login
register
mail settings
Submitter Wei Yongjun
Date Feb. 20, 2009, 4:06 a.m.
Message ID <499E2C54.9000409@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/23464/
State Rejected
Delegated to: David Miller
Headers show

Comments

Wei Yongjun - Feb. 20, 2009, 4:06 a.m.
The functions time_before is more robust for comparing
jiffies against other values.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 net/ipv4/ip_gre.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
David Miller - Feb. 20, 2009, 8:01 a.m.
From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Fri, 20 Feb 2009 12:06:44 +0800

> The functions time_before is more robust for comparing
> jiffies against other values.
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

There is some history of this in the networking code.

If you inspect carefully, the open-coded versions of
time comparisons in the networking have a larger window
of acceptance.

So actually, it is a regression fo use time_*() helpers
in these cases.

If these things have been like this for so long, it is
very likely there is a good reason :-)
--
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
Herbert Xu - Feb. 20, 2009, 8:39 a.m.
David Miller <davem@davemloft.net> wrote:
>
> There is some history of this in the networking code.
> 
> If you inspect carefully, the open-coded versions of
> time comparisons in the networking have a larger window
> of acceptance.

Hmm, it looks like the macro version is actually better in this
case.

-	if (jiffies - t->err_time < IPTUNNEL_ERR_TIMEO)

This wraps at jiffies = LONG_MAX + t->err_time + 1

+	if (time_before(jiffies, t->err_time + IPTUNNEL_ERR_TIMEO))

This expands to

	(long)jiffies - (long)(t->err_time + IPTUNNEL_ERR_TIMEO) < 0

which wraps at jiffies = LONG_MAX + t->err_time + IPTUNNEL_ERR_TIMEO + 1

So assuming that IPTUNNEL_ERR_TIMEO > 0, then the macro version
wraps around after the open-coded version, which would seem to
mean that it's better, no?

Cheers,
David Miller - Feb. 20, 2009, 8:45 a.m.
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 20 Feb 2009 16:39:35 +0800

> So assuming that IPTUNNEL_ERR_TIMEO > 0, then the macro version
> wraps around after the open-coded version, which would seem to
> mean that it's better, no?

Seems that way...

Let me see if I can find Alexey's email where he explains
all of this.  I really don't want to touch these things
without being able to read that again.

If anyone else can find his posting about this stuff from
years ago, please post the link.

Thanks.
--
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
Herbert Xu - Feb. 24, 2009, 7:13 a.m.
Wei Yongjun <yjwei@cn.fujitsu.com> wrote:
> The functions time_before is more robust for comparing
> jiffies against other values.
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

Please resubmit these patches so that they can be reviewed again.

Thanks,
David Miller - Feb. 24, 2009, 7:41 a.m.
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 24 Feb 2009 15:13:52 +0800

> Wei Yongjun <yjwei@cn.fujitsu.com> wrote:
> > The functions time_before is more robust for comparing
> > jiffies against other values.
> > 
> > Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> 
> Please resubmit these patches so that they can be reviewed again.

Indeed, I think my original objections have been cleared up
after some research.
--
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

Patch

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 0101521..811bef7 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -432,7 +432,7 @@  static void ipgre_err(struct sk_buff *skb, u32 info)
 	if (t->parms.iph.ttl == 0 && type == ICMP_TIME_EXCEEDED)
 		goto out;
 
-	if (jiffies - t->err_time < IPTUNNEL_ERR_TIMEO)
+	if (time_before(jiffies, t->err_time + IPTUNNEL_ERR_TIMEO))
 		t->err_count++;
 	else
 		t->err_count = 1;
@@ -744,7 +744,8 @@  static int ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
 #endif
 
 	if (tunnel->err_count > 0) {
-		if (jiffies - tunnel->err_time < IPTUNNEL_ERR_TIMEO) {
+		if (time_before(jiffies,
+				tunnel->err_time + IPTUNNEL_ERR_TIMEO)) {
 			tunnel->err_count--;
 
 			dst_link_failure(skb);