> Woops In the last ten minutes I reviewed some thing. Forget the previous
> patch (and eventually upgrade to arca-31). Excuse for the previus spam...
> begin 664 timeval.gz
[...]
> end
Andrea, I like your basic idea very much: using a (supposedly good)
function to convert timevals and timespecs to jiffies, and vice-versa.
It's obviously a Good Thing. Doing otherwise is reinventing a (small but
tricky) wheel.
Just a few comments about your uuencoded patch, which I split based on
file.
(a) About the time.h patch: since you're overhauling the whole set of
conversion functions, my comments are not limited to your patch per-se.
I don't find it clear at all, and I'm not sure it works. I think this kind
of code should be made clear, since it deals with overflows by its very
nature. I don't see what you mean there:
+ 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 :-)
Also, returning MAX_JIFFY from the xxx_to_jiffies() functions might be OK,
or maybe not. What is that constant supposed to mean? We have
MAX_SCHEDULE_TIMEOUT already, and they seem to have the same value
(MAX_LONG). So basically, when you have an overflow, timespec_to_jiffies
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
calling select() would like. They could prefer to be woken up, only to
find out that it's not time yet. And go back to sleep until it _is_ time.
It's not clear at all what jiffies_to_timespec() should return when passed
a MAX_SCHEDULE_TIMEOUT; an answer coherent with the select() interface
would be "a NULL *timespec", but that's not an option. There's also the
eternal question: are jiffies signed or unsigned?
(As a general note, I find MAX_SCHEDULE_TIMEOUT == LONG_MAX to be a bad
idea. In some ways, it's good, because you can compare another timeout to
it without a special case. But that gives a sense of "ease of use" that
will make you do things like ... add 1 to it. Ooops. Ideally, noone would
be looking into these structs directly but would use nice bulletproof
macros or functions. Actually, I would even do stuff on time_t's and
jiffies through macros/functions)
There are many other similar problems in your patch.
Finally, I'd say that this is pretty long. Can't some of these be
expressed in terms of others, instead of cut&paste? It would also be less
error-prone.
(b) The patch for select.c seems sound (of course it depends on the rest,
but per-se I like it).
(c) I'm not sure what's the deal about signal.c. Your patch:
- timeout = (timespec_to_jiffies(&ts)
- + (ts.tv_sec || ts.tv_nsec));
+ timeout = timespec_to_jiffies(&ts);
I guess substracting 1 to the timeout when it's not (0,0) was intentional
(that code was too "smart", so good riddance anyway).
(d) About the sched.c patch: for me this has nothing to do with the
problem at hand. The change comment is also misleading; schedule_timeout
was there before your patch. And what are you patching against? That's not
2.1.129 I think.
Finally: to get back to the original problem: you and I seem to agree that
it's not worth looping around the do_select() in sys_select() to get huge
and accurate timeouts, even in the face of a large HZ. I haven't received
any answers for my "mini-poll" so far. I'll wait a couple days and I'll
send something with an explicit subject. But in the meantime, what do you
think?
-- 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/