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

Groups > linux.kernel > #1670383 > unrolled thread

Re: [ANNOUNCE] v4.11.5-rt1

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

  Re: [ANNOUNCE] v4.11.5-rt1 Mike Galbraith <efault@gmx.de> - 2017-06-20 09:50 +0200
    Re: [ANNOUNCE] v4.11.5-rt1 Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-06-22 18:40 +0200
      Re: [ANNOUNCE] v4.11.5-rt1 Mike Galbraith <efault@gmx.de> - 2017-06-22 19:40 +0200
        Re: [ANNOUNCE] v4.11.5-rt1 Thomas Gleixner <tglx@linutronix.de> - 2017-06-22 23:40 +0200
        Re: [ANNOUNCE] v4.11.5-rt1 Mike Galbraith <efault@gmx.de> - 2017-06-23 04:10 +0200
        Re: [ANNOUNCE] v4.11.5-rt1 Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-06-23 14:50 +0200

#1670383 — Re: [ANNOUNCE] v4.11.5-rt1

On Mon, 2017-06-19 at 18:29 +0200, Mike Galbraith wrote:
> On Mon, 2017-06-19 at 10:41 -0400, Steven Rostedt wrote:
> > On Mon, 19 Jun 2017 16:13:41 +0200
> > Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> > 
> > 
> > > > Hmm, it shouldn't affect futexes, as it's only called by rtmutex when
> > > > waiter->savestate is true. And that should always be false for futex.  
> > > 
> > > you still have sleeping locks like the hb-lock (which might matter in
> > > case the task blocks on the lock and does not continue for some reason).
> > 
> > But then I'd expect this to be an issue with any spinlock in the kernel.
> 
> Thing to do is set a trap for the bugger.

See ! and ?

