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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 15C30C433EF for ; Wed, 8 Dec 2021 08:31:46 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 34DD16E523; Wed, 8 Dec 2021 08:31:45 +0000 (UTC) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 656CF6E523; Wed, 8 Dec 2021 08:31:44 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10191"; a="261859205" X-IronPort-AV: E=Sophos;i="5.87,297,1631602800"; d="scan'208";a="261859205" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Dec 2021 00:30:56 -0800 X-IronPort-AV: E=Sophos;i="5.87,296,1631602800"; d="scan'208";a="515671165" Received: from amgotede-mobl1.ger.corp.intel.com (HELO [10.213.194.97]) ([10.213.194.97]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Dec 2021 00:30:55 -0800 Message-ID: <64b203a7-b09f-2982-ef3b-b33da7708d0f@linux.intel.com> Date: Wed, 8 Dec 2021 08:30:53 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.1 Content-Language: en-US To: Matthew Auld , intel-gfx@lists.freedesktop.org References: <20211018091055.1998191-1-matthew.auld@intel.com> <20211018091055.1998191-2-matthew.auld@intel.com> <1a8431eb-566d-ac2b-85b7-31c590ec84ff@linux.intel.com> <52fadb30-bdc2-6432-931b-ef1bbf3be0ba@intel.com> From: Tvrtko Ursulin Organization: Intel Corporation UK Plc In-Reply-To: <52fadb30-bdc2-6432-931b-ef1bbf3be0ba@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Intel-gfx] [PATCH v9 2/8] drm/i915/ttm: add tt shmem backend 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: , Cc: =?UTF-8?Q?Thomas_Hellstr=c3=b6m?= , =?UTF-8?Q?Christian_K=c3=b6nig?= , dri-devel@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On 07/12/2021 14:05, Matthew Auld wrote: > On 07/12/2021 13:10, Tvrtko Ursulin wrote: >> >> On 18/10/2021 10:10, Matthew Auld wrote: >>> For cached objects we can allocate our pages directly in shmem. This >>> should make it possible(in a later patch) to utilise the existing >>> i915-gem shrinker code for such objects. For now this is still disabled. >>> >>> v2(Thomas): >>>    - Add optional try_to_writeback hook for objects. Importantly we need >>>      to check if the object is even still shrinkable; in between us >>>      dropping the shrinker LRU lock and acquiring the object lock it >>> could for >>>      example have been moved. Also we need to differentiate between >>>      "lazy" shrinking and the immediate writeback mode. Also later we >>> need to >>>      handle objects which don't even have mm.pages, so bundling this >>> into >>>      put_pages() would require somehow handling that edge case, hence >>>      just letting the ttm backend handle everything in try_to_writeback >>>      doesn't seem too bad. >>> v3(Thomas): >>>    - Likely a bad idea to touch the object from the unpopulate hook, >>>      since it's not possible to hold a reference, without also creating >>>      circular dependency, so likely this is too fragile. For now just >>>      ensure we at least mark the pages as dirty/accessed when called >>> from the >>>      shrinker on WILLNEED objects. >>>    - s/try_to_writeback/shrinker_release_pages, since this can do more >>>      than just writeback. >>>    - Get rid of do_backup boolean and just set the SWAPPED flag prior to >>>      calling unpopulate. >>>    - Keep shmem_tt as lowest priority for the TTM LRU bo_swapout >>> walk, since >>>      these just get skipped anyway. We can try to come up with something >>>      better later. >>> v4(Thomas): >>>    - s/PCI_DMA/DMA/. Also drop NO_KERNEL_MAPPING and NO_WARN, which >>>      apparently doesn't do anything with streaming mappings. >>>    - Just pass along the error for ->truncate, and assume nothing. >>> >>> Signed-off-by: Matthew Auld >>> Cc: Thomas Hellström >>> Cc: Christian König >>> Cc: Oak Zeng >>> Reviewed-by: Thomas Hellström >>> Acked-by: Oak Zeng >> >> [snip] >> >>> -static void try_to_writeback(struct drm_i915_gem_object *obj, >>> -                 unsigned int flags) >>> +static int try_to_writeback(struct drm_i915_gem_object *obj, >>> unsigned int flags) >>>   { >>> +    if (obj->ops->shrinker_release_pages) >>> +        return obj->ops->shrinker_release_pages(obj, >>> +                            flags & I915_SHRINK_WRITEBACK); >> >> I have a high level question about how does this new vfunc fit with >> the existing code. >> >> Because I notice in the implementation >> (i915_ttm_shrinker_release_pages) it ends up doing: >> ... >> >>         switch (obj->mm.madv) { >>         case I915_MADV_DONTNEED: >>                 return i915_ttm_purge(obj); >>         case __I915_MADV_PURGED: >>                 return 0; >>         } >> >> ... and then... >> >>         if (should_writeback) >>                 __shmem_writeback(obj->base.size, >> i915_tt->filp->f_mapping); >> >> Which on a glance looks like a possible conceptual duplication of the >> concepts in this very function (try_to_writeback): >> >>> + >>>       switch (obj->mm.madv) { >>>       case I915_MADV_DONTNEED: >>>           i915_gem_object_truncate(obj); >>> -        return; >>> +        return 0; >>>       case __I915_MADV_PURGED: >>> -        return; >>> +        return 0; >>>       } >>>       if (flags & I915_SHRINK_WRITEBACK) >>>           i915_gem_object_writeback(obj); >> >> So question is this the final design or some futher tidy is >> possible/planned? > > It seems ok to me, all things considered. The TTM version needs to check > if the object is still shrinkable at the start(plus some other stuff), > upon acquiring the object lock. If that succeeds we can do the above > madv checks and potentially just call truncate. Otherwise we can proceed > with shrinking, but again TTM is special here, and we have to call > ttm_bo_validate() underneath(we might not even have mm.pages here). And > then if that all works we might be able to also perform the writeback, > if needed. So I suppose we could add all that directly in > try_to_writeback(), and make it conditional for TTM devices, or I guess > we need two separate hooks, one to do some pre-checking and another do > the actual swap step. Not sure if that is better or worse though. Would implementing the shrinker_release_pages for all objects work? It would contain what currently is in try_to_writeback and so the two paths, if not compatible, would diverge cleanly on the same level of indirection. I mean we would not have two effectively mutually exclusive vfuncs (shrinker_release_pages and writeback) and could unexport i915_gem_object_writeback. >> Background to my question is that I will float a patch which proposes >> to remove writeback altogether. There are reports from the fields that >> it causes deadlocks and looking at 2d6692e642e7 ("drm/i915: Start >> writeback from the shrinker") and its history it seems like it was a >> known risk. >> >> Apart from the code organisation questions, on the practical level - >> do you need writeback from the TTM backend or while I am proposing to >> remove it from the "legacy" paths, I can propose removing it from the >> TTM flow as well? > > Yeah, if that is somehow busted then we should remove from TTM backend > also. Okay thanks, I wanted to check in case there was an extra need in TTM. I will float a patch soon hopefully but testing will be a problem since it seems very hard to repro at the moment. Regards, Tvrtko