Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1362555 > unrolled thread

[PATCH 1/9] sched: add schedule_timeout_idle()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/9] sched: add schedule_timeout_idle() Michal Hocko <mhocko@kernel.org> - 2016-03-22 12:10 +0100
    Re: [PATCH 1/9] sched: add schedule_timeout_idle() Peter Zijlstra <peterz@infradead.org> - 2016-03-22 13:30 +0100
      Re: [PATCH 1/9] sched: add schedule_timeout_idle() Michal Hocko <mhocko@kernel.org> - 2016-03-22 13:40 +0100
        Re: [PATCH 1/9] sched: add schedule_timeout_idle() Peter Zijlstra <peterz@infradead.org> - 2016-03-22 14:00 +0100
          Re: [PATCH 1/9] sched: add schedule_timeout_idle() Michal Hocko <mhocko@kernel.org> - 2016-03-22 14:10 +0100
            Re: [PATCH 1/9] sched: add schedule_timeout_idle() Peter Zijlstra <peterz@infradead.org> - 2016-03-22 14:30 +0100
              Re: [PATCH 1/9] sched: add schedule_timeout_idle() Johannes Weiner <hannes@cmpxchg.org> - 2016-03-22 19:00 +0100
                Re: [PATCH 1/9] sched: add schedule_timeout_idle() Peter Zijlstra <peterz@infradead.org> - 2016-03-22 22:30 +0100
                  Re: [PATCH 1/9] sched: add schedule_timeout_idle() Michal Hocko <mhocko@kernel.org> - 2016-03-23 11:50 +0100
      Re: [PATCH 1/9] sched: add schedule_timeout_idle() Andrew Morton <akpm@linux-foundation.org> - 2016-03-23 00:10 +0100

#1362555 — [PATCH 1/9] sched: add schedule_timeout_idle()

From: Andrew Morton <akpm@linux-foundation.org>

This will be needed in the patch "mm, oom: introduce oom reaper".

Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/sched.h |  1 +
 kernel/time/timer.c   | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 084ed9fba620..9cf5731472fe 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -425,6 +425,7 @@ extern signed long schedule_timeout(signed long timeout);
 extern signed long schedule_timeout_interruptible(signed long timeout);
 extern signed long schedule_timeout_killable(signed long timeout);
 extern signed long schedule_timeout_uninterruptible(signed long timeout);
+extern signed long schedule_timeout_idle(signed long timeout);
 asmlinkage void schedule(void);
 extern void schedule_preempt_disabled(void);
 
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index d1798fa0c743..73164c3aa56b 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1566,6 +1566,17 @@ signed long __sched schedule_timeout_uninterruptible(signed long timeout)
 }
 EXPORT_SYMBOL(schedule_timeout_uninterruptible);
 
