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

Groups > linux.kernel > #1455994 > unrolled thread

Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs

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: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Peter Zijlstra <peterz@infradead.org> - 2016-08-03 21:50 +0200
    Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-03 23:40 +0200
      Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Peter Zijlstra <peterz@infradead.org> - 2016-08-04 16:20 +0200
        Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Peter Zijlstra <peterz@infradead.org> - 2016-08-08 12:30 +0200
          Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Bart Van Assche <bvanassche@acm.org> - 2016-08-08 16:40 +0200
            Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-08 18:30 +0200
              Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-09 19:20 +0200
                Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-10 21:00 +0200
                Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-10 21:10 +0200
              Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Peter Zijlstra <peterz@infradead.org> - 2016-08-10 21:20 +0200
              Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-10 23:30 +0200

#1455994 — Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs

On Wed, Aug 03, 2016 at 09:35:03AM -0700, Bart Van Assche wrote:
> If try_to_wakeup() reads the task state before abort_exclusive_wait()
> sets the task state and if autoremove_wake_function() is called after
> abort_exclusive_wait() has removed a task from a wait list then the
> cascading mechanism for exclusive wakeups in abort_exclusive_wait()
> won't be triggered. Avoid this by serializing the task state change
> in abort_exclusive_wait() and try_to_wakeup().

I'm dense.. what!?

	CPU0			CPU1			CPU2

	

				__lock_page_killable()
				  __wait_on_bit_lock()
				    bit_wait_io()
				      schedule()
	__wake_up_bit()
	  __wake_up(.nr_exclusive=1)
	    spin_lock(&q->lock)
	    __wake_up_common()
	      autoremove_wake_func()
	        try_to_wake_up(p, TASK_NORMAL)
		list_del_init(&wait->task_list)
	    spin_unlock(&q->lock)

							complete_signal(p)
							  signal_wake_up(p, 1)
							    sigaddset(&p->pending.signal, SIGKILL)
							    try_to_wake_up(p, TASK_WAKEKILL)

				      if (signal_pending_state(TASK_KILLABLE))
				        return -EINTR;
				    abort_exclusive_wait()
				      __set_current_state(RUNNING)
				      spin_lock(q->lock)
				      if (!list_empty()) /* empty */
				      else if (waitqueue_active()) /* pending ? */
				        __wake_up_locked_key(q, mode, key)
				      spin_unlock(q->lock)


That seems to do the right thing, so clearly I misunderstand. Please
clarify.


