Hi!
> task taken from http://kerneljanitors.org/TODO:
>
> A _lot_ of drivers end up caring about absolute time,
> because a _lot_ of
> drivers have a very simple issue like:
>
> - poll this port every 10ms until it returns "ready", or
> until we time
> out after 500ms.
>
> And the thing is, you can do it the stupid way:
>
> for (i = 0; i < 50; i++) {
> if (ready())
> return 0;
> msleep(10);
> }
> .. timeout ..
>
> or you can do it the _right_ way. The stupid way is
> simpler, but anybody
> who doesn't see what the problem is has some serious
> problems in kernel
> programming. Hint: it might not be polling for half a
Well, whoever wrote thi has some serious problems (in attitude
department). *Any* loop you design may take half a minute under
streange circumstances.
> second, it might be
> polling for half a _minute_ for all you know.
>
> In other words, the _right_ way to do this is literally
>
> unsigned long timeout = jiffies +
> msecs_to_jiffies(500);
> for (;;) {
> if (ready())
> return 0;
> if (time_after(timeout, jiffies))
> break;
> msleep(10);
> }
>
> which is unquestionably more complex, yes, but it's more
> complex because
> it is CORRECT!
Original code is correct, too.
Anyway you probably want to hide complexity in some macro if we are
going this way.
Pavel
--
Thanks for all the (sleeping) penguins.
Hi!
> > task taken from http://kerneljanitors.org/TODO:
> >
> > A _lot_ of drivers end up caring about absolute time,
> > because a _lot_ of
> > drivers have a very simple issue like:
> >
> > - poll this port every 10ms until it returns "ready", or
> > until we time
> > out after 500ms.
> >
> > And the thing is, you can do it the stupid way:
> >
> > for (i = 0; i < 50; i++) {
> > if (ready())
> > return 0;
> > msleep(10);
> > }
> > .. timeout ..
> >
> > or you can do it the _right_ way. The stupid way is
> > simpler, but anybody
> > who doesn't see what the problem is has some serious
> > problems in kernel
> > programming. Hint: it might not be polling for half a
>
> Well, whoever wrote thi has some serious problems (in attitude
> department). *Any* loop you design may take half a minute under
> streange circumstances.
>
> > second, it might be
> > polling for half a _minute_ for all you know.
> >
> > In other words, the _right_ way to do this is literally
> >
> > unsigned long timeout = jiffies +
> > msecs_to_jiffies(500);
> > for (;;) {
> > if (ready())
> > return 0;
> > if (time_after(timeout, jiffies))
> > break;
> > msleep(10);
> > }
> >
> > which is unquestionably more complex, yes, but it's more
> > complex because
> > it is CORRECT!
Actually it may be broken, depending on use. In some cases this loop
may want to poll the hardware 50 times, 10msec appart... and your loop
can poll it only once in extreme conditions.
Actually your loop is totally broken, and may poll only once (without
any delay) and then directly timeout :-P -- that will break _any_
user.
Pavel
--
Thanks for all the (sleeping) penguins.
G'day
On 8/5/06, Pavel Machek <[email protected]> wrote:
> > Well, whoever wrote thi has some serious problems (in attitude
> > department). *Any* loop you design may take half a minute under
> > streange circumstances.
6.
common mistake in polling loops [from Linus]:
>
> Actually it may be broken, depending on use. In some cases this loop
> may want to poll the hardware 50 times, 10msec appart... and your loop
> can poll it only once in extreme conditions.
>
> Actually your loop is totally broken, and may poll only once (without
> any delay) and then directly timeout :-P -- that will break _any_
> user.
The Idea is that we are checking some event in external hardware that
we know will complete in a given time (This time is not dependant on
system activity but is fixed). After that time if the event has not
happened we know something has borked.
So in the loop, after the time period has expired without the event
happening we can go and clean up and get ready to go again, without
bothering to poll any more, because we already know something has
borked.
What does this give you ? Well it can improve performance by speeding
up re-try's when under heavy system load. The cost of cause is code
complexity.
Darren J.
Hi!
> >> Well, whoever wrote thi has some serious problems (in attitude
> >> department). *Any* loop you design may take half a minute under
> >> streange circumstances.
>
> 6.
> common mistake in polling loops [from Linus]:
Yes, Linus was wrong here. Or more precisely, he's right original code
is broken, but his suggested "fix" is worse than the original.
unsigned long timeout = jiffies + HZ/2;
for (;;) {
if (ready())
return 0;
[IMAGINE HALF A SECOND DELAY HERE]
if (time_after(timeout, jiffies))
break;
msleep(10);
}
Oops.
> >Actually it may be broken, depending on use. In some cases this loop
> >may want to poll the hardware 50 times, 10msec appart... and your loop
> >can poll it only once in extreme conditions.
> >
> >Actually your loop is totally broken, and may poll only once (without
> >any delay) and then directly timeout :-P -- that will break _any_
> >user.
>
> The Idea is that we are checking some event in external hardware that
> we know will complete in a given time (This time is not dependant on
> system activity but is fixed). After that time if the event has not
> happened we know something has borked.
But you have to make sure YOU CHECK READY AFTER THE TIMEOUT. Linus'
code does not do that.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
G'day
On 8/8/06, Pavel Machek <[email protected]> wrote:
> Hi!
>
> > >> Well, whoever wrote thi has some serious problems (in attitude
> > >> department). *Any* loop you design may take half a minute under
> > >> streange circumstances.
> >
> > 6.
> > common mistake in polling loops [from Linus]:
>
> Yes, Linus was wrong here. Or more precisely, he's right original code
> is broken, but his suggested "fix" is worse than the original.
>
> unsigned long timeout = jiffies + HZ/2;
> for (;;) {
> if (ready())
> return 0;
> [IMAGINE HALF A SECOND DELAY HERE]
> if (time_after(timeout, jiffies))
> break;
> msleep(10);
> }
>
> Oops.
>
> > >Actually it may be broken, depending on use. In some cases this loop
> > >may want to poll the hardware 50 times, 10msec appart... and your loop
> > >can poll it only once in extreme conditions.
> > >
> > >Actually your loop is totally broken, and may poll only once (without
> > >any delay) and then directly timeout :-P -- that will break _any_
> > >user.
> >
> > The Idea is that we are checking some event in external hardware that
> > we know will complete in a given time (This time is not dependant on
> > system activity but is fixed). After that time if the event has not
> > happened we know something has borked.
>
> But you have to make sure YOU CHECK READY AFTER THE TIMEOUT. Linus'
> code does not do that.
> Pavel
Sorry I did not realise that was your problem with the code.
Would it help if we just explicitly added a
if (ready())
return 0;
after the loop, in the example code? so people wont miss adding
something like that in?
Darren J.
On 8/7/06, Darren Jenkins <[email protected]> wrote:
> G'day
>
> On 8/8/06, Pavel Machek <[email protected]> wrote:
> > Hi!
> >
> > > >> Well, whoever wrote thi has some serious problems (in attitude
> > > >> department). *Any* loop you design may take half a minute under
> > > >> streange circumstances.
> > >
> > > 6.
> > > common mistake in polling loops [from Linus]:
> >
> > Yes, Linus was wrong here. Or more precisely, he's right original code
> > is broken, but his suggested "fix" is worse than the original.
> >
> > unsigned long timeout = jiffies + HZ/2;
> > for (;;) {
> > if (ready())
> > return 0;
> > [IMAGINE HALF A SECOND DELAY HERE]
> > if (time_after(timeout, jiffies))
> > break;
> > msleep(10);
> > }
> >
> > Oops.
> >
> > > >Actually it may be broken, depending on use. In some cases this loop
> > > >may want to poll the hardware 50 times, 10msec appart... and your loop
> > > >can poll it only once in extreme conditions.
> > > >
> > > >Actually your loop is totally broken, and may poll only once (without
> > > >any delay) and then directly timeout :-P -- that will break _any_
> > > >user.
> > >
> > > The Idea is that we are checking some event in external hardware that
> > > we know will complete in a given time (This time is not dependant on
> > > system activity but is fixed). After that time if the event has not
> > > happened we know something has borked.
> >
> > But you have to make sure YOU CHECK READY AFTER THE TIMEOUT. Linus'
> > code does not do that.
> > Pavel
>
>
> Sorry I did not realise that was your problem with the code.
> Would it help if we just explicitly added a
>
unsigned long timeout = jiffies + HZ/2;
for (;;) {
if (ready())
return 0;
[IMAGINE HALF A SECOND DELAY HERE]
if (time_after(timeout, jiffies)) {
if (ready())
return 0;
break;
}
msleep(10);
}
Wouldn't this be better than adding a check after the break of loop?
> if (ready())
> return 0;
>
> after the loop, in the example code? so people wont miss adding
> something like that in?
Hi!
> >> >> Well, whoever wrote thi has some serious problems (in attitude
> >> >> department). *Any* loop you design may take half a minute under
> >> >> streange circumstances.
> >>
> >> 6.
> >> common mistake in polling loops [from Linus]:
> >
> >Yes, Linus was wrong here. Or more precisely, he's right original code
> >is broken, but his suggested "fix" is worse than the original.
> >
> > unsigned long timeout = jiffies + HZ/2;
> > for (;;) {
> > if (ready())
> > return 0;
> >[IMAGINE HALF A SECOND DELAY HERE]
> > if (time_after(timeout, jiffies))
> > break;
> > msleep(10);
> > }
> >
> >Oops.
> >
> >> >Actually it may be broken, depending on use. In some cases this loop
> >> >may want to poll the hardware 50 times, 10msec appart... and your loop
> >> >can poll it only once in extreme conditions.
> >> >
> >> >Actually your loop is totally broken, and may poll only once (without
> >> >any delay) and then directly timeout :-P -- that will break _any_
> >> >user.
> >>
> >> The Idea is that we are checking some event in external hardware that
> >> we know will complete in a given time (This time is not dependant on
> >> system activity but is fixed). After that time if the event has not
> >> happened we know something has borked.
> >
> >But you have to make sure YOU CHECK READY AFTER THE TIMEOUT. Linus'
> >code does not do that.
>
> Sorry I did not realise that was your problem with the code.
> Would it help if we just explicitly added a
>
> if (ready())
> return 0;
>
> after the loop, in the example code? so people wont miss adding
> something like that in?
Yes, that would do the trick.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[resending due to delivery failure.]
On Monday 07 August 2006 22:53, Om N. wrote:
> On 8/7/06, Darren Jenkins <[email protected]> wrote:
> > G'day
> >
> > On 8/8/06, Pavel Machek <[email protected]> wrote:
> > >
> > > But you have to make sure YOU CHECK READY AFTER THE TIMEOUT. Linus'
> > > code does not do that.
> > > Pavel
> >
> >
> > Sorry I did not realise that was your problem with the code.
> > Would it help if we just explicitly added a
> >
> unsigned long timeout = jiffies + HZ/2;
> for (;;) {
> if (ready())
> return 0;
> [IMAGINE HALF A SECOND DELAY HERE]
> if (time_after(timeout, jiffies)) {
> if (ready())
> return 0;
> break;
> }
> msleep(10);
> }
> Wouldn't this be better than adding a check after the break of loop?
You're getting duplicated code there. That'll be an issue in more
complex loops. How about:
unsigned long timeout = jiffies + HZ/2;
int timeup = 0;
for (;;;) {
if (ready())
return 0;
if (timeup)
break;
msleep(10);
timeup = time_after(timeout, jiffies);
};
... timeout ...
However care is needed with the use of timeup, since timeup==1 only
indicates failure at the point it's tested in the loop; in particular
it won't indicate failure after the loop if "return 0" is changed to
"break".
Andrew Wade
G'day,
On 8/10/06, Andrew James Wade <[email protected]> wrote:
> You're getting duplicated code there. That'll be an issue in more
> complex loops. How about:
>
> unsigned long timeout = jiffies + HZ/2;
> int timeup = 0;
>
> for (;;;) {
> if (ready())
> return 0;
> if (timeup)
> break;
> msleep(10);
> timeup = time_after(timeout, jiffies);
> };
> ... timeout ...
>
Nice, looks better than my idea.
Removes the code duplication and reduces complexity(a little) at the
cost of an extra variable.
The only Nitpick is
- int timeup = 0;
+ unsigned char timeup = 0;
>
> Andrew Wade
Darren J.