On Mon, Oct 17, 2016 at 07:16:50PM -0400, Waiman Long wrote:
> >+++ b/kernel/locking/mutex.c
> >@@ -631,13 +631,21 @@ __mutex_lock_common(struct mutex *lock,
> >
> > lock_contended(&lock->dep_map, ip);
> >
> >+ set_task_state(task, state);
>
> Do we want to set the state here? I am not sure if it is OK to set the task
> state without ever calling schedule().
That's entirely fine, note how we'll set it back to RUNNING at the end.
> > for (;;) {
> >+ /*
> >+ * Once we hold wait_lock, we're serialized against
> >+ * mutex_unlock() handing the lock off to us, do a trylock
> >+ * before testing the error conditions to make sure we pick up
> >+ * the handoff.
> >+ */
> > if (__mutex_trylock(lock, first))
> >- break;
> >+ goto acquired;
> >
> > /*
> >- * got a signal? (This code gets eliminated in the
> >- * TASK_UNINTERRUPTIBLE case.)
> >+ * Check for signals and wound conditions while holding
> >+ * wait_lock. This ensures the lock cancellation is ordered
> >+ * against mutex_unlock() and wake-ups do not go missing.
> > */
> > if (unlikely(signal_pending_state(state, task))) {
> > ret = -EINTR;
> >@@ -650,16 +658,27 @@ __mutex_lock_common(struct mutex *lock,
> > goto err;
> > }
> >
> >- __set_task_state(task, state);
> > spin_unlock_mutex(&lock->wait_lock, flags);
> > schedule_preempt_disabled();
> >- spin_lock_mutex(&lock->wait_lock, flags);
> >
> > if (!first&& __mutex_waiter_is_first(lock,&waiter)) {
> > first = true;
> > __mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
> > }
> >+
> >+ set_task_state(task, state);
>
> I would suggest keep the __set_task_state() above and change
> set_task_state(task, state) to set_task_state(task, TASK_RUNNING) to provide
> the memory barrier. Then we don't need adding __set_task_state() calls
> below.
set_task_state(RUNNING) doesn't make sense, ever.
See the comment near set_task_state() for the reason it has a barrier.
We need it here because when we do that trylock (or optimistic spin) we
need to have set the state and done a barrier, otherwise we can miss a
wakeup and get stuck.
> >+ /*
> >+ * Here we order against unlock; we must either see it change
> >+ * state back to RUNNING and fall through the next schedule(),
> >+ * or we must see its unlock and acquire.
> >+ */
> >+ if (__mutex_trylock(lock, first))
> >+ break;
> >+
>
> I don't think we need a trylock here since we are going to do it at the top
> of the loop within wait_lock anyway.
The idea was to avoid the wait-time of that lock acquire, also, this is
a place-holder for the optimistic spin site for the next patch.