> +++ b/kernel/sched/wait.c
> @@ -277,10 +277,17 @@ void abort_exclusive_wait(wait_queue_head_t *q, wait_queue_t *wait,
>  			unsigned int mode, void *key)
>  {
>  	unsigned long flags;
> +	long wake_up;
> +
> +	/* Serialize against try_to_wake_up() */
> +	raw_spin_lock_irqsave(&current->pi_lock, flags);
> +	wake_up = current->state & (TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE);
> +	if (wake_up)
> +		__set_current_state(TASK_RUNNING);
> +	raw_spin_unlock_irqrestore(&current->pi_lock, flags);
>  
> -	__set_current_state(TASK_RUNNING);
>  	spin_lock_irqsave(&q->lock, flags);
> -	if (!list_empty(&wait->task_list))
> +	if (wake_up)
>  		list_del_init(&wait->task_list);
>  	else if (waitqueue_active(q))
>  		__wake_up_locked_key(q, mode, key);

That just feels wrong,.. very wrong.

[toc] | [next] | [standalone]

#1456036

Hi Bart,

I too can't understand the problem. Perhaps you missed the fact that
abort_exclusive_wait() does everything under wait_queue_head_t->lock ?

On 08/03, Bart Van Assche wrote:
>
> try_to_wake_up() locks task_struct.pi_lock but abort_exclusive_wait() not.
> My assumption is that the following sequence of events leads to the lockup
> that I had mentioned in the description of my patch:
> * try_to_wake_up() is called for the task that will execute
>   abort_exclusive_wait().
> * After try_to_wake_up() has checked task_struct.state and before
>   autoremove_wake_function() has tried to remove the task from the wait
>   queue, abort_exclusive_wait() is executed for the same task.

But we do not care if we race with another try_to_wake_up(), or even with
another exclusive wake_up_nr(wq)/whatever unless wq is the same.

And if this wq is the same, then wake_up_nr() will do try_to_wake_up/autoremove
either before or after abort_exclusive_wait(), wake_up_nr() takes the same
wq->lock.

And this means that abort_exclusive_wait() can't be called "After try_to_wake_up()"
and "before autoremove_wake_function()".

Oleg.

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

#1456431

On Wed, Aug 03, 2016 at 02:51:23PM -0700, Bart Van Assche wrote:
> So I started testing the patch below that should fix the same hang but
> without triggering any wait list corruption.
> 
> diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
> index f15d6b6..4e3f651 100644
> --- a/kernel/sched/wait.c
> +++ b/kernel/sched/wait.c
> @@ -282,7 +282,7 @@ void abort_exclusive_wait(wait_queue_head_t *q,
> wait_queue_t *wait,
>  	spin_lock_irqsave(&q->lock, flags);
>  	if (!list_empty(&wait->task_list))
>  		list_del_init(&wait->task_list);
> -	else if (waitqueue_active(q))
> +	if (waitqueue_active(q))
>  		__wake_up_locked_key(q, mode, key);
>  	spin_unlock_irqrestore(&q->lock, flags);
>  }

So the problem with this patch is that it will violate the nr_exclusive
semantics in that it can result in too many wakeups -- which is a much
less severe (typically harmless) issue.

We now always wake up the next waiter, even if there wasn't an actual
wakeup we raced against. And if we then also get a wakeup, we can end up
with 2 woken tasks (instead of the nr_exclusive=1).

Now, since wait loops must all deal with spurious wakeups, this ends up
as harmless overhead.

But I'd still like to understand where we loose the wakeup. What are you
doing to reproduce this issue?

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

#1457689

On Fri, Aug 05, 2016 at 10:41:33AM -0700, Bart Van Assche wrote:
> On 08/04/2016 07:09 AM, Peter Zijlstra wrote:

> >But I'd still like to understand where we loose the wakeup.
> 
> My assumption is that __wake_up_common() and signal delivery happen
> concurrently, that __wake_up_common() wakes up bit_wait_io() and that signal
> delivery happens after bit_wait_io() has been woken up but before it tests
> the signal pending state.

That would be the exact scenario I drew a picture of, no? I'm still
failing to see the hole there.

Please draw a picture like that and illustrate the hole.

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

#1457839

On 08/08/16 03:22, Peter Zijlstra wrote:
> That would be the exact scenario I drew a picture of, no? I'm still
> failing to see the hole there.
> 
> Please draw a picture like that and illustrate the hole.
 
Hi Peter,

This is the sequence of which I think that it leads to the missed wakeup:

Task 1                    Task 2                    Task 3                    Task 4

lock_page()
 ...
                          lock_page_killable()
                           __lock_page_killable()
                            __wait_on_bit_lock()
                             bit_wait_io()
                              io_schedule()
                               ...
                                                                              lock_page()
                                                                               __lock_page()
                                                                                __wait_on_bit_lock()
                                                                                 bit_wait_io()
                                                                                  io_schedule()
                                                                                   ...


                                                    (signal delivery to task 2)
                                                    try_to_wake_up(task2, ..., ...)
                                                    (try_to_wake_up() returns 1)

unlock_page()
 wake_up_page()
  __wake_up_bit()
   __wake_up(wq, TASK_NORMAL, 1, &key)
    __wake_up_common(wq, mode=TASK_NORMAL, nr_exclusive=1, 0, key)
     wake_bit_function()
      autoremove_wake_function()
       default_wake_function()
        try_to_wake_up() <- skips task 2 because task 3 already changed
                            the task state of task 2
       (autoremove_wake_function() does not do
        list_del_init(&wait->task_list))


                              bit_wait_io() returns -EINTR
                             abort_exclusive_wait() is called by __wait_on_bit_lock()


In the above sequence task 1 does not remove task 2 from the waitqueue
because task 3 had already woken up task 2. The result is that when task 2
calls abort_exclusive_wait() that task 2 is still on the waitqueue. With the
current implementation of abort_exclusive_wait() in the above scenario task
4 is not woken up although it should be woken up. Hence the patch that removes
the "else" keyword from abort_exclusive_wait().

Bart.

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

#1457896

On 08/08, Bart Van Assche wrote:
>
> This is the sequence of which I think that it leads to the missed wakeup:
>
> Task 1                    Task 2                    Task 3                    Task 4
>
> lock_page()
>  ...
>                           lock_page_killable()
>                            __lock_page_killable()
>                             __wait_on_bit_lock()
>                              bit_wait_io()
>                               io_schedule()
>                                ...
>                                                                               lock_page()
>                                                                                __lock_page()
>                                                                                 __wait_on_bit_lock()
>                                                                                  bit_wait_io()
>                                                                                   io_schedule()
>                                                                                    ...
>
>
>                                                     (signal delivery to task 2)
>                                                     try_to_wake_up(task2, ..., ...)
>                                                     (try_to_wake_up() returns 1)
>
> unlock_page()
>  wake_up_page()
>   __wake_up_bit()
>    __wake_up(wq, TASK_NORMAL, 1, &key)
>     __wake_up_common(wq, mode=TASK_NORMAL, nr_exclusive=1, 0, key)
>      wake_bit_function()
>       autoremove_wake_function()
>        default_wake_function()
>         try_to_wake_up() <- skips task 2 because task 3 already changed
>                             the task state of task 2
>        (autoremove_wake_function() does not do
>         list_del_init(&wait->task_list))

Yes.

But since it skips task2, __wake_up_common() doesn't decrement nr_exclusive,
doesn't stop. It continues the list_for_each_entry_safe() loop, and finds the
sleeping task4, and wakes it up,

>                               bit_wait_io() returns -EINTR
>                              abort_exclusive_wait() is called by __wait_on_bit_lock()
>
>
> In the above sequence task 1 does not remove task 2 from the waitqueue
> because task 3 had already woken up task 2. The result is that when task 2
> calls abort_exclusive_wait() that task 2 is still on the waitqueue.

Yes, but this is fine,

> With the
> current implementation of abort_exclusive_wait() in the above scenario task
> 4 is not woken up although it should be woken up.

See above, it must be already woken by __wake_up_common().



So far _I think_ that the bug is somewhere else... Say, someone clears
PG_locked without wake_up(). Then SIGKILL sent to the task sleeping in
sys_read() "adds" the necessary wakeup...

Do you use external modules during the testing?

Oleg.

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

#1459008

On 08/08, Bart Van Assche wrote:
>
> No external modules were loaded when I triggered the lockup

Heh. Could you test the patch below?

Oleg.

--- x/kernel/sched/wait.c
+++ x/kernel/sched/wait.c
@@ -283,7 +283,7 @@ void abort_exclusive_wait(wait_queue_hea
 	if (!list_empty(&wait->task_list))
 		list_del_init(&wait->task_list);
 	else if (waitqueue_active(q))
-		__wake_up_locked_key(q, mode, key);
+		__wake_up_locked_key(q, TASK_NORMAL, key);
 	spin_unlock_irqrestore(&q->lock, flags);
 }
 EXPORT_SYMBOL(abort_exclusive_wait);

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

#1459536

On 08/10, Bart Van Assche wrote:
>
> On 08/10/2016 03:46 AM, Oleg Nesterov wrote:
> > OK. Could you  try another debugging patch below?
> >
> > Oleg.
> > ---
> >
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index e5a3244..9d5f892 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> > @@ -711,6 +711,15 @@ static inline int page_has_private(struct page *page)
> >  	return !!(page->flags & PAGE_FLAGS_PRIVATE);
> >  }
> >
> > +void unlock_page(struct page *page);
> > +static inline void __ClearPageLocked_x(struct page *page)
> > +{
> > +	if (PageLocked(compound_head(page)))
> > +		unlock_page(page);
> > +}
> > +
> > +#define __ClearPageLocked(page)	__ClearPageLocked_x(page)
> > +
> >  #undef PF_ANY
> >  #undef PF_HEAD
> >  #undef PF_NO_TAIL
>
> Hi Oleg,
>
> Are you sure that all __ClearPageLocked() users pass the compound head
> to that macro?