vogelweide:~/:[1]# grep MIKE trace 
MIKE 913.728104: start
MIKE  futex_wait-7496  [031] .......   913.729160: sched_process_fork: comm=futex_wait pid=7496 child_comm=futex_wait child_pid=7497
MIKE  futex_wait-7496  [031] .......   913.729253: sched_process_fork: comm=futex_wait pid=7496 child_comm=futex_wait child_pid=7499
MIKE  futex_wait-7496  [031] .......   913.729307: sched_process_fork: comm=futex_wait pid=7496 child_comm=futex_wait child_pid=7500
MIKE  futex_wait-7496  [031] .......   913.729348: sched_process_fork: comm=futex_wait pid=7496 child_comm=futex_wait child_pid=7501
MIKE  futex_wait-7496  [031] d...2..   913.729430: sched_switch: prev_comm=futex_wait prev_pid=7496 prev_prio=120 prev_state=S ==> next_comm=swapper/31 next_pid=0 next_prio=120
MIKE--futex_wait-7500  [005] d...2..   920.040320: sched_switch: prev_comm=futex_wait prev_pid=7500 prev_prio=120 prev_state=S ==> next_comm=swapper/5 next_pid=0 next_prio=120
MIKE  futex_wait-7497  [058] d...211   920.060009: sched_waking: comm=futex_wait pid=7501 prio=120 target_cpu=044
MIKE  futex_wait-7497  [058] d...311   920.060012: sched_wake_idle_without_ipi: cpu=44
MIKE  futex_wait-7497  [058] d...311   920.060012: sched_wakeup: comm=futex_wait pid=7501 prio=120 target_cpu=044
MIKE--futex_wait-7497  [058] d...2..   920.060035: sched_switch: prev_comm=futex_wait prev_pid=7497 prev_prio=120 prev_state=S ==> next_comm=swapper/58 next_pid=0 next_prio=120
MIKE  futex_wait-7499  [043] dN..411   920.060035: sched_wakeup: comm=ktimersoftd/43 pid=438 prio=0 target_cpu=043
MIKE! futex_wait-7499  [043] .N..111   920.060037: wake_up_lock_sleeper: futex_wait:7497 state:1 saved_state:0
MIKE! futex_wait-7499  [043] dN..211   920.060037: try_to_wake_up: futex_wait:7497 WF_LOCK_SLEEPER !(p->state:1 & state:2) bailing
MIKE 920.060037: futex_wait-7497 is never awakened again until ^C cleanup, 7499/7501 still standing
MIKE  futex_wait-7499  [043] ....111   920.060066: wake_up_lock_sleeper: futex_wait:7501 state:2 saved_state:0
MIKE  futex_wait-7499  [043] d...211   920.060066: try_to_wake_up: futex_wait:7501 WF_LOCK_SLEEPER !(p->state:0 & state:2) bailing
MIKE  futex_wait-7499  [043] ....111   920.060606: wake_up_lock_sleeper: futex_wait:7501 state:2 saved_state:0
MIKE 920.355048: huh? wake_up_lock_sleeper sees state=2 but try_to_wake_up then sees state=0 and bails ?!?
MIKE  futex_wait-7501  [044] ....111   920.355048: wake_up_lock_sleeper: futex_wait:7499 state:2 saved_state:0
MIKE  futex_wait-7501  [044] d...211   920.355049: try_to_wake_up: futex_wait:7499 WF_LOCK_SLEEPER !(p->state:0 & state:2) bailing
MIKE  futex_wait-7499  [048] d..h311   920.355060: sched_stat_runtime: comm=futex_wait pid=7499 runtime=168537 [ns] vruntime=4850005377 [ns]
MIKE  futex_wait-7499  [048] d...311   920.355061: sched_waking: comm=ksoftirqd/48 pid=489 prio=120 target_cpu=048
MIKE  futex_wait-7499  [048] d...411   920.355062: sched_stat_runtime: comm=futex_wait pid=7499 runtime=2972 [ns] vruntime=4850008349 [ns]
MIKE  futex_wait-7499  [048] d.L.411   920.355063: sched_wakeup: comm=ksoftirqd/48 pid=489 prio=120 target_cpu=048
MIKE  futex_wait-7499  [048] d.L.311   920.355064: sched_waking: comm=ktimersoftd/48 pid=488 prio=0 target_cpu=048
MIKE  futex_wait-7499  [048] dNL.411   920.355065: sched_wakeup: comm=ktimersoftd/48 pid=488 prio=0 target_cpu=048
MIKE  futex_wait-7499  [048] .NL.111   920.355066: wake_up_lock_sleeper: futex_wait:7501 state:0 saved_state:0
MIKE  futex_wait-7499  [048] dNL.211   920.355067: try_to_wake_up: futex_wait:7501 WF_LOCK_SLEEPER !(p->state:0 & state:2) bailing
MIKE  futex_wait-7499  [048] dNL.211   920.355067: sched_stat_runtime: comm=futex_wait pid=7499 runtime=2011 [ns] vruntime=4850010360 [ns]
MIKE  futex_wait-7499  [048] d...211   920.355068: sched_switch: prev_comm=futex_wait prev_pid=7499 prev_prio=120 prev_state=R+ ==> next_comm=ktimersoftd/48 next_pid=488 next_prio=0
MIKE       <...>-488   [048] d...2..   920.355070: sched_switch: prev_comm=ktimersoftd/48 prev_pid=488 prev_prio=0 prev_state=S ==> next_comm=ksoftirqd/48 next_pid=489 next_prio=120
MIKE       <...>-489   [048] d...2..   920.355071: sched_stat_runtime: comm=ksoftirqd/48 pid=489 runtime=1925 [ns] vruntime=4838010274 [ns]
MIKE       <...>-489   [048] d...2..   920.355072: sched_switch: prev_comm=ksoftirqd/48 prev_pid=489 prev_prio=120 prev_state=S ==> next_comm=futex_wait next_pid=7499 next_prio=120
MIKE  futex_wait-7501  [044] d...2..   920.355072: sched_stat_runtime: comm=futex_wait pid=7501 runtime=32398 [ns] vruntime=6632181410 [ns]
MIKE--futex_wait-7501  [044] d...2..   920.355074: sched_switch: prev_comm=futex_wait prev_pid=7501 prev_prio=120 prev_state=S ==> next_comm=swapper/44 next_pid=0 next_prio=120
MIKE 920.355074: 7499 is last man standing, but bored, just waking ktimersoftd
MIKE  futex_wait-7499  [048] d..h1..   920.356060: sched_stat_runtime: comm=futex_wait pid=7499 runtime=988793 [ns] vruntime=4850999153 [ns]
MIKE  futex_wait-7499  [048] d...1..   920.356061: sched_waking: comm=ktimersoftd/48 pid=488 prio=0 target_cpu=048
MIKE  futex_wait-7499  [048] dN..2..   920.356063: sched_wakeup: comm=ktimersoftd/48 pid=488 prio=0 target_cpu=048
MIKE  futex_wait-7499  [048] dN..2..   920.356063: sched_stat_runtime: comm=futex_wait pid=7499 runtime=1854 [ns] vruntime=4851001007 [ns]
MIKE  futex_wait-7499  [048] d...2..   920.356064: sched_switch: prev_comm=futex_wait prev_pid=7499 prev_prio=120 prev_state=R ==> next_comm=ktimersoftd/48 next_pid=488 next_prio=0
MIKE       <...>-488   [048] d...2..   920.356065: sched_switch: prev_comm=ktimersoftd/48 prev_pid=488 prev_prio=0 prev_state=S ==> next_comm=futex_wait next_pid=7499 next_prio=120
MIKE  futex_wait-7499  [048] d..h1..   920.384055: sched_stat_runtime: comm=futex_wait pid=7499 runtime=997267 [ns] vruntime=4878950744 [ns]
MIKE  futex_wait-7499  [048] d...1..   920.384056: sched_waking: comm=ktimersoftd/48 pid=488 prio=0 target_cpu=048
MIKE  futex_wait-7499  [048] dN..2..   920.384056: sched_wakeup: comm=ktimersoftd/48 pid=488 prio=0 target_cpu=048
MIKE  futex_wait-7499  [048] dN..2..   920.384057: sched_stat_runtime: comm=futex_wait pid=7499 runtime=1161 [ns] vruntime=4878951905 [ns]
MIKE  futex_wait-7499  [048] d...2..   920.384057: sched_switch: prev_comm=futex_wait prev_pid=7499 prev_prio=120 prev_state=R ==> next_comm=ktimersoftd/48 next_pid=488 next_prio=0
MIKE           <...>-488   [048] d...2..   920.384058: sched_switch: prev_comm=ktimersoftd/48 prev_pid=488 prev_prio=0 prev_state=S ==> next_comm=futex_wait next_pid=7499 next_prio=120
MIKE  futex_wait-7499  [048] d...2..   920.384271: sched_stat_runtime: comm=futex_wait pid=7499 runtime=213243 [ns] vruntime=4879165148 [ns]
MIKE  futex_wait-7499  [048] d...2..   920.384271: sched_switch: prev_comm=futex_wait prev_pid=7499 prev_prio=120 prev_state=S ==> next_comm=swapper/48 next_pid=0 next_prio=120
MIKE 920.384271: we're stalled, all futex_wait have scheduled off, elide gap
MIKE 981.447247: nothing to see above /me poking ^C, elide to EOF