+/*
+ * Like schedule_timeout_uninterruptible(), except this task will not contribute
+ * to load average.
+ */
+signed long __sched schedule_timeout_idle(signed long timeout)
+{
+	__set_current_state(TASK_IDLE);
+	return schedule_timeout(timeout);
+}
+EXPORT_SYMBOL(schedule_timeout_idle);
+
 #ifdef CONFIG_HOTPLUG_CPU
 static void migrate_timer_list(struct tvec_base *new_base, struct hlist_head *head)
 {
-- 
2.7.0

[toc] | [next] | [standalone]

#1362692

On Tue, Mar 22, 2016 at 12:00:18PM +0100, Michal Hocko wrote:

>  extern signed long schedule_timeout_interruptible(signed long timeout);
>  extern signed long schedule_timeout_killable(signed long timeout);
>  extern signed long schedule_timeout_uninterruptible(signed long timeout);
> +extern signed long schedule_timeout_idle(signed long timeout);

> +/*
> + * Like schedule_timeout_uninterruptible(), except this task will not contribute
> + * to load average.
> + */
> +signed long __sched schedule_timeout_idle(signed long timeout)
> +{
> +	__set_current_state(TASK_IDLE);
> +	return schedule_timeout(timeout);
> +}
> +EXPORT_SYMBOL(schedule_timeout_idle);

Yes we have 3 such other wrappers, but I've gotta ask: why? They seem
pretty pointless.

Why not kill the lot?

[toc] | [prev] | [next] | [standalone]

#1362700

On Tue 22-03-16 13:23:45, Peter Zijlstra wrote:
> On Tue, Mar 22, 2016 at 12:00:18PM +0100, Michal Hocko wrote:
> 
> >  extern signed long schedule_timeout_interruptible(signed long timeout);
> >  extern signed long schedule_timeout_killable(signed long timeout);
> >  extern signed long schedule_timeout_uninterruptible(signed long timeout);
> > +extern signed long schedule_timeout_idle(signed long timeout);
> 
> > +/*
> > + * Like schedule_timeout_uninterruptible(), except this task will not contribute
> > + * to load average.
> > + */
> > +signed long __sched schedule_timeout_idle(signed long timeout)
> > +{
> > +	__set_current_state(TASK_IDLE);
> > +	return schedule_timeout(timeout);
> > +}
> > +EXPORT_SYMBOL(schedule_timeout_idle);
> 
> Yes we have 3 such other wrappers, but I've gotta ask: why? They seem
> pretty pointless.

It seems it is just too easy to miss the __set_current_state (I am
talking from my own experience). This also seems to be a pretty common
pattern so why not wrap it under a common call.

> Why not kill the lot?

We have over 400 users, would it be much better if we open code all of
them? It doesn't sound like a huge win to me.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1362707

On Tue, Mar 22, 2016 at 01:33:14PM +0100, Michal Hocko wrote:
> On Tue 22-03-16 13:23:45, Peter Zijlstra wrote:
> > On Tue, Mar 22, 2016 at 12:00:18PM +0100, Michal Hocko wrote:
> > 
> > >  extern signed long schedule_timeout_interruptible(signed long timeout);
> > >  extern signed long schedule_timeout_killable(signed long timeout);
> > >  extern signed long schedule_timeout_uninterruptible(signed long timeout);
> > > +extern signed long schedule_timeout_idle(signed long timeout);
> > 
> > > +/*
> > > + * Like schedule_timeout_uninterruptible(), except this task will not contribute
> > > + * to load average.
> > > + */
> > > +signed long __sched schedule_timeout_idle(signed long timeout)
> > > +{
> > > +	__set_current_state(TASK_IDLE);
> > > +	return schedule_timeout(timeout);
> > > +}
> > > +EXPORT_SYMBOL(schedule_timeout_idle);
> > 
> > Yes we have 3 such other wrappers, but I've gotta ask: why? They seem
> > pretty pointless.
> 
> It seems it is just too easy to miss the __set_current_state (I am
> talking from my own experience).

Well, that's what you get; if you call schedule() and forget to set a
blocking state you also don't block, where the problem?

> This also seems to be a pretty common
> pattern so why not wrap it under a common call.

It just seems extremely silly to create a (out-of-line even) function
for a store and a call.

> > Why not kill the lot?
> 
> We have over 400 users, would it be much better if we open code all of
> them? It doesn't sound like a huge win to me.

Dunno, changing them around isn't much work, we've got coccinelle for
that.

[toc] | [prev] | [next] | [standalone]

#1362712

On Tue 22-03-16 13:51:13, Peter Zijlstra wrote:
> On Tue, Mar 22, 2016 at 01:33:14PM +0100, Michal Hocko wrote:
> > On Tue 22-03-16 13:23:45, Peter Zijlstra wrote:
> > > On Tue, Mar 22, 2016 at 12:00:18PM +0100, Michal Hocko wrote:
> > > 
> > > >  extern signed long schedule_timeout_interruptible(signed long timeout);
> > > >  extern signed long schedule_timeout_killable(signed long timeout);
> > > >  extern signed long schedule_timeout_uninterruptible(signed long timeout);
> > > > +extern signed long schedule_timeout_idle(signed long timeout);
> > > 
> > > > +/*
> > > > + * Like schedule_timeout_uninterruptible(), except this task will not contribute
> > > > + * to load average.
> > > > + */
> > > > +signed long __sched schedule_timeout_idle(signed long timeout)
> > > > +{
> > > > +	__set_current_state(TASK_IDLE);
> > > > +	return schedule_timeout(timeout);
> > > > +}
> > > > +EXPORT_SYMBOL(schedule_timeout_idle);
> > > 
> > > Yes we have 3 such other wrappers, but I've gotta ask: why? They seem
> > > pretty pointless.
> > 
> > It seems it is just too easy to miss the __set_current_state (I am
> > talking from my own experience).
> 
> Well, that's what you get; if you call schedule() and forget to set a
> blocking state you also don't block, where the problem?

The error prone nature of schedule_timeout usage was the reason to
introduce them in the first place IIRC which makes me think this is
something that is not so uncommon.
 
[...]

> > > Why not kill the lot?
> > 
> > We have over 400 users, would it be much better if we open code all of
> > them? It doesn't sound like a huge win to me.
> 
> Dunno, changing them around isn't much work, we've got coccinelle for
> that.

If that sounds like a more appropriate plan I won't object. I can simply
change my patch to do __set_current_state and schedule_timeout.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1362740

On Tue, Mar 22, 2016 at 02:08:23PM +0100, Michal Hocko wrote:
> On Tue 22-03-16 13:51:13, Peter Zijlstra wrote:
> If that sounds like a more appropriate plan I won't object. I can simply
> change my patch to do __set_current_state and schedule_timeout.

I dunno, I just think these wrappers are silly.

[toc] | [prev] | [next] | [standalone]

#1362888

On Tue, Mar 22, 2016 at 02:22:49PM +0100, Peter Zijlstra wrote:
> On Tue, Mar 22, 2016 at 02:08:23PM +0100, Michal Hocko wrote:
> > On Tue 22-03-16 13:51:13, Peter Zijlstra wrote:
> > If that sounds like a more appropriate plan I won't object. I can simply
> > change my patch to do __set_current_state and schedule_timeout.
> 
> I dunno, I just think these wrappers are silly.

Adding out-of-line, exported wrappers for every single task state is
kind of silly. But it's still a common operation to wait in a certain
state, so having a single function for that makes sense. Kind of like
spin_lock_irqsave and friends.

Maybe this would be better?:

static inline long schedule_timeout_state(long timeout, long state)
{
	__set_current_state(state);
	return schedule_timeout(timeout);
}

[toc] | [prev] | [next] | [standalone]

#1363025

On Tue, Mar 22, 2016 at 01:56:26PM -0400, Johannes Weiner wrote:
> On Tue, Mar 22, 2016 at 02:22:49PM +0100, Peter Zijlstra wrote:
> > On Tue, Mar 22, 2016 at 02:08:23PM +0100, Michal Hocko wrote:
> > > On Tue 22-03-16 13:51:13, Peter Zijlstra wrote:
> > > If that sounds like a more appropriate plan I won't object. I can simply
> > > change my patch to do __set_current_state and schedule_timeout.
> > 
> > I dunno, I just think these wrappers are silly.
> 
> Adding out-of-line, exported wrappers for every single task state is
> kind of silly. But it's still a common operation to wait in a certain
> state, so having a single function for that makes sense. Kind of like
> spin_lock_irqsave and friends.
> 
> Maybe this would be better?:
> 
> static inline long schedule_timeout_state(long timeout, long state)
> {
> 	__set_current_state(state);
> 	return schedule_timeout(timeout);
> }

Probably. However, with such semantics the schedule*() name is wrong
too, you cannot use these functions to build actual wait loops etc.

So maybe:

static inline long sleep_in_state(long timeout, long state)
{
	__set_current_state(state);
	return schedule_timeout(timeout);
}

might be an even better name; but at that point we look very like the
msleep*() class of function, so maybe we should do:

long sleep_in_state(long state, long timeout)
{
	while (timeout && !signal_pending_state(state, current)) {
		__set_current_state(state);
		timeout = schedule_timeout(timeout);
	}
	return timeout;
}

Hmm ?

[toc] | [prev] | [next] | [standalone]

#1363325

On Tue 22-03-16 22:23:52, Peter Zijlstra wrote:
[...]
> Probably. However, with such semantics the schedule*() name is wrong
> too, you cannot use these functions to build actual wait loops etc.
> 
> So maybe:
> 
> static inline long sleep_in_state(long timeout, long state)
> {
> 	__set_current_state(state);
> 	return schedule_timeout(timeout);
> }
> 
> might be an even better name; but at that point we look very like the
> msleep*() class of function, so maybe we should do:
> 
> long sleep_in_state(long state, long timeout)
> {
> 	while (timeout && !signal_pending_state(state, current)) {
> 		__set_current_state(state);
> 		timeout = schedule_timeout(timeout);
> 	}
> 	return timeout;
> }
> 
> Hmm ?

I am not sure how many callers do care about premature wake-ups (e.g
I could find a use for it) but this indeed has a better and cleaner
semantic.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1363094

On Tue, 22 Mar 2016 13:23:45 +0100 Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Mar 22, 2016 at 12:00:18PM +0100, Michal Hocko wrote:
> 
> >  extern signed long schedule_timeout_interruptible(signed long timeout);
> >  extern signed long schedule_timeout_killable(signed long timeout);
> >  extern signed long schedule_timeout_uninterruptible(signed long timeout);
> > +extern signed long schedule_timeout_idle(signed long timeout);
> 
> > +/*
> > + * Like schedule_timeout_uninterruptible(), except this task will not contribute
> > + * to load average.
> > + */
> > +signed long __sched schedule_timeout_idle(signed long timeout)
> > +{
> > +	__set_current_state(TASK_IDLE);
> > +	return schedule_timeout(timeout);
> > +}
> > +EXPORT_SYMBOL(schedule_timeout_idle);
> 
> Yes we have 3 such other wrappers, but I've gotta ask: why? They seem
> pretty pointless.

I like the wrappers.  At least, more than having to read the open-coded
version.  The latter is just more stuff to interpret and to check
whereas I can look at "schedule_timeout_idle" and think "yup, I know
what that does".

But whatever.  I'll probably be sending this series up for 4.6 and we can
worry about the schedule_timeout_foo() stuff later.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web