From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E8F6EC433EF for ; Mon, 13 Sep 2021 08:42:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D0C1560F5B for ; Mon, 13 Sep 2021 08:42:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238067AbhIMIn7 (ORCPT ); Mon, 13 Sep 2021 04:43:59 -0400 Received: from mga12.intel.com ([192.55.52.136]:26953 "EHLO mga12.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237831AbhIMIn4 (ORCPT ); Mon, 13 Sep 2021 04:43:56 -0400 X-IronPort-AV: E=McAfee;i="6200,9189,10105"; a="201118785" X-IronPort-AV: E=Sophos;i="5.85,288,1624345200"; d="scan'208";a="201118785" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Sep 2021 01:42:40 -0700 X-IronPort-AV: E=Sophos;i="5.85,288,1624345200"; d="scan'208";a="551510733" Received: from einfeld-mobl.ger.corp.intel.com (HELO [10.252.46.222]) ([10.252.46.222]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Sep 2021 01:42:38 -0700 Subject: Re: [PATCH v2] kernel/locking: Add context to ww_mutex_trylock. To: Peter Zijlstra Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Ingo Molnar , Will Deacon , Waiman Long , Boqun Feng , Liam Girdwood , Mark Brown , linux-kernel@vger.kernel.org, Daniel Vetter References: <20210907132044.157225-1-maarten.lankhorst@linux.intel.com> <96ab9cf1-250a-8f34-51ec-4a7f66a87b39@linux.intel.com> From: Maarten Lankhorst Message-ID: Date: Mon, 13 Sep 2021 10:42:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Op 10-09-2021 om 17:02 schreef Peter Zijlstra: > On Thu, Sep 09, 2021 at 11:32:18AM +0200, Maarten Lankhorst wrote: >> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c >> index d456579d0952..791c28005eef 100644 >> --- a/kernel/locking/mutex.c >> +++ b/kernel/locking/mutex.c >> @@ -736,6 +736,44 @@ __ww_mutex_lock(struct mutex *lock, unsigned int state, unsigned int subclass, >> return __mutex_lock_common(lock, state, subclass, NULL, ip, ww_ctx, true); >> } >> >> +/** >> + * ww_mutex_trylock - tries to acquire the w/w mutex with optional acquire context >> + * @lock: mutex to lock >> + * @ctx: optional w/w acquire context >> + * >> + * Trylocks a mutex with the optional acquire context; no deadlock detection is >> + * possible. Returns 1 if the mutex has been acquired successfully, 0 otherwise. >> + * >> + * Unlike ww_mutex_lock, no deadlock handling is performed. However, if a @ctx is >> + * specified, -EALREADY and -EDEADLK handling may happen in calls to ww_mutex_lock. >> + * >> + * A mutex acquired with this function must be released with ww_mutex_unlock. >> + */ >> +int __sched >> +ww_mutex_trylock(struct ww_mutex *ww, struct ww_acquire_ctx *ctx) >> +{ >> + bool locked; >> + >> + if (!ctx) >> + return mutex_trylock(&ww->base); >> + >> +#ifdef CONFIG_DEBUG_MUTEXES >> + DEBUG_LOCKS_WARN_ON(ww->base.magic != &ww->base); >> +#endif >> + >> + preempt_disable(); >> + locked = __mutex_trylock(&ww->base); >> + >> + if (locked) { >> + ww_mutex_set_context_fastpath(ww, ctx); >> + mutex_acquire_nest(&ww->base.dep_map, 0, 1, &ctx->dep_map, _RET_IP_); >> + } >> + preempt_enable(); >> + >> + return locked; >> +} >> +EXPORT_SYMBOL(ww_mutex_trylock); >> + >> #ifdef CONFIG_DEBUG_LOCK_ALLOC >> void __sched >> mutex_lock_nested(struct mutex *lock, unsigned int subclass) >> diff --git a/kernel/locking/ww_rt_mutex.c b/kernel/locking/ww_rt_mutex.c >> index 3f1fff7d2780..c4cb863edb4c 100644 >> --- a/kernel/locking/ww_rt_mutex.c >> +++ b/kernel/locking/ww_rt_mutex.c >> @@ -50,6 +50,18 @@ __ww_rt_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ww_ctx, >> return ret; >> } >> >> +int __sched >> +ww_mutex_trylock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx) >> +{ >> + int locked = rt_mutex_trylock(&lock->base); >> + >> + if (locked && ctx) >> + ww_mutex_set_context_fastpath(lock, ctx); >> + >> + return locked; >> +} >> +EXPORT_SYMBOL(ww_mutex_trylock); >> + >> int __sched >> ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx) >> { > That doesn't look right, how's this for you? > > --- > --- a/kernel/locking/mutex.c > +++ b/kernel/locking/mutex.c > @@ -94,6 +94,9 @@ static inline unsigned long __owner_flag > return owner & MUTEX_FLAGS; > } > > +/* > + * Returns: __mutex_owner(lock) on failure or NULL on success. > + */ > static inline struct task_struct *__mutex_trylock_common(struct mutex *lock, bool handoff) > { > unsigned long owner, curr = (unsigned long)current; > @@ -736,6 +739,47 @@ __ww_mutex_lock(struct mutex *lock, unsi > return __mutex_lock_common(lock, state, subclass, NULL, ip, ww_ctx, true); > } > > +/** > + * ww_mutex_trylock - tries to acquire the w/w mutex with optional acquire context > + * @ww: mutex to lock > + * @ww_ctx: optional w/w acquire context > + * > + * Trylocks a mutex with the optional acquire context; no deadlock detection is > + * possible. Returns 1 if the mutex has been acquired successfully, 0 otherwise. > + * > + * Unlike ww_mutex_lock, no deadlock handling is performed. However, if a @ctx is > + * specified, -EALREADY handling may happen in calls to ww_mutex_trylock. > + * > + * A mutex acquired with this function must be released with ww_mutex_unlock. > + */ > +int ww_mutex_trylock(struct ww_mutex *ww, struct ww_acquire_ctx *ww_ctx) > +{ > + if (!ww_ctx) > + return mutex_trylock(&ww->base); > + > + MUTEX_WARN_ON(ww->base.magic != &ww->base); > + > + if (unlikely(ww_ctx == READ_ONCE(ww->ctx))) > + return -EALREADY; I'm not 100% sure this is a good idea, because it would make the trylock weird. For i915 I checked manually, because I didn't want to change the function signature. This is probably the other extreme. "if (ww_mutex_trylock())" would look correct, but actually be wrong and lead to double unlock without adjustments. Maybe we could make a ww_mutex_trylock_ctx_err, which would return -EALREADY or -EBUSY on failure, and 0 on success? We could keep ww_mutex_trylock without ctx, probably just #define as (!ww_mutex_trylock_ctx_err(lock, NULL)) > + /* > + * Reset the wounded flag after a kill. No other process can > + * race and wound us here, since they can't have a valid owner > + * pointer if we don't have any locks held. > + */ > + if (ww_ctx->acquired == 0) > + ww_ctx->wounded = 0; Yeah I guess this needs fixing too. Not completely sure since trylock wouldn't do the whole ww dance, but since it's our first lock, probably best to do so regardless so other users don't trip over it. > + > + if (__mutex_trylock(&ww->base)) { > + ww_mutex_set_context_fastpath(ww, ww_ctx); > + mutex_acquire_nest(&ww->base.dep_map, 0, 1, &ww_ctx->dep_map, _RET_IP_); > + return 1; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(ww_mutex_trylock); > + > #ifdef CONFIG_DEBUG_LOCK_ALLOC > void __sched > mutex_lock_nested(struct mutex *lock, unsigned int subclass) > --- a/kernel/locking/ww_rt_mutex.c > +++ b/kernel/locking/ww_rt_mutex.c > @@ -9,6 +9,34 @@ > #define WW_RT > #include "rtmutex.c" > > +int ww_mutex_trylock(struct ww_mutex *lock, struct ww_acquire_ctx *ww_ctx) > +{ > + struct rt_mutex *rtm = &lock->base; > + > + if (!ww_ctx) > + return rt_mutex_trylock(rtm); > + > + if (unlikely(ww_ctx == READ_ONCE(lock->ctx))) > + return -EALREADY; > + > + /* > + * Reset the wounded flag after a kill. No other process can > + * race and wound us here, since they can't have a valid owner > + * pointer if we don't have any locks held. > + */ > + if (ww_ctx->acquired == 0) > + ww_ctx->wounded = 0; > + > + if (__rt_mutex_trylock(&rtm->rtmutex)) { > + ww_mutex_set_context_fastpath(lock, ww_ctx); > + mutex_acquire_nest(&rtm->dep_map, 0, 1, ww_ctx->dep_map, _RET_IP_); > + return 1; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(ww_mutex_trylock); > + > static int __sched > __ww_rt_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ww_ctx, > unsigned int state, unsigned long ip) From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 63953C433F5 for ; Mon, 13 Sep 2021 08:42:43 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1C35560F5B for ; Mon, 13 Sep 2021 08:42:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 1C35560F5B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 909DA6E11A; Mon, 13 Sep 2021 08:42:42 +0000 (UTC) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2DFB26E11A; Mon, 13 Sep 2021 08:42:41 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10105"; a="201118786" X-IronPort-AV: E=Sophos;i="5.85,288,1624345200"; d="scan'208";a="201118786" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Sep 2021 01:42:40 -0700 X-IronPort-AV: E=Sophos;i="5.85,288,1624345200"; d="scan'208";a="551510733" Received: from einfeld-mobl.ger.corp.intel.com (HELO [10.252.46.222]) ([10.252.46.222]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Sep 2021 01:42:38 -0700 To: Peter Zijlstra Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Ingo Molnar , Will Deacon , Waiman Long , Boqun Feng , Liam Girdwood , Mark Brown , linux-kernel@vger.kernel.org, Daniel Vetter References: <20210907132044.157225-1-maarten.lankhorst@linux.intel.com> <96ab9cf1-250a-8f34-51ec-4a7f66a87b39@linux.intel.com> From: Maarten Lankhorst Message-ID: Date: Mon, 13 Sep 2021 10:42:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [Intel-gfx] [PATCH v2] kernel/locking: Add context to ww_mutex_trylock. X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" Op 10-09-2021 om 17:02 schreef Peter Zijlstra: > On Thu, Sep 09, 2021 at 11:32:18AM +0200, Maarten Lankhorst wrote: >> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c >> index d456579d0952..791c28005eef 100644 >> --- a/kernel/locking/mutex.c >> +++ b/kernel/locking/mutex.c >> @@ -736,6 +736,44 @@ __ww_mutex_lock(struct mutex *lock, unsigned int state, unsigned int subclass, >> return __mutex_lock_common(lock, state, subclass, NULL, ip, ww_ctx, true); >> } >> >> +/** >> + * ww_mutex_trylock - tries to acquire the w/w mutex with optional acquire context >> + * @lock: mutex to lock >> + * @ctx: optional w/w acquire context >> + * >> + * Trylocks a mutex with the optional acquire context; no deadlock detection is >> + * possible. Returns 1 if the mutex has been acquired successfully, 0 otherwise. >> + * >> + * Unlike ww_mutex_lock, no deadlock handling is performed. However, if a @ctx is >> + * specified, -EALREADY and -EDEADLK handling may happen in calls to ww_mutex_lock. >> + * >> + * A mutex acquired with this function must be released with ww_mutex_unlock. >> + */ >> +int __sched >> +ww_mutex_trylock(struct ww_mutex *ww, struct ww_acquire_ctx *ctx) >> +{ >> + bool locked; >> + >> + if (!ctx) >> + return mutex_trylock(&ww->base); >> + >> +#ifdef CONFIG_DEBUG_MUTEXES >> + DEBUG_LOCKS_WARN_ON(ww->base.magic != &ww->base); >> +#endif >> + >> + preempt_disable(); >> + locked = __mutex_trylock(&ww->base); >> + >> + if (locked) { >> + ww_mutex_set_context_fastpath(ww, ctx); >> + mutex_acquire_nest(&ww->base.dep_map, 0, 1, &ctx->dep_map, _RET_IP_); >> + } >> + preempt_enable(); >> + >> + return locked; >> +} >> +EXPORT_SYMBOL(ww_mutex_trylock); >> + >> #ifdef CONFIG_DEBUG_LOCK_ALLOC >> void __sched >> mutex_lock_nested(struct mutex *lock, unsigned int subclass) >> diff --git a/kernel/locking/ww_rt_mutex.c b/kernel/locking/ww_rt_mutex.c >> index 3f1fff7d2780..c4cb863edb4c 100644 >> --- a/kernel/locking/ww_rt_mutex.c >> +++ b/kernel/locking/ww_rt_mutex.c >> @@ -50,6 +50,18 @@ __ww_rt_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ww_ctx, >> return ret; >> } >> >> +int __sched >> +ww_mutex_trylock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx) >> +{ >> + int locked = rt_mutex_trylock(&lock->base); >> + >> + if (locked && ctx) >> + ww_mutex_set_context_fastpath(lock, ctx); >> + >> + return locked; >> +} >> +EXPORT_SYMBOL(ww_mutex_trylock); >> + >> int __sched >> ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx) >> { > That doesn't look right, how's this for you? > > --- > --- a/kernel/locking/mutex.c > +++ b/kernel/locking/mutex.c > @@ -94,6 +94,9 @@ static inline unsigned long __owner_flag > return owner & MUTEX_FLAGS; > } > > +/* > + * Returns: __mutex_owner(lock) on failure or NULL on success. > + */ > static inline struct task_struct *__mutex_trylock_common(struct mutex *lock, bool handoff) > { > unsigned long owner, curr = (unsigned long)current; > @@ -736,6 +739,47 @@ __ww_mutex_lock(struct mutex *lock, unsi > return __mutex_lock_common(lock, state, subclass, NULL, ip, ww_ctx, true); > } > > +/** > + * ww_mutex_trylock - tries to acquire the w/w mutex with optional acquire context > + * @ww: mutex to lock > + * @ww_ctx: optional w/w acquire context > + * > + * Trylocks a mutex with the optional acquire context; no deadlock detection is > + * possible. Returns 1 if the mutex has been acquired successfully, 0 otherwise. > + * > + * Unlike ww_mutex_lock, no deadlock handling is performed. However, if a @ctx is > + * specified, -EALREADY handling may happen in calls to ww_mutex_trylock. > + * > + * A mutex acquired with this function must be released with ww_mutex_unlock. > + */ > +int ww_mutex_trylock(struct ww_mutex *ww, struct ww_acquire_ctx *ww_ctx) > +{ > + if (!ww_ctx) > + return mutex_trylock(&ww->base); > + > + MUTEX_WARN_ON(ww->base.magic != &ww->base); > + > + if (unlikely(ww_ctx == READ_ONCE(ww->ctx))) > + return -EALREADY; I'm not 100% sure this is a good idea, because it would make the trylock weird. For i915 I checked manually, because I didn't want to change the function signature. This is probably the other extreme. "if (ww_mutex_trylock())" would look correct, but actually be wrong and lead to double unlock without adjustments. Maybe we could make a ww_mutex_trylock_ctx_err, which would return -EALREADY or -EBUSY on failure, and 0 on success? We could keep ww_mutex_trylock without ctx, probably just #define as (!ww_mutex_trylock_ctx_err(lock, NULL)) > + /* > + * Reset the wounded flag after a kill. No other process can > + * race and wound us here, since they can't have a valid owner > + * pointer if we don't have any locks held. > + */ > + if (ww_ctx->acquired == 0) > + ww_ctx->wounded = 0; Yeah I guess this needs fixing too. Not completely sure since trylock wouldn't do the whole ww dance, but since it's our first lock, probably best to do so regardless so other users don't trip over it. > + > + if (__mutex_trylock(&ww->base)) { > + ww_mutex_set_context_fastpath(ww, ww_ctx); > + mutex_acquire_nest(&ww->base.dep_map, 0, 1, &ww_ctx->dep_map, _RET_IP_); > + return 1; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(ww_mutex_trylock); > + > #ifdef CONFIG_DEBUG_LOCK_ALLOC > void __sched > mutex_lock_nested(struct mutex *lock, unsigned int subclass) > --- a/kernel/locking/ww_rt_mutex.c > +++ b/kernel/locking/ww_rt_mutex.c > @@ -9,6 +9,34 @@ > #define WW_RT > #include "rtmutex.c" > > +int ww_mutex_trylock(struct ww_mutex *lock, struct ww_acquire_ctx *ww_ctx) > +{ > + struct rt_mutex *rtm = &lock->base; > + > + if (!ww_ctx) > + return rt_mutex_trylock(rtm); > + > + if (unlikely(ww_ctx == READ_ONCE(lock->ctx))) > + return -EALREADY; > + > + /* > + * Reset the wounded flag after a kill. No other process can > + * race and wound us here, since they can't have a valid owner > + * pointer if we don't have any locks held. > + */ > + if (ww_ctx->acquired == 0) > + ww_ctx->wounded = 0; > + > + if (__rt_mutex_trylock(&rtm->rtmutex)) { > + ww_mutex_set_context_fastpath(lock, ww_ctx); > + mutex_acquire_nest(&rtm->dep_map, 0, 1, ww_ctx->dep_map, _RET_IP_); > + return 1; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(ww_mutex_trylock); > + > static int __sched > __ww_rt_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ww_ctx, > unsigned int state, unsigned long ip)