2005-10-21 11:22:54

by Yitzchak Eidus

[permalink] [raw]
Subject: question about code from the linux kernel development ( se ) book

first i am very sorry if it isnt the place to ask questions like this
but i didnt know where else to ask ( i tryed irc channels and i was
send from there to this list )
anyway:
does this following code look buggy? :
DECLARE_WAITQUEUE ( wait , current );
add_wait_queue ( q , &wait );
while ( !condition ) {
set_current_stat ( TASK_INTERRUPTABLE ); i
if ( signal_pending ( current ) )
/* handle signal */
schedule ( ); }
set_current_state ( TASK_RUNNING );
remove_wait_queue ( q , &wait );
first:doesnt in the way from checking the !condition to
set_current_state the condition can be changed no?

second:why not putting the schedule ( ); right after the
set_current_state ( ) , what the point in checking the if (
signal_pending ( ) first, if the proccess doesnt started to sleep yet?
third: in the cleaning in the way from putting the set_current_state (
TASK_RUNNING ) into remove_wait_queue , cant the queue wait list ( q )
wake up again the wait procsess?
( thnks for the help , please if it can be done answer quickly i am
tanker in the idf and need to come back to the army soon , ( no
internet there... ) )
thnks!


2005-10-21 13:03:37

by Steven Rostedt

[permalink] [raw]
Subject: Re: question about code from the linux kernel development ( se ) book


Oh God, please indent code examples, and if your email client strips white
space, change your email client.

On Fri, 21 Oct 2005, Yitzchak Eidus wrote:

> first i am very sorry if it isnt the place to ask questions like this
> but i didnt know where else to ask ( i tryed irc channels and i was
> send from there to this list )
> anyway:
> does this following code look buggy? :

[ Indention added ]

> DECLARE_WAITQUEUE ( wait , current );
> add_wait_queue ( q , &wait );
> while ( !condition ) {
> set_current_stat ( TASK_INTERRUPTABLE ); i
> if ( signal_pending ( current ) )
> /* handle signal */

I assume that the signal_pending is the if result and not the schedule.
Since there was no indentation I couldn't tell.

> schedule ( );
/* Moved brace down added */
}
> set_current_state ( TASK_RUNNING );
> remove_wait_queue ( q , &wait );

Before I go to your questions, I'll first answer that this _is_ buggy
code. If the condition is checked and you are woken up between the
while (!condition) and the set_current_state, then you will end up
sleeping forever or until someone sends you a signal.

> first:doesnt in the way from checking the !condition to
> set_current_state the condition can be changed no?

Yes, and then put it again after schedule.

>
> second:why not putting the schedule ( ); right after the
> set_current_state ( ) , what the point in checking the if (
> signal_pending ( ) first, if the proccess doesnt started to sleep yet?

Yes, I would put the signal_pending check after the schedule (as most of
the kernel does this).

> third: in the cleaning in the way from putting the set_current_state (
> TASK_RUNNING ) into remove_wait_queue , cant the queue wait list ( q )
> wake up again the wait procsess?

Yeah, so? There's no harm in that, except for an extra cpu cycles that
are done to wake it up. That, I wouldn't change.

> ( thnks for the help , please if it can be done answer quickly i am
> tanker in the idf and need to come back to the army soon , ( no
> internet there... ) )

BTW, this is not an IRC, we use normal capitalization and normal spelling
(when we know how to spell a word ;-). So the next time you send to the
list, send it as if you were writing a serious letter, or you may just be
ignored. (as you might have been if I didn't respond).

-- Steve

2005-10-21 13:10:14

by Jesper Juhl

[permalink] [raw]
Subject: Re: question about code from the linux kernel development ( se ) book

On 10/21/05, Steven Rostedt <[email protected]> wrote:
>
> Oh God, please indent code examples, and if your email client strips white
> space, change your email client.
>
> On Fri, 21 Oct 2005, Yitzchak Eidus wrote:
>
> > first i am very sorry if it isnt the place to ask questions like this
> > but i didnt know where else to ask ( i tryed irc channels and i was
> > send from there to this list )
> > anyway:
> > does this following code look buggy? :
>
> [ Indention added ]
>
Thanks.

> > DECLARE_WAITQUEUE ( wait , current );
> > add_wait_queue ( q , &wait );
> > while ( !condition ) {
> > set_current_stat ( TASK_INTERRUPTABLE ); i

It is probably just a typo, but that "i" at the end of the line would be a bug.


--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html

2005-10-21 14:41:06

by Yitzchak Eidus

[permalink] [raw]
Subject: Re: question about code from the linux kernel development ( se ) book

First Steve I want to thanks you for answering me
your answered all the questions that i had
I can understand your complains , and i will try to make it better
next time ( i almost never used mailing lists... , so sorry about the
white space striping , and the spelling... )

Jesper - yes it was a typo

Nick - thanks :)

On 10/21/05, Steven Rostedt <[email protected]> wrote:
>
> Oh God, please indent code examples, and if your email client strips white
> space, change your email client.
>
> On Fri, 21 Oct 2005, Yitzchak Eidus wrote:
>
> > first i am very sorry if it isnt the place to ask questions like this
> > but i didnt know where else to ask ( i tryed irc channels and i was
> > send from there to this list )
> > anyway:
> > does this following code look buggy? :
>
> [ Indention added ]
>
> > DECLARE_WAITQUEUE ( wait , current );
> > add_wait_queue ( q , &wait );
> > while ( !condition ) {
> > set_current_stat ( TASK_INTERRUPTABLE ); i
> > if ( signal_pending ( current ) )
> > /* handle signal */
>
> I assume that the signal_pending is the if result and not the schedule.
> Since there was no indentation I couldn't tell.
>
> > schedule ( );
> /* Moved brace down added */
> }
> > set_current_state ( TASK_RUNNING );
> > remove_wait_queue ( q , &wait );
>
> Before I go to your questions, I'll first answer that this _is_ buggy
> code. If the condition is checked and you are woken up between the
> while (!condition) and the set_current_state, then you will end up
> sleeping forever or until someone sends you a signal.
>
> > first:doesnt in the way from checking the !condition to
> > set_current_state the condition can be changed no?
>
> Yes, and then put it again after schedule.
>
> >
> > second:why not putting the schedule ( ); right after the
> > set_current_state ( ) , what the point in checking the if (
> > signal_pending ( ) first, if the proccess doesnt started to sleep yet?
>
> Yes, I would put the signal_pending check after the schedule (as most of
> the kernel does this).
>
> > third: in the cleaning in the way from putting the set_current_state (
> > TASK_RUNNING ) into remove_wait_queue , cant the queue wait list ( q )
> > wake up again the wait procsess?
>
> Yeah, so? There's no harm in that, except for an extra cpu cycles that
> are done to wake it up. That, I wouldn't change.
>
> > ( thnks for the help , please if it can be done answer quickly i am
> > tanker in the idf and need to come back to the army soon , ( no
> > internet there... ) )
>
> BTW, this is not an IRC, we use normal capitalization and normal spelling
> (when we know how to spell a word ;-). So the next time you send to the
> list, send it as if you were writing a serious letter, or you may just be
> ignored. (as you might have been if I didn't respond).
>
> -- Steve
>

2005-10-21 15:07:28

by Steven Rostedt

[permalink] [raw]
Subject: Re: question about code from the linux kernel development ( se ) book


On Fri, 21 Oct 2005, Yitzchak Eidus wrote:

> First Steve I want to thanks you for answering me
> your answered all the questions that i had
> I can understand your complains , and i will try to make it better
> next time ( i almost never used mailing lists... , so sorry about the
> white space striping , and the spelling... )
>
> Jesper - yes it was a typo
>
> Nick - thanks :)
>

Hi Yitzchak,

No problem. Here's a link to read to know how to reply to this list.
This isn't quite the same as most mailing lists because of the volume of
mail one receives here. (I get around 300 a day). So here's part of the
FAQ in sending to this list. One thing to notice is that you shouldn't
"top-post". That is, to reply above what you are replying to and leave
the entire message intact. ;-)

http://www.tux.org/lkml/#s3-9

-- Steve

2005-10-21 18:38:45

by Lee Revell

[permalink] [raw]
Subject: Re: question about code from the linux kernel development ( se ) book

On Fri, 2005-10-21 at 09:03 -0400, Steven Rostedt wrote:
> Oh God, please indent code examples, and if your email client strips white
> space, change your email client.

Or better yet PLEASE REPORT THIS AS A BUG IN YOUR EMAIL CLIENT.
Otherwise we're going to be plagued with this crap forever.

Lee