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=-19.0 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, 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 70F36C43214 for ; Mon, 16 Aug 2021 13:34:24 +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 3E9426054F for ; Mon, 16 Aug 2021 13:34:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 3E9426054F 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 5DDF789E9B; Mon, 16 Aug 2021 13:34:22 +0000 (UTC) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by gabe.freedesktop.org (Postfix) with ESMTPS id C550689E2B; Mon, 16 Aug 2021 13:34:15 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10077"; a="196128353" X-IronPort-AV: E=Sophos;i="5.84,326,1620716400"; d="scan'208";a="196128353" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Aug 2021 06:34:15 -0700 X-IronPort-AV: E=Sophos;i="5.84,326,1620716400"; d="scan'208";a="530527900" Received: from ttulbure-mobl1.ger.corp.intel.com (HELO [10.252.56.52]) ([10.252.56.52]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Aug 2021 06:34:14 -0700 To: =?UTF-8?Q?Thomas_Hellstr=c3=b6m?= , Matthew Auld Cc: Intel Graphics Development , ML dri-devel References: <20210816084855.75586-1-thomas.hellstrom@linux.intel.com> From: Maarten Lankhorst Message-ID: <00736b43-8abf-ff15-e9dd-5ed54da77379@linux.intel.com> Date: Mon, 16 Aug 2021 15:34:13 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [Intel-gfx] [PATCH] drm/i915: Ditch the i915_gem_ww_ctx loop member 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 16-08-2021 om 15:30 schreef Thomas Hellström: > > On 8/16/21 3:25 PM, Matthew Auld wrote: >> On Mon, 16 Aug 2021 at 09:49, Thomas Hellström >> wrote: >>> It's only used by the for_i915_gem_ww() macro and we can use >>> the (typically) on-stack _err variable in its place. >>> >>> While initially setting the _err variable to -EDEADLK to enter the >>> loop, we clear it before actually entering using fetch_and_zero() to >>> avoid empty loops or code not setting the _err variable running forever. >>> >>> Suggested-by: Maarten Lankhorst >>> Signed-off-by: Thomas Hellström >>> --- >>>   drivers/gpu/drm/i915/i915_gem_ww.h | 23 ++++++++--------------- >>>   1 file changed, 8 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem_ww.h b/drivers/gpu/drm/i915/i915_gem_ww.h >>> index f6b1a796667b..98348b1e6182 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem_ww.h >>> +++ b/drivers/gpu/drm/i915/i915_gem_ww.h >>> @@ -7,12 +7,13 @@ >>> >>>   #include >>> >>> +#include "i915_utils.h" >>> + >>>   struct i915_gem_ww_ctx { >>>          struct ww_acquire_ctx ctx; >>>          struct list_head obj_list; >>>          struct drm_i915_gem_object *contended; >>> -       unsigned short intr; >>> -       unsigned short loop; >>> +       bool intr; >>>   }; >>> >>>   void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr); >>> @@ -23,28 +24,20 @@ void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj); >>>   /* Internal functions used by the inlines! Don't use. */ >>>   static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err) >>>   { >>> -       ww->loop = 0; >>>          if (err == -EDEADLK) { >>>                  err = i915_gem_ww_ctx_backoff(ww); >>>                  if (!err) >>> -                       ww->loop = 1; >>> +                       err = -EDEADLK; >>>          } >>> >>> -       if (!ww->loop) >>> +       if (err != -EDEADLK) >>>                  i915_gem_ww_ctx_fini(ww); >>> >>>          return err; >>>   } >>> >>> -static inline void >>> -__i915_gem_ww_init(struct i915_gem_ww_ctx *ww, bool intr) >>> -{ >>> -       i915_gem_ww_ctx_init(ww, intr); >>> -       ww->loop = 1; >>> -} >>> - >>> -#define for_i915_gem_ww(_ww, _err, _intr)                      \ >>> -       for (__i915_gem_ww_init(_ww, _intr); (_ww)->loop;       \ >>> +#define for_i915_gem_ww(_ww, _err, _intr)                        \ >>> +       for (i915_gem_ww_ctx_init(_ww, _intr), (_err) = -EDEADLK; \ >>> +            fetch_and_zero(&_err) == -EDEADLK;                   \ >> Doesn't this now hide "normal" errors, like say get_pages() returning >> -ENOSPC or so? > > Yes, good catch. We should either just clear the -EDEADLK case, or not clear the error at all.. > > /Thomas I believe not setting _err is a bug anyway. Why would you do such a loop without at least one err = ww_mutex_lock(&ww); ? Infinite loop would catch that at first test. ~Maarten