[toc] | [next] | [standalone]

#1672807

On 2017-06-20 09:45:06 [+0200], Mike Galbraith wrote:
> See ! and ?

See see.
What about this:

diff --git a/include/linux/sched.h b/include/linux/sched.h
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1014,8 +1014,20 @@ struct wake_q_head {
 #define WAKE_Q(name)					\
 	struct wake_q_head name = { WAKE_Q_TAIL, &name.first }
 
-extern void wake_q_add(struct wake_q_head *head,
-			      struct task_struct *task);
+extern void __wake_q_add(struct wake_q_head *head,
+			 struct task_struct *task, bool sleeper);
+static inline void wake_q_add(struct wake_q_head *head,
+			      struct task_struct *task)
+{
+	__wake_q_add(head, task, false);
+}
+
+static inline void wake_q_add_sleeper(struct wake_q_head *head,
+				      struct task_struct *task)
+{
+	__wake_q_add(head, task, true);
+}
+
 extern void __wake_up_q(struct wake_q_head *head, bool sleeper);
 
 static inline void wake_up_q(struct wake_q_head *head)
@@ -1745,6 +1757,7 @@ struct task_struct {
 	raw_spinlock_t pi_lock;
 
 	struct wake_q_node wake_q;
+	struct wake_q_node wake_q_sleeper;
 
 #ifdef CONFIG_RT_MUTEXES
 	/* PI waiters blocked on a rt_mutex held by this task */
diff --git a/kernel/fork.c b/kernel/fork.c
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -558,6 +558,7 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
 	tsk->splice_pipe = NULL;
 	tsk->task_frag.page = NULL;
 	tsk->wake_q.next = NULL;
+	tsk->wake_q_sleeper.next = NULL;
 
 	account_kernel_stack(tsk, 1);
 
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1506,7 +1506,7 @@ static void mark_wakeup_next_waiter(struct wake_q_head *wake_q,
 	 */
 	preempt_disable();
 	if (waiter->savestate)
-		wake_q_add(wake_sleeper_q, waiter->task);
+		wake_q_add_sleeper(wake_sleeper_q, waiter->task);
 	else
 		wake_q_add(wake_q, waiter->task);
 	raw_spin_unlock(&current->pi_lock);
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -430,9 +430,15 @@ static bool set_nr_if_polling(struct task_struct *p)
 #endif
 #endif
 
-void wake_q_add(struct wake_q_head *head, struct task_struct *task)
+void __wake_q_add(struct wake_q_head *head, struct task_struct *task,
+		  bool sleeper)
 {
-	struct wake_q_node *node = &task->wake_q;
+	struct wake_q_node *node;
+
+	if (sleeper)
+		node = &task->wake_q_sleeper;
+	else
+		node = &task->wake_q;
 
 	/*
 	 * Atomically grab the task, if ->wake_q is !nil already it means
@@ -461,11 +467,17 @@ void __wake_up_q(struct wake_q_head *head, bool sleeper)
 	while (node != WAKE_Q_TAIL) {
 		struct task_struct *task;
 
-		task = container_of(node, struct task_struct, wake_q);
+		if (sleeper)
+			task = container_of(node, struct task_struct, wake_q_sleeper);
+		else
+			task = container_of(node, struct task_struct, wake_q);
 		BUG_ON(!task);
 		/* task can safely be re-inserted now */
 		node = node->next;
-		task->wake_q.next = NULL;
+		if (sleeper)
+			task->wake_q_sleeper.next = NULL;
+		else
+			task->wake_q.next = NULL;
 
 		/*
 		 * wake_up_process() implies a wmb() to pair with the queueing

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

#1672910

On Thu, 2017-06-22 at 18:34 +0200, Sebastian Andrzej Siewior wrote:
> On 2017-06-20 09:45:06 [+0200], Mike Galbraith wrote:
> > See ! and ?
> 
> See see.
> What about this:

I'll give it a go, likely during the weekend.

I moved 4.11-rt today (also repros nicely) due to ftrace annoying me.
 After yet more staring at ever more huge traces (opposite of goal;),
then taking a break to stare at source again, I decided that the dual
wake_q business should die.. and the stall died with it.

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1014,8 +1014,20 @@ struct wake_q_head {
>  #define WAKE_Q(name)					\
>  	struct wake_q_head name = { WAKE_Q_TAIL, &name.first }
>  
> -extern void wake_q_add(struct wake_q_head *head,
> -			      struct task_struct *task);
> +extern void __wake_q_add(struct wake_q_head *head,
> +			 struct task_struct *task, bool sleeper);
> +static inline void wake_q_add(struct wake_q_head *head,
> +			      struct task_struct *task)
> +{
> +	__wake_q_add(head, task, false);
> +}
> +
> +static inline void wake_q_add_sleeper(struct wake_q_head *head,
> +				      struct task_struct *task)
> +{
> +	__wake_q_add(head, task, true);
> +}
> +
>  extern void __wake_up_q(struct wake_q_head *head, bool sleeper);
>  
>  static inline void wake_up_q(struct wake_q_head *head)
> @@ -1745,6 +1757,7 @@ struct task_struct {
>  	raw_spinlock_t pi_lock;
>  
>  	struct wake_q_node wake_q;
> +	struct wake_q_node wake_q_sleeper;
>  
>  #ifdef CONFIG_RT_MUTEXES
>  	/* PI waiters blocked on a rt_mutex held by this task */
> diff --git a/kernel/fork.c b/kernel/fork.c
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -558,6 +558,7 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
>  	tsk->splice_pipe = NULL;
>  	tsk->task_frag.page = NULL;
>  	tsk->wake_q.next = NULL;
> +	tsk->wake_q_sleeper.next = NULL;
>  
>  	account_kernel_stack(tsk, 1);
>  
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -1506,7 +1506,7 @@ static void mark_wakeup_next_waiter(struct wake_q_head *wake_q,
>  	 */
>  	preempt_disable();
>  	if (waiter->savestate)
> -		wake_q_add(wake_sleeper_q, waiter->task);
> +		wake_q_add_sleeper(wake_sleeper_q, waiter->task);
>  	else
>  		wake_q_add(wake_q, waiter->task);
>  	raw_spin_unlock(&current->pi_lock);
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -430,9 +430,15 @@ static bool set_nr_if_polling(struct task_struct *p)
>  #endif
>  #endif
>  
> -void wake_q_add(struct wake_q_head *head, struct task_struct *task)
> +void __wake_q_add(struct wake_q_head *head, struct task_struct *task,
> +		  bool sleeper)
>  {
> -	struct wake_q_node *node = &task->wake_q;
> +	struct wake_q_node *node;
> +
> +	if (sleeper)
> +		node = &task->wake_q_sleeper;
> +	else
> +		node = &task->wake_q;
>  
>  	/*
>  	 * Atomically grab the task, if ->wake_q is !nil already it means
> @@ -461,11 +467,17 @@ void __wake_up_q(struct wake_q_head *head, bool sleeper)
>  	while (node != WAKE_Q_TAIL) {
>  		struct task_struct *task;
>  
> -		task = container_of(node, struct task_struct, wake_q);
> +		if (sleeper)
> +			task = container_of(node, struct task_struct, wake_q_sleeper);
> +		else
> +			task = container_of(node, struct task_struct, wake_q);
>  		BUG_ON(!task);
>  		/* task can safely be re-inserted now */
>  		node = node->next;
> -		task->wake_q.next = NULL;
> +		if (sleeper)
> +			task->wake_q_sleeper.next = NULL;
> +		else
> +			task->wake_q.next = NULL;
>  
>  		/*
>  		 * wake_up_process() implies a wmb() to pair with the queueing

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

#1673068

[Multipart message — attachments visible in raw view] — view raw

On Thu, 22 Jun 2017, Mike Galbraith wrote:
> On Thu, 2017-06-22 at 18:34 +0200, Sebastian Andrzej Siewior wrote:
> > On 2017-06-20 09:45:06 [+0200], Mike Galbraith wrote:
> > > See ! and ?
> > 
> > See see.
> > What about this:
> 
> I'll give it a go, likely during the weekend.
> 
> I moved 4.11-rt today (also repros nicely) due to ftrace annoying me.
>  After yet more staring at ever more huge traces (opposite of goal;),
> then taking a break to stare at source again, I decided that the dual
> wake_q business should die.. and the stall died with it.

The dual wake_q business is the semantically correct seperation. The evils
of 'sleeping spinlocks' require to preserve the task state accross the lock
sleep and act upon the real wakeup (not the lock wakeup) proper.

The TASK_ALL wakeup of lock sleepers was hiding that non seperation of
these two except for the resulting wreckage of ptrace, which we simply
ignored for a long time because ....

Now that we figure out that TASK_ALL is wrong we exposed the convolution of
competing lock wakeups and regular wakeups. Seperating wake_q for those is
the only sane solution.

Thanks,

	tglx

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

#1673175

On Thu, 2017-06-22 at 19:30 +0200, Mike Galbraith wrote:
> On Thu, 2017-06-22 at 18:34 +0200, Sebastian Andrzej Siewior wrote:
> > On 2017-06-20 09:45:06 [+0200], Mike Galbraith wrote:
> > > See ! and ?
> > 
> > See see.
> > What about this:
> 
> I'll give it a go, likely during the weekend.

What the heck, after so much time expended squabbling with this bugger,
beans/biscuits work will hardly notice a mere hour or so...

Yup, it's confirmed dead, and no doubt my GUI goblins will be too.

	-Mike

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

#1673518

On 2017-06-22 19:30:07 [+0200], Mike Galbraith wrote:
> On Thu, 2017-06-22 at 18:34 +0200, Sebastian Andrzej Siewior wrote:
> > On 2017-06-20 09:45:06 [+0200], Mike Galbraith wrote:
> > > See ! and ?
> > 
> > See see.
> > What about this:
> 
> I'll give it a go, likely during the weekend.

It survived >1d on my AMD-A10 box. I am adding this description:

|If a task is queued as a sleeper for a wakeup and never goes to
|schedule() (because it just obtained the lock) then it will receive a
|spurious wake up which is not "bad", it is considered. Until that wake
|up happens this task can no be enqueued for any wake ups handled by the
|WAKE_Q infrastructure (because a task can only be enqueued once). This
|wouldn't be bad if we would use the same wakeup mechanism for the wake
|up of sleepers as we do for "normal" wake ups. But we don't…
|
|So.
|   T1			T2		T3
|   spin_lock(x)				spin_unlock(x);
|   					wake_q_add(q1, T1)
|   spin_unlock(x)
|   set_state(TASK_INTERRUPTIBLE)
|   if (!condition)
|	schedule()
|			condition = true
|			wake_q_add(q2, T1)
|			// T1 not added, still enqueued
|			wake_up_q(q2)
|					wake_up_q_sleeper(q1)
|					// T1 not woken up, wrong task state
|
|In order to solve this race this patch adds a wake_q_node for the
|sleeper case.

and consider this closed unless I hear from you different things :)

Sebastian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web