Hmm. it obviously should... which kernel version do you use for testing?

From include/linux/page-flags.h

	__PAGEFLAG(Locked, locked, PF_NO_TAIL)

and

	#define PF_NO_TAIL(page, enforce) ({                                    \
		VM_BUG_ON_PGFLAGS(enforce && PageTail(page), page);     \
	compound_head(page);})
	
and this matches compound_head() in lock/unlock_page().

> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -711,6 +711,17 @@ static inline int page_has_private(struct page *page)
>  	return !!(page->flags & PAGE_FLAGS_PRIVATE);
>  }
>
> +void unlock_page(struct page *page);
> +static inline void __ClearPageLocked_x(struct page *page)
> +{
> +	if (PageLocked(compound_head(page)))
> +		unlock_page(page);
> +	else
> +		__ClearPageLocked(page);
> +}

No, no. If you use an old kernel (which doesn't call compound_head() in
lock_page()), then just remove compound_head() from __ClearPageLocked_x()
above:

	static inline void __ClearPageLocked_x(struct page *page)
	{
		if (PageLocked(page))
			unlock_page(page);
	}

even if this shouldn't make any difference afaics, note the
VM_BUG_ON_PGFLAGS() above.

Oleg.

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

#1459606

On 08/09, Bart Van Assche wrote:
>
> On 08/09/2016 10:15 AM, Oleg Nesterov wrote:
> >
> > --- x/kernel/sched/wait.c
> > +++ x/kernel/sched/wait.c
> > @@ -283,7 +283,7 @@ void abort_exclusive_wait(wait_queue_hea
> >  	if (!list_empty(&wait->task_list))
> >  		list_del_init(&wait->task_list);
> >  	else if (waitqueue_active(q))
> > -		__wake_up_locked_key(q, mode, key);
> > +		__wake_up_locked_key(q, TASK_NORMAL, key);
> >  	spin_unlock_irqrestore(&q->lock, flags);
> >  }
> >  EXPORT_SYMBOL(abort_exclusive_wait);
>
> Hello Oleg,
>
> That patch looks interesting to me.

