lkml.org 
[lkml]   [1998]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
Subject[tiny patch] Timeout overflow in select()
Andrea, as you've seen, 2.1.130 fixes the select() timeout overflow. It's
done directly in sys_select() - a simple fix (even though not exactly the
one I proposed).

So I guess the original problem is solved. The only remaining point that I
see is that currently, an overflow means "eternity". It might not be the
best thing to do. Strictly speaking it would probably be better to do "as
long as possible, but not forever" (see rationale).

But that's really a detail.

(Linus, if you're reading this, and are into details, here's what I mean:

--- linux-2.1.130/fs/select.c Sun Nov 22 10:08:50 1998
+++ linux/fs/select.c Sat Nov 28 17:35:03 1998
@@ -224,6 +224,9 @@
if ((unsigned long) sec < MAX_SELECT_SECONDS) {
timeout = ROUND_UP(usec, 1000000/HZ);
timeout += sec * (unsigned long) HZ;
+ } else {
+ /* long, but not forever */
+ timeout = (MAX_SELECT_SECONDS-1)*HZ;
}
}

)
Rationale: consider the portability of some code using long timeouts for
select(). With 2.1.130, on some archs, the select() will wake up after
(say) a day. On some others, never. The user code can't even know what's
the limit (well, without testing :-) ). If it was woken up after a couple
hours, it could easily recover: just check the current time - in most
cases it will do that anyway. The "-1" in my patch makes _really_ sure we
have no collision with "eternity" (in case someone changes the definition
of MAX_SELECT_SECOND). Linus?

Andrea, here are some comments on your original patch, for the record:

On Thu, 26 Nov 1998, Andrea Arcangeli wrote:

> >+ nsec_tmp = nsec + (1000000000UL+HZ-1)/HZ - 1;
> >+ if (nsec_tmp > nsec)
> >+ nsec = nsec_tmp;
> >"A moment of consideration" shows that (nsec_tmp > nsec) is false only
> >when you have an overflow in the addition in the previous line. I'm sorry,
> >but if that's what you meant, that's clear as mud :-)
> Yes it' s what I mean. What does it mean "clear as mud"? What I should
> change to fix the problem you are reporting?

I guess it's a matter of style. Usually, an overflow triggers something.
Here it's the "fall-through" case: when you have an overflow, your code
leaves nsec with some unspecified "too large to add" value, but not any
specific one. You could have a commented "else" like this to make the
intent clear:

nsec_tmp = nsec + (1000000000UL+HZ-1)/HZ - 1;
if (nsec_tmp > nsec)
nsec = nsec_tmp;
else nsec = SOME_SUITABLE_VALUE; /* overflow */
Ooops, I forgot: you're into cycle-shaving ;-)

> >will return something that means "forever", which is not the same as "as
> >long as you can do, but not forever". That might not be what some people
>
> This is true but it' s the right thing to do according to me... Really we
> could do two helper function, one that return MAX_JIFFY-1; and one that
> returns MAX_JIFFY;.

To me, what's questionable is the MAX_TIMEOUT_VALUE == MAX_LONG, and the
name of the macro itself. It should be "ETERNITY_TIMEOUT", or something
like that. It's not the upper bound on the timeout. "Eternity" is a
special case. It's probably better to leave it clearly separated from the
range of valid values. The same way open() returns -1 on error: -1 is not
a valid FD (which is, in realistic settings, an unsigned short). Same
trick for getchar(). The rest depends on the exact semantics you want for
select(), as I explained above.

One more comment on your patch: Generally speaking, this style of checking
overflows _after_ they happen makes me a little nervous. It works in many
cases, like what you do: "a+b". If you're doing "a+b+c", testing things
like the sign of the result doesn't catch all overflows; nor if you have
multiplications. So a seemingly innocent change made later on could break
the test: a case of bad "bug locality".

And I still like the idea of using functions to deal with timevals. Well,
kinda. It makes sense not to do it as well, since the special cases are
hard to handle in a generic manner.

Anyway, I'll stop commenting on this, it's a dead horse (and has been for
some time). And I probably can't teach you anything!

--
Stephane Belmon <sbelmon@cse.ucsd.edu>
University of California, San Diego
Computer Science and Engineering Department


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:45    [W:0.050 / U:1.308 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site