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

Groups > linux.kernel > #1416535 > unrolled thread

[PATCH 0/2] sched/debug: more schedstats fixes

Started byJosh Poimboeuf <jpoimboe@redhat.com>
First post2016-06-07 21:50 +0200
Last post2016-06-08 16:30 +0200
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] sched/debug: more schedstats fixes Josh Poimboeuf <jpoimboe@redhat.com> - 2016-06-07 21:50 +0200
    [PATCH 1/2] sched/debug: fix 'schedstats=enable' cmdline option Josh Poimboeuf <jpoimboe@redhat.com> - 2016-06-07 21:50 +0200
      Re: [PATCH 1/2] sched/debug: fix 'schedstats=enable' cmdline option Mel Gorman <mgorman@techsingularity.net> - 2016-06-08 10:10 +0200
      [tip:sched/core] sched/debug: Fix 'schedstats=enable' cmdline  option tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2016-06-08 16:30 +0200

#1416535 — [PATCH 0/2] sched/debug: more schedstats fixes

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2016-06-07 21:50 +0200
Subject[PATCH 0/2] sched/debug: more schedstats fixes
Message-ID<rHvaG-6PS-13@gated-at.bofh.it>
Fixes for two more bugs I found in the schedstats code, in addition to
to the other patches I sent out:
  
  https://lkml.kernel.org/r/cover.1464994423.git.jpoimboe@redhat.com

Josh Poimboeuf (2):
  sched/debug: fix 'schedstats=enable' cmdline option
  sched/debug: fix deadlock when enabling sched events

 include/trace/events/sched.h | 30 +++++++++++++++---------------
 kernel/sched/core.c          | 26 +++++++++++++++++++++-----
 kernel/sched/fair.c          | 29 +++++++----------------------
 3 files changed, 43 insertions(+), 42 deletions(-)

-- 
2.4.11

[toc] | [next] | [standalone]

#1416538 — [PATCH 1/2] sched/debug: fix 'schedstats=enable' cmdline option

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2016-06-07 21:50 +0200
Subject[PATCH 1/2] sched/debug: fix 'schedstats=enable' cmdline option
Message-ID<rHvaG-6PS-29@gated-at.bofh.it>
In reply to#1416535
The 'schedstats=enable' option doesn't work, and also produces the
following warning during boot:

  WARNING: CPU: 0 PID: 0 at /home/jpoimboe/git/linux/kernel/jump_label.c:61 static_key_slow_inc+0x8c/0xa0
  static_key_slow_inc used before call to jump_label_init
  Modules linked in:
  CPU: 0 PID: 0 Comm: swapper Not tainted 4.7.0-rc1+ #25
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
   0000000000000086 3ae3475a4bea95d4 ffffffff81e03da8 ffffffff8143fc83
   ffffffff81e03df8 0000000000000000 ffffffff81e03de8 ffffffff810b1ffb
   0000003d00000096 ffffffff823514d0 ffff88007ff197c8 0000000000000000
  Call Trace:
   [<ffffffff8143fc83>] dump_stack+0x85/0xc2
   [<ffffffff810b1ffb>] __warn+0xcb/0xf0
   [<ffffffff810b207f>] warn_slowpath_fmt+0x5f/0x80
   [<ffffffff811e9c0c>] static_key_slow_inc+0x8c/0xa0
   [<ffffffff810e07c6>] static_key_enable+0x16/0x40
   [<ffffffff8216d633>] setup_schedstats+0x29/0x94
   [<ffffffff82148a05>] unknown_bootoption+0x89/0x191
   [<ffffffff810d8617>] parse_args+0x297/0x4b0
   [<ffffffff82148d61>] start_kernel+0x1d8/0x4a9
   [<ffffffff8214897c>] ? set_init_arg+0x55/0x55
   [<ffffffff82148120>] ? early_idt_handler_array+0x120/0x120
   [<ffffffff821482db>] x86_64_start_reservations+0x2f/0x31
   [<ffffffff82148427>] x86_64_start_kernel+0x14a/0x16d

