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 5FF41CCA473 for ; Fri, 24 Jun 2022 20:23:56 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 21F1710F10D; Fri, 24 Jun 2022 20:23:55 +0000 (UTC) Received: from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com [IPv6:2a00:1450:4864:20::52c]) by gabe.freedesktop.org (Postfix) with ESMTPS id B848010F10D for ; Fri, 24 Jun 2022 20:23:53 +0000 (UTC) Received: by mail-ed1-x52c.google.com with SMTP id ej4so4975285edb.7 for ; Fri, 24 Jun 2022 13:23:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=rM6StsZ/3rmWNZ6eG3nFC9QNenxnUOTVwI/HAfEm2EQ=; b=VYViBveaMX2iHovNjImE8ft1aL8oby/bDvJodcDA4y4Eo+tutY1hGinHsM3aKTvLEE UKfzf6+eCMmxZ49Ku37S9pc+tHn/uh5pXrCXh3E8Ggw/FSYRFeIyB8RDclfGgyvtHmNh MqpcvSiLK6jgmcJHZD8u3uLJ1nBF6f8mOUmtg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=rM6StsZ/3rmWNZ6eG3nFC9QNenxnUOTVwI/HAfEm2EQ=; b=L8xe2kIMaLcPZsHHfPasXq1MlIvFbXB/k00em/cLfEV0IT/Nd64PdsHLeWog5Rtx01 xCzRQKm7f7w+2KdW8oP0Aq51V1NI9Ly5G3JLCB7W5rkxbO7EHl5Q7rss1yw1C+ls+Axx A/LL7mCflCHIKpKOq0YgfkxDLd8dM2ekMKKc/nrKW4h7L40z6aTQOXOqZfxBfpsthdEf W0J4M2Br3rDpC8QdchFjdxxUGb+RmyVTIBmWwqykHG43uEwG91iUhNSwXZI7pe+XNjRP aZDlMHi4TTcjur3BpCkguARxH1lTYap+sSAzQjj7pfQ5eKuuZUJKLVG9LB7fpDHiIWhw fQ9Q== X-Gm-Message-State: AJIora9V7lEbH/PMNUVFslBR/iIA4ephucCN0dDn2Rc4QHlKDx7SMw3u jjb/Cc1arpCDF5TTQM09e2SlMg== X-Google-Smtp-Source: AGRyM1uJnErMjUto5d2AkMD9qcjjZpgW235kvjYxoK3tSLlU8a0umCcexISHAvYdIuLK226BGQhyeA== X-Received: by 2002:a05:6402:4248:b0:435:9150:ccfb with SMTP id g8-20020a056402424800b004359150ccfbmr1101365edb.374.1656102232134; Fri, 24 Jun 2022 13:23:52 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id z23-20020a170906435700b007094f98788csm1630637ejm.113.2022.06.24.13.23.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 Jun 2022 13:23:51 -0700 (PDT) Date: Fri, 24 Jun 2022 22:23:49 +0200 From: Daniel Vetter To: Rob Clark Subject: Re: [PATCH v6 17/22] drm/shmem-helper: Add generic memory shrinker Message-ID: Mail-Followup-To: Rob Clark , Dmitry Osipenko , David Airlie , Gerd Hoffmann , Gurchetan Singh , Chia-I Wu , Daniel Almeida , Gert Wollny , Gustavo Padovan , Daniel Stone , Tomeu Vizoso , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , Rob Herring , Steven Price , Alyssa Rosenzweig , Emil Velikov , Robin Murphy , Qiang Yu , Sumit Semwal , Christian =?iso-8859-1?Q?K=F6nig?= , "Pan, Xinhui" , Thierry Reding , Tomasz Figa , Marek Szyprowski , Mauro Carvalho Chehab , Alex Deucher , Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , Tvrtko Ursulin , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Dmitry Osipenko , linux-tegra@vger.kernel.org, linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org, amd-gfx@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, kernel@collabora.com References: <20220526235040.678984-1-dmitry.osipenko@collabora.com> <20220526235040.678984-18-dmitry.osipenko@collabora.com> <3bb3dc53-69fc-8cdb-ae37-583b9b2660a3@collabora.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux phenom 5.10.0-8-amd64 X-BeenThere: amd-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: David Airlie , Joonas Lahtinen , dri-devel@lists.freedesktop.org, Gurchetan Singh , Thierry Reding , Gerd Hoffmann , Dmitry Osipenko , kernel@collabora.com, Sumit Semwal , Marek Szyprowski , Rob Herring , Daniel Stone , Steven Price , Gustavo Padovan , Alyssa Rosenzweig , Dmitry Osipenko , virtualization@lists.linux-foundation.org, Chia-I Wu , linux-media@vger.kernel.org, Daniel Vetter , intel-gfx@lists.freedesktop.org, Maarten Lankhorst , Maxime Ripard , linaro-mm-sig@lists.linaro.org, Jani Nikula , Rodrigo Vivi , linux-tegra@vger.kernel.org, Mauro Carvalho Chehab , Tvrtko Ursulin , Daniel Almeida , amd-gfx@lists.freedesktop.org, Tomeu Vizoso , Gert Wollny , "Pan, Xinhui" , Emil Velikov , linux-kernel@vger.kernel.org, Tomasz Figa , Qiang Yu , Thomas Zimmermann , Alex Deucher , Robin Murphy , Christian =?iso-8859-1?Q?K=F6nig?= Errors-To: amd-gfx-bounces@lists.freedesktop.org Sender: "amd-gfx" On Mon, Jun 20, 2022 at 08:18:04AM -0700, Rob Clark wrote: > On Mon, Jun 20, 2022 at 7:09 AM Dmitry Osipenko > wrote: > > > > On 6/19/22 20:53, Rob Clark wrote: > > ... > > >> +static unsigned long > > >> +drm_gem_shmem_shrinker_count_objects(struct shrinker *shrinker, > > >> + struct shrink_control *sc) > > >> +{ > > >> + struct drm_gem_shmem_shrinker *gem_shrinker = to_drm_shrinker(shrinker); > > >> + struct drm_gem_shmem_object *shmem; > > >> + unsigned long count = 0; > > >> + > > >> + if (!mutex_trylock(&gem_shrinker->lock)) > > >> + return 0; > > >> + > > >> + list_for_each_entry(shmem, &gem_shrinker->lru_evictable, madv_list) { > > >> + count += shmem->base.size; > > >> + > > >> + if (count >= SHRINK_EMPTY) > > >> + break; > > >> + } > > >> + > > >> + mutex_unlock(&gem_shrinker->lock); > > > > > > As I mentioned on other thread, count_objects, being approximate but > > > lockless and fast is the important thing. Otherwise when you start > > > hitting the shrinker on many threads, you end up serializing them all, > > > even if you have no pages to return to the system at that point. > > > > Daniel's point for dropping the lockless variant was that we're already > > in trouble if we're hitting shrinker too often and extra optimizations > > won't bring much benefits to us. > > At least with zram swap (which I highly recommend using even if you > are not using a physical swap file/partition), swapin/out is actually > quite fast. And if you are leaning on zram swap to fit 8GB of chrome > browser on a 4GB device, the shrinker gets hit quite a lot. Lower > spec (4GB RAM) chromebooks can be under constant memory pressure and > can quite easily get into a situation where you are hitting the > shrinker on many threads simultaneously. So it is pretty important > for all shrinkers in the system (not just drm driver) to be as > concurrent as possible. As long as you avoid serializing reclaim on > all the threads, performance can still be quite good, but if you don't > performance will fall off a cliff. > > jfwiw, we are seeing pretty good results (iirc 40-70% increase in open > tab counts) with the combination of eviction + multigen LRU[1] + > sizing zram swap to be 2x physical RAM > > [1] https://lwn.net/Articles/856931/ > > > Alright, I'll add back the lockless variant (or will use yours > > drm_gem_lru) in the next revision. The code difference is very small > > after all. > > > > ... > > >> + /* prevent racing with the dma-buf importing/exporting */ > > >> + if (!mutex_trylock(&gem_shrinker->dev->object_name_lock)) { > > >> + *lock_contention |= true; > > >> + goto resv_unlock; > > >> + } > > > > > > I'm not sure this is a good idea to serialize on object_name_lock. > > > Purgeable buffers should never be shared (imported or exported). So > > > at best you are avoiding evicting and immediately swapping back in, in > > > a rare case, at the cost of serializing multiple threads trying to > > > reclaim pages in parallel. > > > > The object_name_lock shouldn't cause contention in practice. But objects > > are also pinned on attachment, hence maybe this lock is indeed > > unnecessary.. I'll re-check it. > > I'm not worried about contention with export/import/etc, but > contention between multiple threads hitting the shrinker in parallel. > I guess since you are using trylock, it won't *block* the other > threads hitting shrinker, but they'll just end up looping in > do_shrink_slab() because they are hitting contention. > > I'd have to do some experiments to see how it works out in practice, > but my gut feel is that it isn't a good idea Yeah trylock on anything else than the object lock is No Good in the shrinker. And it really shouldn't be needed, since import/export should pin stuff as needed. Which should be protected by the dma_resv object lock. If not, we need to fix that. Picking a random drm-internal lock like this is definitely no good design. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch