restart_syscall: use freezable blocking call

restart_syscall: use freezable blocking call

[Maninder Singh]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 2721 bytes --]

Hello All, 

Avoid waking up every thread sleeping in a restart_syscall call during suspend and resume by calling a freezable blocking call. 
Previous patches modified the freezer to avoid sending wakeups to threads that are blocked in freezable blocking calls.
Ref: https://lkml.org/lkml/2013/5/1/424
We have faced similiar issue of sleep fail when calling restart() system call in stress testing.

Patch :-
Subject: restart_syscall: use freezable blocking call
Avoid waking up every thread sleeping in a restart_syscall call during
suspend and resume by calling a freezable blocking call.  Previous
patches modified the freezer to avoid sending wakeups to threads
that are blocked in freezable blocking calls.
Ref: https://lkml.org/lkml/2013/5/1/424
Backtrace: 
[<c03e3924>] (__schedule+0x0/0x5d8) from [<c03e3f88>] (schedule+0x8c/0x90)
[<c03e3efc>] (schedule+0x0/0x90) from [<c03e3150>] (schedule_hrtimeout_range_clock+0xdc/0x110)
[<c03e3074>] (schedule_hrtimeout_range_clock+0x0/0x110) from [<c03e31a0>] (schedule_hrtimeout_range+0x1c/0x20)
 r9:d16c9be0 r8:8b7d9c2c r7:00000000 r6:00000000 r5:d16c8028
[<c03e3184>] (schedule_hrtimeout_range+0x0/0x20) from [<c015778c>] (poll_schedule_timeout+0x48/0x6c)
[<c0157744>] (poll_schedule_timeout+0x0/0x6c) from [<c0158994>] (do_sys_poll+0x2c8/0x378)
 r5:d16c9f78 r4:00000000
[<c01586cc>] (do_sys_poll+0x0/0x378) from [<c0158a84>] (do_restart_poll+0x40/0x5c)
[<c0158a44>] (do_restart_poll+0x0/0x5c) from [<c005710c>] (sys_restart_syscall+0x2c/0x30)
 r4:fffffe7a
[<c00570e0>] (sys_restart_syscall+0x0/0x30) from [<c001a180>] (ret_fast_syscall+0x0/0x48)
esTask03        R running      0   270    204 0x00000001
Signed-off-by: Yogesh Gaur <yn.gaur <at> samsung.com>
Signed-off-by: Amit Arora <amit.arora <at> samsung.com>
Reviewed-by : Ajeet Yadav <ajeet.y <at> samsung.com>
---
 kernel/time/hrtimer.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 37e50aa..75ebc3f 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1775,7 +1775,7 @@ schedule_hrtimeout_range_clock(ktime_t *expires, unsigned long delta,
   * A NULL parameter means "infinite"
   */
  if (!expires) {
-  schedule();
+  freezable_schedule();
   return -EINTR;
  }
 
@@ -1789,7 +1789,7 @@ schedule_hrtimeout_range_clock(ktime_t *expires, unsigned long delta,
   t.task = NULL;
 
  if (likely(t.task))
-  schedule();
+  freezable_schedule();
 
  hrtimer_cancel(&t.timer);
  destroy_hrtimer_on_stack(&t.timer);
-- 
1.7.1




Thanks and Regards,
Maninder Singhÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: restart_syscall: use freezable blocking call

[Andrew Morton]

On Mon, 30 Mar 2015 11:07:35 +0000 (GMT) Maninder Singh <maninder1.s@samsung.com> wrote:

> Hello All, 
> 
> Avoid waking up every thread sleeping in a restart_syscall call during suspend and resume by calling a freezable blocking call. 
> Previous patches modified the freezer to avoid sending wakeups to threads that are blocked in freezable blocking calls.
> Ref: https://lkml.org/lkml/2013/5/1/424
> We have faced similiar issue of sleep fail when calling restart() system call in stress testing.
> 
> Patch :-
> Subject: restart_syscall: use freezable blocking call
> Avoid waking up every thread sleeping in a restart_syscall call during
> suspend and resume by calling a freezable blocking call.  Previous
> patches modified the freezer to avoid sending wakeups to threads
> that are blocked in freezable blocking calls.

This patch is rather mucked up - tabs replaced with spaces, apparently
mangled text in the changelog.

Thomas is having a bit of time off, so I fixed most of that up and
queued the patch in -mm for some testing.  Hopefully Thomas and/or
Rafael will be able to comment?


From: Maninder Singh <maninder1.s@samsung.com>
Subject: kernel/time/hrtimer.c: restart_syscall: use freezable blocking call

Avoid waking up every thread sleeping in a restart_syscall call during
suspend and resume by calling a freezable blocking call.  Previous patches
modified the freezer to avoid sending wakeups to threads that are blocked
in freezable blocking calls.

Ref: https://lkml.org/lkml/2013/5/1/424

Backtrace: 
[<c03e3924>] (__schedule+0x0/0x5d8) from [<c03e3f88>] (schedule+0x8c/0x90)
[<c03e3efc>] (schedule+0x0/0x90) from [<c03e3150>] (schedule_hrtimeout_range_clock+0xdc/0x110)
[<c03e3074>] (schedule_hrtimeout_range_clock+0x0/0x110) from [<c03e31a0>] (schedule_hrtimeout_range+0x1c/0x20)
 r9:d16c9be0 r8:8b7d9c2c r7:00000000 r6:00000000 r5:d16c8028
[<c03e3184>] (schedule_hrtimeout_range+0x0/0x20) from [<c015778c>] (poll_schedule_timeout+0x48/0x6c)
[<c0157744>] (poll_schedule_timeout+0x0/0x6c) from [<c0158994>] (do_sys_poll+0x2c8/0x378) r5:d16c9f78 r4:00000000
[<c01586cc>] (do_sys_poll+0x0/0x378) from [<c0158a84>] (do_restart_poll+0x40/0x5c)
[<c0158a44>] (do_restart_poll+0x0/0x5c) from [<c005710c>] (sys_restart_syscall+0x2c/0x30) r4:fffffe7a
[<c00570e0>] (sys_restart_syscall+0x0/0x30) from [<c001a180>] (ret_fast_syscall+0x0/0x48)


Signed-off-by: Yogesh Gaur <yn.gaur <at> samsung.com>
Signed-off-by: Amit Arora <amit.arora <at> samsung.com>
Reviewed-by : Ajeet Yadav <ajeet.y <at> samsung.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/time/hrtimer.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN kernel/time/hrtimer.c~restart_syscall-use-freezable-blocking-call kernel/time/hrtimer.c
--- a/kernel/time/hrtimer.c~restart_syscall-use-freezable-blocking-call
+++ a/kernel/time/hrtimer.c
@@ -1767,7 +1767,7 @@ schedule_hrtimeout_range_clock(ktime_t *
 	 * A NULL parameter means "infinite"
 	 */
 	if (!expires) {
-		schedule();
+		freezable_schedule();
 		return -EINTR;
 	}
 
@@ -1781,7 +1781,7 @@ schedule_hrtimeout_range_clock(ktime_t *
 		t.task = NULL;
 
 	if (likely(t.task))
-		schedule();
+		freezable_schedule();
 
 	hrtimer_cancel(&t.timer);
 	destroy_hrtimer_on_stack(&t.timer);
_

Re: restart_syscall: use freezable blocking call

[Rafael J. Wysocki]

CC more maintainers.

On Monday, March 30, 2015 03:43:51 PM Andrew Morton wrote:
> On Mon, 30 Mar 2015 11:07:35 +0000 (GMT) Maninder Singh <maninder1.s@samsung.com> wrote:
> 
> > Hello All, 
> > 
> > Avoid waking up every thread sleeping in a restart_syscall call during suspend and resume by calling a freezable blocking call. 
> > Previous patches modified the freezer to avoid sending wakeups to threads that are blocked in freezable blocking calls.
> > Ref: https://lkml.org/lkml/2013/5/1/424
> > We have faced similiar issue of sleep fail when calling restart() system call in stress testing.
> > 
> > Patch :-
> > Subject: restart_syscall: use freezable blocking call
> > Avoid waking up every thread sleeping in a restart_syscall call during
> > suspend and resume by calling a freezable blocking call.  Previous
> > patches modified the freezer to avoid sending wakeups to threads
> > that are blocked in freezable blocking calls.
> 
> This patch is rather mucked up - tabs replaced with spaces, apparently
> mangled text in the changelog.
> 
> Thomas is having a bit of time off, so I fixed most of that up and
> queued the patch in -mm for some testing.  Hopefully Thomas and/or
> Rafael will be able to comment?
> 
> 
> From: Maninder Singh <maninder1.s@samsung.com>
> Subject: kernel/time/hrtimer.c: restart_syscall: use freezable blocking call
> 
> Avoid waking up every thread sleeping in a restart_syscall call during
> suspend and resume by calling a freezable blocking call.  Previous patches
> modified the freezer to avoid sending wakeups to threads that are blocked
> in freezable blocking calls.
> 
> Ref: https://lkml.org/lkml/2013/5/1/424
> 
> Backtrace: 
> [<c03e3924>] (__schedule+0x0/0x5d8) from [<c03e3f88>] (schedule+0x8c/0x90)
> [<c03e3efc>] (schedule+0x0/0x90) from [<c03e3150>] (schedule_hrtimeout_range_clock+0xdc/0x110)
> [<c03e3074>] (schedule_hrtimeout_range_clock+0x0/0x110) from [<c03e31a0>] (schedule_hrtimeout_range+0x1c/0x20)
>  r9:d16c9be0 r8:8b7d9c2c r7:00000000 r6:00000000 r5:d16c8028
> [<c03e3184>] (schedule_hrtimeout_range+0x0/0x20) from [<c015778c>] (poll_schedule_timeout+0x48/0x6c)
> [<c0157744>] (poll_schedule_timeout+0x0/0x6c) from [<c0158994>] (do_sys_poll+0x2c8/0x378) r5:d16c9f78 r4:00000000
> [<c01586cc>] (do_sys_poll+0x0/0x378) from [<c0158a84>] (do_restart_poll+0x40/0x5c)
> [<c0158a44>] (do_restart_poll+0x0/0x5c) from [<c005710c>] (sys_restart_syscall+0x2c/0x30) r4:fffffe7a
> [<c00570e0>] (sys_restart_syscall+0x0/0x30) from [<c001a180>] (ret_fast_syscall+0x0/0x48)
> 
> 
> Signed-off-by: Yogesh Gaur <yn.gaur <at> samsung.com>
> Signed-off-by: Amit Arora <amit.arora <at> samsung.com>
> Reviewed-by : Ajeet Yadav <ajeet.y <at> samsung.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  kernel/time/hrtimer.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff -puN kernel/time/hrtimer.c~restart_syscall-use-freezable-blocking-call kernel/time/hrtimer.c
> --- a/kernel/time/hrtimer.c~restart_syscall-use-freezable-blocking-call
> +++ a/kernel/time/hrtimer.c
> @@ -1767,7 +1767,7 @@ schedule_hrtimeout_range_clock(ktime_t *
>  	 * A NULL parameter means "infinite"
>  	 */
>  	if (!expires) {
> -		schedule();
> +		freezable_schedule();
>  		return -EINTR;
>  	}
>  
> @@ -1781,7 +1781,7 @@ schedule_hrtimeout_range_clock(ktime_t *
>  		t.task = NULL;
>  
>  	if (likely(t.task))
> -		schedule();
> +		freezable_schedule();
>  
>  	hrtimer_cancel(&t.timer);
>  	destroy_hrtimer_on_stack(&t.timer);
> _
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: restart_syscall: use freezable blocking call

[Peter Zijlstra]

On Tue, Mar 31, 2015 at 01:39:07AM +0200, Rafael J. Wysocki wrote:
> > From: Maninder Singh <maninder1.s@samsung.com>
> > Subject: kernel/time/hrtimer.c: restart_syscall: use freezable blocking call
> > 
> > Avoid waking up every thread sleeping in a restart_syscall call during
> > suspend and resume by calling a freezable blocking call.  Previous patches
> > modified the freezer to avoid sending wakeups to threads that are blocked
> > in freezable blocking calls.

This is what, no why mentioned.

> > 
> > Signed-off-by: Yogesh Gaur <yn.gaur <at> samsung.com>
> > Signed-off-by: Amit Arora <amit.arora <at> samsung.com>

This SoB chain is garbage. Most important, note that the author (From by
lack of Author) didn't actually sign off on it.

> > Reviewed-by : Ajeet Yadav <ajeet.y <at> samsung.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > 
> >  kernel/time/hrtimer.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff -puN kernel/time/hrtimer.c~restart_syscall-use-freezable-blocking-call kernel/time/hrtimer.c
> > --- a/kernel/time/hrtimer.c~restart_syscall-use-freezable-blocking-call
> > +++ a/kernel/time/hrtimer.c
> > @@ -1767,7 +1767,7 @@ schedule_hrtimeout_range_clock(ktime_t *
> >  	 * A NULL parameter means "infinite"
> >  	 */
> >  	if (!expires) {
> > -		schedule();
> > +		freezable_schedule();
> >  		return -EINTR;
> >  	}
> >  
> > @@ -1781,7 +1781,7 @@ schedule_hrtimeout_range_clock(ktime_t *
> >  		t.task = NULL;
> >  
> >  	if (likely(t.task))
> > -		schedule();
> > +		freezable_schedule();
> >  
> >  	hrtimer_cancel(&t.timer);
> >  	destroy_hrtimer_on_stack(&t.timer);

Not a single word on why this would be correct. At the very least show
us you've thought about it.

Re: restart_syscall: use freezable blocking call

[Sasha Levin]

On 03/31/2015 11:07 AM, Peter Zijlstra wrote:
>>> @@ -1781,7 +1781,7 @@ schedule_hrtimeout_range_clock(ktime_t *
>>> > >  		t.task = NULL;
>>> > >  
>>> > >  	if (likely(t.task))
>>> > > -		schedule();
>>> > > +		freezable_schedule();
>>> > >  
>>> > >  	hrtimer_cancel(&t.timer);
>>> > >  	destroy_hrtimer_on_stack(&t.timer);
> Not a single word on why this would be correct. At the very least show
> us you've thought about it.

It's not correct.

[    0.634248] =====================================
[    0.635011] [ BUG: swapper/0/1 still has locks held! ]
[    0.635566] 4.0.0-rc6-next-20150402-sasha-00038-g3637da5 #2121 Not tainted
[    0.636009] -------------------------------------
[    0.636508] 2 locks held by swapper/0/1:
[    0.637008] #0: (cpu_hotplug.lock){.+.+.+}, at: get_online_cpus (kernel/cpu.c:96)
[    0.638009] #1: (smpboot_threads_lock){+.+.+.}, at: smpboot_register_percpu_thread (kernel/smpboot.c:286)
[    0.639013] Mutex: counter: 0 owner: swapper/0
[    0.640010]
[    0.640010] stack backtrace:
[    0.640543] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.0.0-rc6-next-20150402-sasha-00038-g3637da5 #2121
[    0.641012]  ffff8809e2748000 00000000d284eea5 ffff88004e8df948 ffffffff9dabb4a2
[    0.642622]  0000000000000000 ffff8809e2748000 ffff88004e8df968 ffffffff932e848d
[    0.643627]  ffff88004e8df9a8 ffff88004e8dfa48 ffff88004e8dfa78 ffffffff9db2e57c
[    0.644624] Call Trace:
[    0.645021] dump_stack (lib/dump_stack.c:52)
[    0.645644] debug_check_no_locks_held (kernel/locking/lockdep.c:4178)
[    0.646019] schedule_hrtimeout_range_clock (include/linux/freezer.h:56 include/linux/freezer.h:66 include/linux/freezer.h:127 include/linux/freezer.h:172 kernel/time/hrtimer.c:1784)
[    0.647017] ? hrtimer_nanosleep_restart (kernel/time/hrtimer.c:1754)
[    0.648015] ? get_lock_stats (kernel/locking/lockdep.c:249)
[    0.648702] ? hrtimer_get_res (kernel/time/hrtimer.c:1472)
[    0.649016] ? schedule_hrtimeout_range_clock (kernel/time/hrtimer.c:1780)
[    0.650020] ? __this_cpu_preempt_check (lib/smp_processor_id.c:63)
[    0.651016] schedule_hrtimeout (kernel/time/hrtimer.c:1856)
[    0.652015] wait_task_inactive (kernel/sched/core.c:1260 (discriminator 1))
[    0.652656] ? migrate_swap (kernel/sched/core.c:1184)
[    0.653015] ? out_of_line_wait_on_atomic_t (kernel/sched/completion.c:121)
[    0.654016] ? wake_up_state (kernel/sched/core.c:2973)
[    0.654614] __kthread_bind (kernel/kthread.c:331)
[    0.655015] __kthread_unpark (kernel/kthread.c:394)
[    0.655622] kthread_unpark (kernel/kthread.c:413)
[    0.656014] ? ftrace_define_fields_softirq (kernel/softirq.c:755)
[    0.657014] smpboot_unpark_thread.isra.0 (kernel/smpboot.c:227)
[    0.657725] smpboot_register_percpu_thread (kernel/smpboot.c:286)
[    0.658013] ? ftrace_define_fields_softirq (kernel/softirq.c:755)
[    0.659015] ? ftrace_define_fields_softirq (kernel/softirq.c:755)
[    0.660013] spawn_ksoftirqd (kernel/softirq.c:758)
[    0.660609] do_one_initcall (init/main.c:787)
[    0.661014] ? try_to_run_init_process (init/main.c:776)
[    0.662017] ? print_cpu_info (arch/x86/kernel/cpu/common.c:1140 arch/x86/kernel/cpu/common.c:1135)
[    0.662638] ? native_smp_prepare_cpus (arch/x86/kernel/smpboot.c:1167)
[    0.663016] kernel_init_freeable (init/main.c:888 init/main.c:994)
[    0.664014] ? start_kernel (init/main.c:972)
[    0.664619] ? finish_task_switch (include/linux/tick.h:125 kernel/sched/core.c:2248)
[    0.665013] ? finish_task_switch (kernel/sched/sched.h:1077 kernel/sched/core.c:2230)
[    0.666019] ? rest_init (init/main.c:927)
[    0.666597] kernel_init (init/main.c:932)
[    0.667015] ? rest_init (init/main.c:927)
[    0.667593] ret_from_fork (arch/x86/kernel/entry_64.S:535)
[    0.668014] ? rest_init (init/main.c:927)


Thanks,
Sasha

Re: restart_syscall: use freezable blocking call

[Peter Zijlstra]

On Thu, Apr 02, 2015 at 12:03:08PM -0400, Sasha Levin wrote:
> It's not correct.

Yay for lockdep ;-)

> [    0.645644] debug_check_no_locks_held (kernel/locking/lockdep.c:4178)

Should we also have that call in freezer_do_not_count() ?

Re: restart_syscall: use freezable blocking call

[Tejun Heo]

On Thu, Apr 02, 2015 at 07:25:13PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 02, 2015 at 12:03:08PM -0400, Sasha Levin wrote:
> > It's not correct.
> 
> Yay for lockdep ;-)
> 
> > [    0.645644] debug_check_no_locks_held (kernel/locking/lockdep.c:4178)
> 
> Should we also have that call in freezer_do_not_count() ?

I think conerting a general scheduling function like
schedule_hrtimeout to do freezable sleeps blindly is a pretty bad
idea.  This must be determined per specific sleep site.

Thanks.

-- 
tejun