The problem is that it tries to update the 'sched_schedstats' static key
before jump labels have been initialized.

Changing jump_label_init() to be called earlier before
parse_early_param() wouldn't fix it: it would still fail trying to
poke_text() because mm isn't yet initialized.

Instead, just create a temporary '__sched_schedstats' variable which can
be copied to the static key later during sched_init() after jump labels
have been initialized.

Fixes: cb2517653fcc ("sched/debug: Make schedstats a runtime tunable that is disabled by default")
Cc: stable@vger.kernel.org
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 kernel/sched/core.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 7f2cae4..385c947 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2253,9 +2253,11 @@ int sysctl_numa_balancing(struct ctl_table *table, int write,
 #endif
 #endif
 
+#ifdef CONFIG_SCHEDSTATS
+
 DEFINE_STATIC_KEY_FALSE(sched_schedstats);
+static bool __initdata __sched_schedstats = false;
 
-#ifdef CONFIG_SCHEDSTATS
 static void set_schedstats(bool enabled)
 {
 	if (enabled)
@@ -2278,11 +2280,16 @@ static int __init setup_schedstats(char *str)
 	if (!str)
 		goto out;
 
+	/*
+	 * This code is called before jump labels have been set up, so we can't
+	 * change the static branch directly just yet.  Instead set a temporary
+	 * variable so init_schedstats() can do it later.
+	 */
 	if (!strcmp(str, "enable")) {
-		set_schedstats(true);
+		__sched_schedstats = true;
 		ret = 1;
 	} else if (!strcmp(str, "disable")) {
-		set_schedstats(false);
+		__sched_schedstats = false;
 		ret = 1;
 	}
 out:
@@ -2293,6 +2300,11 @@ out:
 }
 __setup("schedstats=", setup_schedstats);
 
+static void __init init_schedstats(void)
+{
+	set_schedstats(__sched_schedstats);
+}
+
 #ifdef CONFIG_PROC_SYSCTL
 int sysctl_schedstats(struct ctl_table *table, int write,
 			 void __user *buffer, size_t *lenp, loff_t *ppos)
@@ -2313,8 +2325,10 @@ int sysctl_schedstats(struct ctl_table *table, int write,
 		set_schedstats(state);
 	return err;
 }
-#endif
-#endif
+#endif /* CONFIG_PROC_SYSCTL */
+#else  /* !CONFIG_SCHEDSTATS */
+static inline void init_schedstats(void) {}
+#endif /* CONFIG_SCHEDSTATS */
 
 /*
  * fork()/clone()-time setup:
@@ -7487,6 +7501,8 @@ void __init sched_init(void)
 #endif
 	init_sched_fair_class();
 
+	init_schedstats();
+
 	scheduler_running = 1;
 }
 
-- 
2.4.11

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

#1417011 — Re: [PATCH 1/2] sched/debug: fix 'schedstats=enable' cmdline option

FromMel Gorman <mgorman@techsingularity.net>
Date2016-06-08 10:10 +0200
SubjectRe: [PATCH 1/2] sched/debug: fix 'schedstats=enable' cmdline option
Message-ID<rHGIN-64G-3@gated-at.bofh.it>
In reply to#1416538
On Tue, Jun 07, 2016 at 02:43:16PM -0500, Josh Poimboeuf wrote:
> The 'schedstats=enable' option doesn't work, and also produces the
> following warning during boot:
> 
>   WARNING: CPU: 0 PID: 0 at /home/jpoimboe/git/linux/kernel/jump_label.c:61 static_key_slow_inc+0x8c/0xa0
>   static_key_slow_inc used before call to jump_label_init
>   Modules linked in:
>   CPU: 0 PID: 0 Comm: swapper Not tainted 4.7.0-rc1+ #25
>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
>    0000000000000086 3ae3475a4bea95d4 ffffffff81e03da8 ffffffff8143fc83
>    ffffffff81e03df8 0000000000000000 ffffffff81e03de8 ffffffff810b1ffb
>    0000003d00000096 ffffffff823514d0 ffff88007ff197c8 0000000000000000
>   Call Trace:
>    [<ffffffff8143fc83>] dump_stack+0x85/0xc2
>    [<ffffffff810b1ffb>] __warn+0xcb/0xf0
>    [<ffffffff810b207f>] warn_slowpath_fmt+0x5f/0x80
>    [<ffffffff811e9c0c>] static_key_slow_inc+0x8c/0xa0
>    [<ffffffff810e07c6>] static_key_enable+0x16/0x40
>    [<ffffffff8216d633>] setup_schedstats+0x29/0x94
>    [<ffffffff82148a05>] unknown_bootoption+0x89/0x191
>    [<ffffffff810d8617>] parse_args+0x297/0x4b0
>    [<ffffffff82148d61>] start_kernel+0x1d8/0x4a9
>    [<ffffffff8214897c>] ? set_init_arg+0x55/0x55
>    [<ffffffff82148120>] ? early_idt_handler_array+0x120/0x120
>    [<ffffffff821482db>] x86_64_start_reservations+0x2f/0x31
>    [<ffffffff82148427>] x86_64_start_kernel+0x14a/0x16d
> 
> The problem is that it tries to update the 'sched_schedstats' static key
> before jump labels have been initialized.
> 

Odd, I thought that particular one had been addressed. At least I saw
errors like that in other areas before and they got fixed later by ftrace.

> Changing jump_label_init() to be called earlier before
> parse_early_param() wouldn't fix it: it would still fail trying to
> poke_text() because mm isn't yet initialized.
> 
> Instead, just create a temporary '__sched_schedstats' variable which can
> be copied to the static key later during sched_init() after jump labels
> have been initialized.
> 
> Fixes: cb2517653fcc ("sched/debug: Make schedstats a runtime tunable that is disabled by default")
> Cc: stable@vger.kernel.org
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  kernel/sched/core.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 7f2cae4..385c947 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2253,9 +2253,11 @@ int sysctl_numa_balancing(struct ctl_table *table, int write,
>  #endif
>  #endif
>  
> +#ifdef CONFIG_SCHEDSTATS
> +
>  DEFINE_STATIC_KEY_FALSE(sched_schedstats);
> +static bool __initdata __sched_schedstats = false;
>  

Initialisation is unnecessary. Otherwise;

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

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

#1417502 — [tip:sched/core] sched/debug: Fix 'schedstats=enable' cmdline option

Fromtip-bot for Josh Poimboeuf <tipbot@zytor.com>
Date2016-06-08 16:30 +0200
Subject[tip:sched/core] sched/debug: Fix 'schedstats=enable' cmdline option
Message-ID<rHMEx-1il-1@gated-at.bofh.it>
In reply to#1416538
Commit-ID:  4698f88c06b893f2acc0b443004a53bf490fde7c
Gitweb:     http://git.kernel.org/tip/4698f88c06b893f2acc0b443004a53bf490fde7c
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Tue, 7 Jun 2016 14:43:16 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 8 Jun 2016 14:33:05 +0200

sched/debug: Fix 'schedstats=enable' cmdline option

The 'schedstats=enable' option doesn't work, and also produces the
following warning during boot:

  WARNING: CPU: 0 PID: 0 at /home/jpoimboe/git/linux/kernel/jump_label.c:61 static_key_slow_inc+0x8c/0xa0
  static_key_slow_inc used before call to jump_label_init
  Modules linked in:
  CPU: 0 PID: 0 Comm: swapper Not tainted 4.7.0-rc1+ #25
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
   0000000000000086 3ae3475a4bea95d4 ffffffff81e03da8 ffffffff8143fc83
   ffffffff81e03df8 0000000000000000 ffffffff81e03de8 ffffffff810b1ffb
   0000003d00000096 ffffffff823514d0 ffff88007ff197c8 0000000000000000
  Call Trace:
   [<ffffffff8143fc83>] dump_stack+0x85/0xc2
   [<ffffffff810b1ffb>] __warn+0xcb/0xf0
   [<ffffffff810b207f>] warn_slowpath_fmt+0x5f/0x80
   [<ffffffff811e9c0c>] static_key_slow_inc+0x8c/0xa0
   [<ffffffff810e07c6>] static_key_enable+0x16/0x40
   [<ffffffff8216d633>] setup_schedstats+0x29/0x94
   [<ffffffff82148a05>] unknown_bootoption+0x89/0x191
   [<ffffffff810d8617>] parse_args+0x297/0x4b0
   [<ffffffff82148d61>] start_kernel+0x1d8/0x4a9
   [<ffffffff8214897c>] ? set_init_arg+0x55/0x55
   [<ffffffff82148120>] ? early_idt_handler_array+0x120/0x120
   [<ffffffff821482db>] x86_64_start_reservations+0x2f/0x31
   [<ffffffff82148427>] x86_64_start_kernel+0x14a/0x16d

The problem is that it tries to update the 'sched_schedstats' static key
before jump labels have been initialized.

Changing jump_label_init() to be called earlier before
parse_early_param() wouldn't fix it: it would still fail trying to
poke_text() because mm isn't yet initialized.

Instead, just create a temporary '__sched_schedstats' variable which can
be copied to the static key later during sched_init() after jump labels
have been initialized.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: cb2517653fcc ("sched/debug: Make schedstats a runtime tunable that is disabled by default")
Link: http://lkml.kernel.org/r/453775fe3433bed65731a583e228ccea806d18cd.1465322027.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/core.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 7f2cae4..385c947 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2253,9 +2253,11 @@ int sysctl_numa_balancing(struct ctl_table *table, int write,
 #endif
 #endif
 
+#ifdef CONFIG_SCHEDSTATS
+
 DEFINE_STATIC_KEY_FALSE(sched_schedstats);
+static bool __initdata __sched_schedstats = false;
 
-#ifdef CONFIG_SCHEDSTATS
 static void set_schedstats(bool enabled)
 {
 	if (enabled)
@@ -2278,11 +2280,16 @@ static int __init setup_schedstats(char *str)
 	if (!str)
 		goto out;
 
+	/*
+	 * This code is called before jump labels have been set up, so we can't
+	 * change the static branch directly just yet.  Instead set a temporary
+	 * variable so init_schedstats() can do it later.
+	 */
 	if (!strcmp(str, "enable")) {
-		set_schedstats(true);
+		__sched_schedstats = true;
 		ret = 1;
 	} else if (!strcmp(str, "disable")) {
-		set_schedstats(false);
+		__sched_schedstats = false;
 		ret = 1;
 	}
 out:
@@ -2293,6 +2300,11 @@ out:
 }
 __setup("schedstats=", setup_schedstats);
 
+static void __init init_schedstats(void)
+{
+	set_schedstats(__sched_schedstats);
+}
+
 #ifdef CONFIG_PROC_SYSCTL
 int sysctl_schedstats(struct ctl_table *table, int write,
 			 void __user *buffer, size_t *lenp, loff_t *ppos)
@@ -2313,8 +2325,10 @@ int sysctl_schedstats(struct ctl_table *table, int write,
 		set_schedstats(state);
 	return err;
 }
-#endif
-#endif
+#endif /* CONFIG_PROC_SYSCTL */
+#else  /* !CONFIG_SCHEDSTATS */
+static inline void init_schedstats(void) {}
+#endif /* CONFIG_SCHEDSTATS */
 
 /*
  * fork()/clone()-time setup:
@@ -7487,6 +7501,8 @@ void __init sched_init(void)
 #endif
 	init_sched_fair_class();
 
+	init_schedstats();
+
 	scheduler_running = 1;
 }

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web