And I'll redo/resend it, __wake_up_locked_key(mode) is simply wrong I think.

But it can't affect lock_page() because TASK_KILLABLE includes TASK_UNINTERRUPTIBLE
and we do not have lock_page_interruptible().


> Unfortunately even with that patch
> applied I still see lockups.

Thanks. I hoped this change can fix some another exclusive wait...

OK. Could you  try another debugging patch below?

Oleg.
---

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index e5a3244..9d5f892 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -711,6 +711,15 @@ static inline int page_has_private(struct page *page)
 	return !!(page->flags & PAGE_FLAGS_PRIVATE);
 }
 
+void unlock_page(struct page *page);
+static inline void __ClearPageLocked_x(struct page *page)
+{
+	if (PageLocked(compound_head(page)))
+		unlock_page(page);
+}
+
+#define __ClearPageLocked(page)	__ClearPageLocked_x(page)
+
 #undef PF_ANY
 #undef PF_HEAD
 #undef PF_NO_TAIL

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

#1459624

On Wed, Aug 10, 2016 at 12:57:25PM +0200, Oleg Nesterov wrote:
> This condition is fine, and the trace is clear. This means that lock_page_killable()
> was interrupted and wake_bit_function() was not called. We do not need another wakeup
> in this case but somehow it helps. Again, I think because the necessary wakeup was
> already lost/missed.

I suspect the same. Removing that else generates 'spurious' wakeups,
which can unstick the situation, hiding the real source of the problem.

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

#1459997

On 08/09, Bart Van Assche wrote:
>
> Hello Oleg,
>
> Something that puzzles me is that removing the "else" keyword from
> abort_exclusive_wait() is sufficient to avoid the hang.

Yes, we need to understand this.

> If there would
> be code that clears PG_locked without calling wake_up() this hang
> probably would also be triggered by workloads that do not wake up
> lock_page_killable() with a signal.

Yes, and I already have another debugging patch to test this... it simply turns
lock_page_killable() into lock_page(). But lets check __ClearPageLocked() first
(the patch I sent a minute ago).

> BTW, the
> WARN_ONCE(!list_empty(&wait->task_list) && waitqueue_active(q), "mode =
> %#x\n", mode) statement that I added in abort_exclusive_wait() just
> produced the following call stack:

This condition is fine, and the trace is clear. This means that lock_page_killable()
was interrupted and wake_bit_function() was not called. We do not need another wakeup
in this case but somehow it helps. Again, I think because the necessary wakeup was
already lost/missed.

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web