Re: [PATCH] net/ipv4/*, net/core/neighbour.c jiffies cleanup

[Andreas Dilger <adilger@turbolabs.com>]

On Nov 07, 2001  16:44 -0800, David S. Miller wrote:
> Why do they these cases that are actually in the code need to cast to
> a signed value to get a correct answer?  They are not like your
> example.
> 
> Almost all of these cases are:
> 
> 	(jiffies - SOME_VALUE_KNOWN_TO_BE_IN_THE_PAST) > 5 * HZ
> 
> So you say if we don't cast to signed, this won't get it right on
> wrap-around?  I disagree, let's say "long" is 32-bits and jiffies
> wrapped around to "0x2" and SOME_VALUE... is 0xfffffff8.  The
> subtraction above yields 10, and that is what we want.

OK, in this code it mostly appears that the wraparound is correct, but
in some cases it is very hard to say without specific knowledge of the code:

	neigh->confirmed = jiffies - (n->parms->base_reachable_time<<1);
	.
	.
	.
	if ((state&NUD_VALID) &&
	    now - neigh->confirmed < neigh->parms->reachable_time) {

How are we to know that the above calculation won't be wrong because of
jiffies wrap?

> Please show me a bad case where casting to signed is necessary.

I don't know enough about the net code to say for sure, but for example
in drivers/sound/sb_common.c:

	limit = jiffies + HZ / 10;      /* Timeout */

	for (i = 0; i < 500000 && (limit-jiffies)>0; i++)

Since limit and jiffies are both unsigned, the value will always be > 0,
unless they are equal.

> I actually ran through the tree the other night myself starting to
> convert these things, then I noticed that I couldn't even convince
> myself that the code was incorrect.

I don't disagree that it is possible to make correct code without the
macros, but it is easier to guarantee that it IS correct with the macros.
Rather than evaluate each jiffies usage on a case-by-case basis, it is
much easier to do a code audit and fix all comparisons.  I don't see that
it harms anything to use the macros instead.

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/