[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e914d4fd-5afc-35f9-c068-7044ceda53dd@redhat.com>
Date: Mon, 28 Jun 2021 13:57:40 -0400
From: Waiman Long <llong@...hat.com>
To: Yanfei Xu <yanfei.xu@...driver.com>, peterz@...radead.org,
mingo@...hat.com, boqun.feng@...il.com
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH] locking/mutex: fix the MUTEX_FLAG_HANDOFF bit is cleared
unexpected
On 6/28/21 11:51 AM, Yanfei Xu wrote:
> When the mutex unlock path is excuted with WAITERS bit and without
> HANDOFF bit set, it will wake up the first task in wait_list. If
> there are some tasks not in wait_list are stealing lock, it is very
> likely successfully due to the task field of lock is NULL and flags
> field is non-NULL. Then the HANDOFF bit will be cleared. But if the
> HANDOFF bit was just set by the waked task in wait_list, this clearing
> is unexcepted.
I think you mean "unexpected". Right? Anyway, all the setting and
clearing of the HANDOFF bit are atomic. There shouldn't be any
unexpected clearing.
> __mutex_lock_common __mutex_lock_common
> __mutex_trylock schedule_preempt_disabled
> /*steal lock successfully*/ __mutex_set_flag(lock,MUTEX_FLAG_HANDOFF)
> __mutex_trylock_or_owner
> if (task==NULL)
> flags &= ~MUTEX_FLAG_HANDOFF
> atomic_long_cmpxchg_acquire
> __mutex_trylock //failed
> mutex_optimistic_spin //failed
> schedule_preempt_disabled //sleep without HANDOFF bit
>
> So the HANDOFF bit should be set as late as possible, here we defer
> it util the task is going to be scheduled.
> Signed-off-by: Yanfei Xu <yanfei.xu@...driver.com>
> ---
>
> Hi maintainers,
>
> I am not very sure if I missed or misunderstanded something, please help
> to review. Many thanks!
>
> kernel/locking/mutex.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index 013e1b08a1bf..e57d920e96bf 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -1033,17 +1033,17 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
> }
>
> spin_unlock(&lock->wait_lock);
> +
> + if (first)
> + __mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
> schedule_preempt_disabled();
>
> /*
> * ww_mutex needs to always recheck its position since its waiter
> * list is not FIFO ordered.
> */
> - if (ww_ctx || !first) {
> + if (ww_ctx || !first)
> first = __mutex_waiter_is_first(lock, &waiter);
> - if (first)
> - __mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
> - }
>
> set_current_state(state);
> /*
In general, I don't mind setting the HANDOFF bit later, but
mutex_optimistic_spin() at the end of the loop should only be called
after the HANDOFF bit is set. So the logic isn't quite right yet.
Cheers,
Longman
Powered by blists - more mailing lists