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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C0CE6E7490C for ; Mon, 2 Oct 2023 19:28:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238849AbjJBT2Z (ORCPT ); Mon, 2 Oct 2023 15:28:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54110 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238548AbjJBT2X (ORCPT ); Mon, 2 Oct 2023 15:28:23 -0400 Received: from madras.collabora.co.uk (madras.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6F452B4 for ; Mon, 2 Oct 2023 12:28:19 -0700 (PDT) Received: from [192.168.2.166] (109-252-153-31.dynamic.spd-mgts.ru [109.252.153.31]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: dmitry.osipenko) by madras.collabora.co.uk (Postfix) with ESMTPSA id 3908266072E6; Mon, 2 Oct 2023 20:28:16 +0100 (BST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1696274897; bh=VncolUacwShJRXUnmRFLyNX7YGgpYmIuNvpYd4udSIs=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=KEHflzPQ9DpVtLthaJdLTKw7qMODOTSkU18JJqTBWR8Bzlqkad44+s6IeoqGrNGiQ ajCXOu4CKtvps3Ld6VgRq6sxQGLKKj/NPVCH9N3uGxUKEHApesimkBjUGHPPEJUhsq LJNgObx33Q0INSSrEzKRsLr8CO0yIxzrmrfu+wBBgDQDYulSImwjIwTyEeFMTkOvx4 GFGLm0i9/KhbWehto8MfIebxXdXcHPda2BwYfjYQgsdh8eyRKsN5A5d5vyaqtQibaa NhzvlA/6X5kNDJuOWzG68VOr3ylWFwqLnBcIdo/VJi+Xfb85ZlyiuzMCXLTbMUSgbk ymVUxyV8DiR4A== Message-ID: Date: Mon, 2 Oct 2023 22:28:13 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH v17 13/18] drm/shmem-helper: Add memory shrinker To: Boris Brezillon Cc: David Airlie , Gerd Hoffmann , Gurchetan Singh , Chia-I Wu , Daniel Vetter , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , =?UTF-8?Q?Christian_K=c3=b6nig?= , Qiang Yu , Steven Price , Emma Anholt , Melissa Wen , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, kernel@collabora.com, virtualization@lists.linux-foundation.org References: <20230914232721.408581-1-dmitry.osipenko@collabora.com> <20230914232721.408581-14-dmitry.osipenko@collabora.com> <20230915104633.0d5c3932@collabora.com> <454c464e-4534-7ec3-6d38-49b7df83c7be@collabora.com> <20230926093517.11a172ad@collabora.com> Content-Language: en-US From: Dmitry Osipenko In-Reply-To: <20230926093517.11a172ad@collabora.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 9/26/23 10:35, Boris Brezillon wrote: >> On 9/15/23 11:46, Boris Brezillon wrote: >>> The naming becomes quite confusing, with drm_gem_shmem_unpin_locked() >>> and drm_gem_shmem_unpin_pages_locked(). By the look of it, it seems to >>> do exactly the opposite of drm_gem_shmem_swapin_locked(), except for >>> the missing ->evicted = true, which we can move here anyway, given >>> drm_gem_shmem_purge_locked() explicitly set it to false anyway. The >>> other thing that's missing is the >>> drm_gem_shmem_update_pages_state_locked(), but it can also be moved >>> there I think, if the the ->madv update happens before the >>> drm_gem_shmem_unpin_pages_locked() call in >>> drm_gem_shmem_purge_locked(). >>> >>> So, how about renaming this function drm_gem_shmem_swapout_locked()? >> The swapout name would be misleading to me because pages aren't moved to >> swap, but allowed to be moved. I'll rename it to >> drm_gem_shmem_shrinker_unpin_locked(). > If you go this way, I would argue that drm_gem_shmem_swapin_locked() is > just as incorrect as drm_gem_shmem_swapout_locked(), in that > drm_gem_get_pages() might just return pages that were flagged > reclaimable but never reclaimed/swapped-out. I do think that having > some symmetry in the naming makes more sense than being 100% accurate. That function is internal to drm-shmem and is used for both eviction and purging. Having "swap-out" invoked by the purging also doesn't sound good. Given that the function in question mainly "unmaps" the pages, what about drm_gem_shmem_shkinker_unmap_pages_locked()? >>>> { >>>> struct drm_gem_object *obj = &shmem->base; >>>> struct drm_device *dev = obj->dev; >>>> >>>> dma_resv_assert_held(shmem->base.resv); >>>> >>>> - drm_WARN_ON(obj->dev, !drm_gem_shmem_is_purgeable(shmem)); >>>> + if (shmem->evicted) >>>> + return; >>>> >>>> dma_unmap_sgtable(dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0); >>> Are we sure we'll always have sgt != NULL? IIRC, if the GEM is only >>> mmap-ed in userspace, get_sgt() is not necessarily called by the driver >>> (needed to map in GPU space), and we have a potential NULL deref here. >>> Maybe that changed at some point in the series, and sgt is >>> unconditionally populated when get_pages() is called now. >> The sgt is always set in this function because it's part of shrinker and >> shrinker doesn't touch GEMs without sgt. > Okay, that's questionable. Why would we not want to reclaim BOs that > are only mapped in userspace (sgt == NULL && pages_use_count > 0 && > pages_pin_count == 0)? I agree that creating such a BO would be > pointless (why create a buffer through DRM if it's not passed to the > GPU), but that's still something the API allows... This is a pre-existing behaviour. There is no driver that uses pages without sgt, hence there is nobody to test such code paths. Maybe will worth to explicitly prohibit usage of get_pages() without having sgt for clarity. But this should be separate to this patchset, IMO. -- Best regards, Dmitry