[KJ] do all schedule_timeout() calls need to set state first?

[KJ] do all schedule_timeout() calls need to set state first?

[Robert P. J. Day]

  based on what i've been reading, all calls to schedule_timeout()
*must* first set the current process state to either
TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE, is that correct?

  since i was once again perusing the source tree and noticed the
following in drivers/hwmon/ams/ams-i2c.c (line 93):

        while (remaining) {
                result = ams_i2c_read(AMS_COMMAND);
                if (result = 0 || result & 0x80)
                        return 0;

                remaining = schedule_timeout(remaining);
        }

it certainly *seems* that the state isn't being set here.  is that an
error?  just curious.

rday

-- 
====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
====================================
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ] do all schedule_timeout() calls need to set state first?

[Nishanth Aravamudan]

On 19.03.2007 [14:00:53 -0400], Robert P. J. Day wrote:
> 
>   based on what i've been reading, all calls to schedule_timeout()
> *must* first set the current process state to either
> TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE, is that correct?

Yes, but note that setting it directly before is not always necessary,
in particular when used with wait-queues. But, take a look at the
comment heading schedule_timeout():

 * Make the current task sleep until @timeout jiffies have elapsed. The
 * routine will return immediately unless the current task state has
 * been set (see set_current_state()).

>   since i was once again perusing the source tree and noticed the
> following in drivers/hwmon/ams/ams-i2c.c (line 93):
> 
>         while (remaining) {
>                 result = ams_i2c_read(AMS_COMMAND);
>                 if (result = 0 || result & 0x80)
>                         return 0;
> 
>                 remaining = schedule_timeout(remaining);
>         }
> 
> it certainly *seems* that the state isn't being set here.  is that an
> error?  just curious.

Yes, it is. That is why we have schedule_timeout_interruptible() and
schedule_timeout_uninterruptible().

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ] do all schedule_timeout() calls need to set state first?

[Robert P. J. Day]

On Mon, 19 Mar 2007, Nishanth Aravamudan wrote:

> On 19.03.2007 [14:00:53 -0400], Robert P. J. Day wrote:
> >
> >   based on what i've been reading, all calls to schedule_timeout()
> > *must* first set the current process state to either
> > TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE, is that correct?
>
> Yes, but note that setting it directly before is not always
> necessary, in particular when used with wait-queues.

right, i realized that.  but it does have to be set at *some* point
before invoking that routine.

> But, take a look at the comment heading schedule_timeout():
>
>  * Make the current task sleep until @timeout jiffies have elapsed. The
>  * routine will return immediately unless the current task state has
>  * been set (see set_current_state()).
>
> >   since i was once again perusing the source tree and noticed the
> > following in drivers/hwmon/ams/ams-i2c.c (line 93):
> >
> >         while (remaining) {
> >                 result = ams_i2c_read(AMS_COMMAND);
> >                 if (result = 0 || result & 0x80)
> >                         return 0;
> >
> >                 remaining = schedule_timeout(remaining);
> >         }
> >
> > it certainly *seems* that the state isn't being set here.  is that an
> > error?  just curious.
>
> Yes, it is.

as in, yes, it is an error?  (i'm *assuming* that's what you meant
here.)

> That is why we have schedule_timeout_interruptible() and
> schedule_timeout_uninterruptible().

i'd noticed those -- does that mean that there is no need to ever call
schedule_timeout(), and that one should always use one of those two
routines above?  or am i misreading this?

rday
-- 
====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
====================================
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ] do all schedule_timeout() calls need to set state first?

[Nishanth Aravamudan]

On 19.03.2007 [15:47:06 -0400], Robert P. J. Day wrote:
> On Mon, 19 Mar 2007, Nishanth Aravamudan wrote:
> 
> > On 19.03.2007 [14:00:53 -0400], Robert P. J. Day wrote:
> > >
> > >   based on what i've been reading, all calls to schedule_timeout()
> > > *must* first set the current process state to either
> > > TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE, is that correct?
> >
> > Yes, but note that setting it directly before is not always
> > necessary, in particular when used with wait-queues.
> 
> right, i realized that.  but it does have to be set at *some* point
> before invoking that routine.

Yes. Presuming the intent is to actually schedule a timeout. I could
conceive of some (brain-dead, perhaps) code that would either sleep or
not, depending on some condition, and thus may not change its state
before calling schedule_timeout. Last time I looked, though, there was
no such code in the tree (and I would discourage it if I saw it).

> > But, take a look at the comment heading schedule_timeout():
> >
> >  * Make the current task sleep until @timeout jiffies have elapsed. The
> >  * routine will return immediately unless the current task state has
> >  * been set (see set_current_state()).
> >
> > >   since i was once again perusing the source tree and noticed the
> > > following in drivers/hwmon/ams/ams-i2c.c (line 93):
> > >
> > >         while (remaining) {
> > >                 result = ams_i2c_read(AMS_COMMAND);
> > >                 if (result = 0 || result & 0x80)
> > >                         return 0;
> > >
> > >                 remaining = schedule_timeout(remaining);
> > >         }
> > >
> > > it certainly *seems* that the state isn't being set here.  is that an
> > > error?  just curious.
> >
> > Yes, it is.
> 
> as in, yes, it is an error?  (i'm *assuming* that's what you meant
> here.)

Yes.

> > That is why we have schedule_timeout_interruptible() and
> > schedule_timeout_uninterruptible().
> 
> i'd noticed those -- does that mean that there is no need to ever call
> schedule_timeout(), and that one should always use one of those two
> routines above?  or am i misreading this?

Well, there are cases where you don't want to call the
{un,}interruptible versions. wait-queues, I mentioned earlier. And
sometimes, the task is put in a particular state, does some work and
then sleeps. It's not always clear that the state is important during
that work but rather than risk breaking something, I usually left those
alone.

In general, though, you are right, you shouldn't need to call
schedule_timeout(). In the wait-queue case, for instance, you could just
use wait_event_timeout().

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors