Message ID | 20240105184624.508603-10-dmitry.osipenko@collabora.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-18247-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:6f82:b0:100:9c79:88ff with SMTP id tb2csp6403925dyb; Fri, 5 Jan 2024 10:51:08 -0800 (PST) X-Google-Smtp-Source: AGHT+IF0BLNSJvsAR+lxfehZCWdx82B/DxDuwBZwkOPFtmJATgAKX+91Cx8R5nBBSbKAeveARHxd X-Received: by 2002:a17:902:74cb:b0:1d3:479d:3d56 with SMTP id f11-20020a17090274cb00b001d3479d3d56mr2585963plt.133.1704480667926; Fri, 05 Jan 2024 10:51:07 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1704480667; cv=none; d=google.com; s=arc-20160816; b=X8mRrYL8lz3SPRuvURbkFekKvz3rhxq0r3kfWRa0Pl4qjVpqiZrS0xmoGtAOvEmqt4 4izz5/lJn8TsvIjCIaOZLwOzfHO3dP6vhkH30cL0cmz30Gsj8QdqpRg3aJIfGeIPrFn3 oZ3GTKJN2uFOs9fhjG/rAtEZvc2aybINhnoaB5ARkTWz/0oFFMDOilmj39cQyU8eoab6 AN9vpg2OVzaOLkctPVYHi+nPNyO4UNRDMxg3Nx/1aUu1qL0+0+XeTiICTtFxKVa8eLUl EJkMrrCitW/IlqC1llL8VAHsQ8bIiz4J9N3wwyAB17nX8v5bBY9BEl8/UZwwDpFwcEvz zMFg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=39sf9oMKuAIqdIjA45ye+e3dPV6ZRcHHnGFV7k7jF+A=; fh=5cel2jD5h+yPMXVwxbomyVhwojHUqATy6nFjd4aOh4o=; b=nNKTdcwoP46FM5hHFBHhuljGfMulNltW8pEp85Y+EZkO/rFY8JkHKb0dhJBqNgXV6h oGtzkGMKBtA8/Pak0G3ix4rzDdknjWNjZ+yBeOur68/tgxpvGj7YXQy4TkLViJltqgFq t7J1SETm5G9LgHKC7qah9AzSwsB9i22/Xpte42XKBrpP3EeliQ9+1F1y/Qo+q99IQnPC 25K9Gx0a1aJ56BLnaVhTVdK9//re1Yrw0zjPvCtRkGSRI0FQYLCY+Nn7OJTIdy81oncw d0O0tXmiT2nuyGx5Ku1cpIHngXcLEbeQVS/6WjkGTQIsUAXbHtrh13bcN5jat/fbjDRG m3jQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b=3FX6C1Gf; spf=pass (google.com: domain of linux-kernel+bounces-18247-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) smtp.mailfrom="linux-kernel+bounces-18247-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org. [147.75.48.161]) by mx.google.com with ESMTPS id j13-20020a170902c3cd00b001d4f32f16desi868057plj.319.2024.01.05.10.51.07 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 05 Jan 2024 10:51:07 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-18247-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161; Authentication-Results: mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b=3FX6C1Gf; spf=pass (google.com: domain of linux-kernel+bounces-18247-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) smtp.mailfrom="linux-kernel+bounces-18247-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id 208F7B22A58 for <ouuuleilei@gmail.com>; Fri, 5 Jan 2024 18:49:38 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 475F7381B5; Fri, 5 Jan 2024 18:47:04 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b="3FX6C1Gf" X-Original-To: linux-kernel@vger.kernel.org Received: from madrid.collaboradmins.com (madrid.collaboradmins.com [46.235.227.194]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 530C935EFD for <linux-kernel@vger.kernel.org>; Fri, 5 Jan 2024 18:47:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1704480419; bh=hwlCMWKCwWq+FTlAozKtV1c6NXEsmQgXkAKTAl4/P/Y=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=3FX6C1Gf+urqihuAHe188P3CvAOPc3Kru2MKDUDKN4uTIEgbX3lMfS9y7FyoeG8B9 Od2AE3+82Ow23PkbPb0rI9E8sEGsKmBlo0Rta6o7+qoFlficKQS7Od//ZdEIEYnbIz fkaSUhuJpTba3VPNM9Y+RVgZ2hc+SDTVd3n8HrWGqMpJIknqswaqzG4ny49Ylf/Oxa nu98Ylxpv2q+wUnZh7UVFWlzX3VrldN2MGRP1uCbBh/3ae9XsiqY1YX9FMx6V0yKjM 5mqniy6HoKmSITyLbrfdwGuzsoZnLq9AshXLBCSonPtVuc+7cxoqYJY4t+JsV7lpCO DI4yKiNy9aRbw== Received: from workpc.. (cola.collaboradmins.com [195.201.22.229]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: dmitry.osipenko) by madrid.collaboradmins.com (Postfix) with ESMTPSA id 934B33782047; Fri, 5 Jan 2024 18:46:57 +0000 (UTC) From: Dmitry Osipenko <dmitry.osipenko@collabora.com> To: David Airlie <airlied@gmail.com>, Gerd Hoffmann <kraxel@redhat.com>, Gurchetan Singh <gurchetansingh@chromium.org>, Chia-I Wu <olvaffe@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, =?utf-8?q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>, Qiang Yu <yuq825@gmail.com>, Steven Price <steven.price@arm.com>, Boris Brezillon <boris.brezillon@collabora.com>, Emma Anholt <emma@anholt.net>, Melissa Wen <mwen@igalia.com> Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, kernel@collabora.com, virtualization@lists.linux-foundation.org Subject: [PATCH v19 09/30] drm/shmem-helper: Add and use lockless drm_gem_shmem_get_pages() Date: Fri, 5 Jan 2024 21:46:03 +0300 Message-ID: <20240105184624.508603-10-dmitry.osipenko@collabora.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240105184624.508603-1-dmitry.osipenko@collabora.com> References: <20240105184624.508603-1-dmitry.osipenko@collabora.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1787277520874044664 X-GMAIL-MSGID: 1787277520874044664 |
Series |
Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers
|
|
Commit Message
Dmitry Osipenko
Jan. 5, 2024, 6:46 p.m. UTC
Add lockless drm_gem_shmem_get_pages() helper that skips taking reservation lock if pages_use_count is non-zero, leveraging from atomicity of the refcount_t. Make drm_gem_shmem_mmap() to utilize the new helper. Acked-by: Maxime Ripard <mripard@kernel.org> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> --- drivers/gpu/drm/drm_gem_shmem_helper.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
Comments
On Fri, Jan 05, 2024 at 09:46:03PM +0300, Dmitry Osipenko wrote: > Add lockless drm_gem_shmem_get_pages() helper that skips taking reservation > lock if pages_use_count is non-zero, leveraging from atomicity of the > refcount_t. Make drm_gem_shmem_mmap() to utilize the new helper. > > Acked-by: Maxime Ripard <mripard@kernel.org> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index cacf0f8c42e2..1c032513abf1 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -226,6 +226,20 @@ void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > } > EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked); > > +static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) > +{ > + int ret; Just random drive-by comment: a might_lock annotation here might be good, or people could hit some really interesting bugs that are rather hard to reproduce ... -Sima > + > + if (refcount_inc_not_zero(&shmem->pages_use_count)) > + return 0; > + > + dma_resv_lock(shmem->base.resv, NULL); > + ret = drm_gem_shmem_get_pages_locked(shmem); > + dma_resv_unlock(shmem->base.resv); > + > + return ret; > +} > + > static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem) > { > int ret; > @@ -609,10 +623,7 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct > return ret; > } > > - dma_resv_lock(shmem->base.resv, NULL); > - ret = drm_gem_shmem_get_pages_locked(shmem); > - dma_resv_unlock(shmem->base.resv); > - > + ret = drm_gem_shmem_get_pages(shmem); > if (ret) > return ret; > > -- > 2.43.0 >
On 1/25/24 20:24, Daniel Vetter wrote: > On Fri, Jan 05, 2024 at 09:46:03PM +0300, Dmitry Osipenko wrote: >> Add lockless drm_gem_shmem_get_pages() helper that skips taking reservation >> lock if pages_use_count is non-zero, leveraging from atomicity of the >> refcount_t. Make drm_gem_shmem_mmap() to utilize the new helper. >> >> Acked-by: Maxime Ripard <mripard@kernel.org> >> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> >> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> --- >> drivers/gpu/drm/drm_gem_shmem_helper.c | 19 +++++++++++++++---- >> 1 file changed, 15 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >> index cacf0f8c42e2..1c032513abf1 100644 >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >> @@ -226,6 +226,20 @@ void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) >> } >> EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked); >> >> +static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) >> +{ >> + int ret; > > Just random drive-by comment: a might_lock annotation here might be good, > or people could hit some really interesting bugs that are rather hard to > reproduce ... > -Sima Thanks for the suggestion!
On Thu, 25 Jan 2024 18:24:04 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Jan 05, 2024 at 09:46:03PM +0300, Dmitry Osipenko wrote: > > Add lockless drm_gem_shmem_get_pages() helper that skips taking reservation > > lock if pages_use_count is non-zero, leveraging from atomicity of the > > refcount_t. Make drm_gem_shmem_mmap() to utilize the new helper. > > > > Acked-by: Maxime Ripard <mripard@kernel.org> > > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > > Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > --- > > drivers/gpu/drm/drm_gem_shmem_helper.c | 19 +++++++++++++++---- > > 1 file changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > > index cacf0f8c42e2..1c032513abf1 100644 > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > > @@ -226,6 +226,20 @@ void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > > } > > EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked); > > > > +static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) > > +{ > > + int ret; > > Just random drive-by comment: a might_lock annotation here might be good, > or people could hit some really interesting bugs that are rather hard to > reproduce ... Actually, being able to acquire a ref in a dma-signalling path on an object we know for sure already has refcount >= 1 (because we previously acquired a ref in a path where dma_resv_lock() was allowed), was the primary reason I suggested moving to this atomic-refcount approach. In the meantime, drm_gpuvm has evolved in a way that allows me to not take the ref in the dma-signalling path (the gpuvm_bo object now holds the ref, and it's acquired/released outside the dma-signalling path). Not saying we shouldn't add this might_lock(), but others might have good reasons to have this function called in a path where locking is not allowed.
On 1/26/24 13:18, Boris Brezillon wrote: > On Thu, 25 Jan 2024 18:24:04 +0100 > Daniel Vetter <daniel@ffwll.ch> wrote: > >> On Fri, Jan 05, 2024 at 09:46:03PM +0300, Dmitry Osipenko wrote: >>> Add lockless drm_gem_shmem_get_pages() helper that skips taking reservation >>> lock if pages_use_count is non-zero, leveraging from atomicity of the >>> refcount_t. Make drm_gem_shmem_mmap() to utilize the new helper. >>> >>> Acked-by: Maxime Ripard <mripard@kernel.org> >>> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> >>> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> >>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>> --- >>> drivers/gpu/drm/drm_gem_shmem_helper.c | 19 +++++++++++++++---- >>> 1 file changed, 15 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >>> index cacf0f8c42e2..1c032513abf1 100644 >>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >>> @@ -226,6 +226,20 @@ void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) >>> } >>> EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked); >>> >>> +static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) >>> +{ >>> + int ret; >> >> Just random drive-by comment: a might_lock annotation here might be good, >> or people could hit some really interesting bugs that are rather hard to >> reproduce ... > > Actually, being able to acquire a ref in a dma-signalling path on an > object we know for sure already has refcount >= 1 (because we previously > acquired a ref in a path where dma_resv_lock() was allowed), was the > primary reason I suggested moving to this atomic-refcount approach. > > In the meantime, drm_gpuvm has evolved in a way that allows me to not > take the ref in the dma-signalling path (the gpuvm_bo object now holds > the ref, and it's acquired/released outside the dma-signalling path). > > Not saying we shouldn't add this might_lock(), but others might have > good reasons to have this function called in a path where locking > is not allowed. For Panthor the might_lock indeed won't be a appropriate, thanks for reminding about it. I'll add explanatory comment to the code.
On Fri, Jan 26, 2024 at 07:43:29PM +0300, Dmitry Osipenko wrote: > On 1/26/24 13:18, Boris Brezillon wrote: > > On Thu, 25 Jan 2024 18:24:04 +0100 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > >> On Fri, Jan 05, 2024 at 09:46:03PM +0300, Dmitry Osipenko wrote: > >>> Add lockless drm_gem_shmem_get_pages() helper that skips taking reservation > >>> lock if pages_use_count is non-zero, leveraging from atomicity of the > >>> refcount_t. Make drm_gem_shmem_mmap() to utilize the new helper. > >>> > >>> Acked-by: Maxime Ripard <mripard@kernel.org> > >>> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > >>> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> > >>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >>> --- > >>> drivers/gpu/drm/drm_gem_shmem_helper.c | 19 +++++++++++++++---- > >>> 1 file changed, 15 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > >>> index cacf0f8c42e2..1c032513abf1 100644 > >>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > >>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > >>> @@ -226,6 +226,20 @@ void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > >>> } > >>> EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked); > >>> > >>> +static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) > >>> +{ > >>> + int ret; > >> > >> Just random drive-by comment: a might_lock annotation here might be good, > >> or people could hit some really interesting bugs that are rather hard to > >> reproduce ... > > > > Actually, being able to acquire a ref in a dma-signalling path on an > > object we know for sure already has refcount >= 1 (because we previously > > acquired a ref in a path where dma_resv_lock() was allowed), was the > > primary reason I suggested moving to this atomic-refcount approach. > > > > In the meantime, drm_gpuvm has evolved in a way that allows me to not > > take the ref in the dma-signalling path (the gpuvm_bo object now holds > > the ref, and it's acquired/released outside the dma-signalling path). > > > > Not saying we shouldn't add this might_lock(), but others might have > > good reasons to have this function called in a path where locking > > is not allowed. > > For Panthor the might_lock indeed won't be a appropriate, thanks for > reminding about it. I'll add explanatory comment to the code. Hm these kind of tricks feel very dangerous to me. I think it would be good to split up the two cases into two functions: 1. first one does only the atomic_inc and splats if the refcount is zero. I think something in the name that denotes that we're incrementing a borrowed pages reference would be good here, so like get_borrowed_pages (there's not really a naming convention for these in the kernel). Unfortunately no rust so we can't enforce that you provide the right kind of borrowed reference at compile time. 2. second one has the might_lock. This way you force callers to think what they're doing and ideally document where the borrowed reference is from, and ideally document that in the code. Otherwise we'll end up with way too much "works in testing, but is a nice CVE" code :-/ Cheers, Sima
On 1/30/24 11:34, Daniel Vetter wrote: > On Fri, Jan 26, 2024 at 07:43:29PM +0300, Dmitry Osipenko wrote: >> On 1/26/24 13:18, Boris Brezillon wrote: >>> On Thu, 25 Jan 2024 18:24:04 +0100 >>> Daniel Vetter <daniel@ffwll.ch> wrote: >>> >>>> On Fri, Jan 05, 2024 at 09:46:03PM +0300, Dmitry Osipenko wrote: >>>>> Add lockless drm_gem_shmem_get_pages() helper that skips taking reservation >>>>> lock if pages_use_count is non-zero, leveraging from atomicity of the >>>>> refcount_t. Make drm_gem_shmem_mmap() to utilize the new helper. >>>>> >>>>> Acked-by: Maxime Ripard <mripard@kernel.org> >>>>> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> >>>>> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> >>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>>> --- >>>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 19 +++++++++++++++---- >>>>> 1 file changed, 15 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >>>>> index cacf0f8c42e2..1c032513abf1 100644 >>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >>>>> @@ -226,6 +226,20 @@ void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) >>>>> } >>>>> EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked); >>>>> >>>>> +static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) >>>>> +{ >>>>> + int ret; >>>> >>>> Just random drive-by comment: a might_lock annotation here might be good, >>>> or people could hit some really interesting bugs that are rather hard to >>>> reproduce ... >>> >>> Actually, being able to acquire a ref in a dma-signalling path on an >>> object we know for sure already has refcount >= 1 (because we previously >>> acquired a ref in a path where dma_resv_lock() was allowed), was the >>> primary reason I suggested moving to this atomic-refcount approach. >>> >>> In the meantime, drm_gpuvm has evolved in a way that allows me to not >>> take the ref in the dma-signalling path (the gpuvm_bo object now holds >>> the ref, and it's acquired/released outside the dma-signalling path). >>> >>> Not saying we shouldn't add this might_lock(), but others might have >>> good reasons to have this function called in a path where locking >>> is not allowed. >> >> For Panthor the might_lock indeed won't be a appropriate, thanks for >> reminding about it. I'll add explanatory comment to the code. > > Hm these kind of tricks feel very dangerous to me. I think it would be > good to split up the two cases into two functions: > > 1. first one does only the atomic_inc and splats if the refcount is zero. > I think something in the name that denotes that we're incrementing a > borrowed pages reference would be good here, so like get_borrowed_pages > (there's not really a naming convention for these in the kernel). > Unfortunately no rust so we can't enforce that you provide the right kind > of borrowed reference at compile time. > > 2. second one has the might_lock. > > This way you force callers to think what they're doing and ideally > document where the borrowed reference is from, and ideally document that > in the code. Otherwise we'll end up with way too much "works in testing, > but is a nice CVE" code :-/ We indeed can have both variants of the borrowed/non-borrowed functions. Thanks again for the suggestions
On Tue, 30 Jan 2024 09:34:29 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Jan 26, 2024 at 07:43:29PM +0300, Dmitry Osipenko wrote: > > On 1/26/24 13:18, Boris Brezillon wrote: > > > On Thu, 25 Jan 2024 18:24:04 +0100 > > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > >> On Fri, Jan 05, 2024 at 09:46:03PM +0300, Dmitry Osipenko wrote: > > >>> Add lockless drm_gem_shmem_get_pages() helper that skips taking reservation > > >>> lock if pages_use_count is non-zero, leveraging from atomicity of the > > >>> refcount_t. Make drm_gem_shmem_mmap() to utilize the new helper. > > >>> > > >>> Acked-by: Maxime Ripard <mripard@kernel.org> > > >>> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > > >>> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> > > >>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > >>> --- > > >>> drivers/gpu/drm/drm_gem_shmem_helper.c | 19 +++++++++++++++---- > > >>> 1 file changed, 15 insertions(+), 4 deletions(-) > > >>> > > >>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > > >>> index cacf0f8c42e2..1c032513abf1 100644 > > >>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > > >>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > > >>> @@ -226,6 +226,20 @@ void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > > >>> } > > >>> EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked); > > >>> > > >>> +static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) > > >>> +{ > > >>> + int ret; > > >> > > >> Just random drive-by comment: a might_lock annotation here might be good, > > >> or people could hit some really interesting bugs that are rather hard to > > >> reproduce ... > > > > > > Actually, being able to acquire a ref in a dma-signalling path on an > > > object we know for sure already has refcount >= 1 (because we previously > > > acquired a ref in a path where dma_resv_lock() was allowed), was the > > > primary reason I suggested moving to this atomic-refcount approach. > > > > > > In the meantime, drm_gpuvm has evolved in a way that allows me to not > > > take the ref in the dma-signalling path (the gpuvm_bo object now holds > > > the ref, and it's acquired/released outside the dma-signalling path). > > > > > > Not saying we shouldn't add this might_lock(), but others might have > > > good reasons to have this function called in a path where locking > > > is not allowed. > > > > For Panthor the might_lock indeed won't be a appropriate, thanks for > > reminding about it. I'll add explanatory comment to the code. > > Hm these kind of tricks feel very dangerous to me. I think it would be > good to split up the two cases into two functions: > > 1. first one does only the atomic_inc and splats if the refcount is zero. > I think something in the name that denotes that we're incrementing a > borrowed pages reference would be good here, so like get_borrowed_pages > (there's not really a naming convention for these in the kernel). > Unfortunately no rust so we can't enforce that you provide the right kind > of borrowed reference at compile time. Yeah, I also considered adding a dedicated function for that use case at some point, instead of abusing get_pages(). Given I no longer need it, we can probably add this might_lock() and defer the addition of this get_borrowed_pages() helper until someone actually needs it. > > 2. second one has the might_lock. > > This way you force callers to think what they're doing and ideally > document where the borrowed reference is from, and ideally document that > in the code. Otherwise we'll end up with way too much "works in testing, > but is a nice CVE" code :-/ Totally agree with you on that point.
On 1/30/24 13:10, Boris Brezillon wrote: > On Tue, 30 Jan 2024 09:34:29 +0100 > Daniel Vetter <daniel@ffwll.ch> wrote: > >> On Fri, Jan 26, 2024 at 07:43:29PM +0300, Dmitry Osipenko wrote: >>> On 1/26/24 13:18, Boris Brezillon wrote: >>>> On Thu, 25 Jan 2024 18:24:04 +0100 >>>> Daniel Vetter <daniel@ffwll.ch> wrote: >>>> >>>>> On Fri, Jan 05, 2024 at 09:46:03PM +0300, Dmitry Osipenko wrote: >>>>>> Add lockless drm_gem_shmem_get_pages() helper that skips taking reservation >>>>>> lock if pages_use_count is non-zero, leveraging from atomicity of the >>>>>> refcount_t. Make drm_gem_shmem_mmap() to utilize the new helper. >>>>>> >>>>>> Acked-by: Maxime Ripard <mripard@kernel.org> >>>>>> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> >>>>>> Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> >>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>>>> --- >>>>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 19 +++++++++++++++---- >>>>>> 1 file changed, 15 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >>>>>> index cacf0f8c42e2..1c032513abf1 100644 >>>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >>>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >>>>>> @@ -226,6 +226,20 @@ void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked); >>>>>> >>>>>> +static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) >>>>>> +{ >>>>>> + int ret; >>>>> >>>>> Just random drive-by comment: a might_lock annotation here might be good, >>>>> or people could hit some really interesting bugs that are rather hard to >>>>> reproduce ... >>>> >>>> Actually, being able to acquire a ref in a dma-signalling path on an >>>> object we know for sure already has refcount >= 1 (because we previously >>>> acquired a ref in a path where dma_resv_lock() was allowed), was the >>>> primary reason I suggested moving to this atomic-refcount approach. >>>> >>>> In the meantime, drm_gpuvm has evolved in a way that allows me to not >>>> take the ref in the dma-signalling path (the gpuvm_bo object now holds >>>> the ref, and it's acquired/released outside the dma-signalling path). >>>> >>>> Not saying we shouldn't add this might_lock(), but others might have >>>> good reasons to have this function called in a path where locking >>>> is not allowed. >>> >>> For Panthor the might_lock indeed won't be a appropriate, thanks for >>> reminding about it. I'll add explanatory comment to the code. >> >> Hm these kind of tricks feel very dangerous to me. I think it would be >> good to split up the two cases into two functions: >> >> 1. first one does only the atomic_inc and splats if the refcount is zero. >> I think something in the name that denotes that we're incrementing a >> borrowed pages reference would be good here, so like get_borrowed_pages >> (there's not really a naming convention for these in the kernel). >> Unfortunately no rust so we can't enforce that you provide the right kind >> of borrowed reference at compile time. > > Yeah, I also considered adding a dedicated function for that use case > at some point, instead of abusing get_pages(). Given I no longer need > it, we can probably add this might_lock() and defer the addition of this > get_borrowed_pages() helper until someone actually needs it. Ack, I'll add the might_lock() then. Missed previously that you don't need to use get_pages() anymore. Thanks
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index cacf0f8c42e2..1c032513abf1 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -226,6 +226,20 @@ void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) } EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked); +static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem) +{ + int ret; + + if (refcount_inc_not_zero(&shmem->pages_use_count)) + return 0; + + dma_resv_lock(shmem->base.resv, NULL); + ret = drm_gem_shmem_get_pages_locked(shmem); + dma_resv_unlock(shmem->base.resv); + + return ret; +} + static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem) { int ret; @@ -609,10 +623,7 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct return ret; } - dma_resv_lock(shmem->base.resv, NULL); - ret = drm_gem_shmem_get_pages_locked(shmem); - dma_resv_unlock(shmem->base.resv); - + ret = drm_gem_shmem_get_pages(shmem); if (ret) return ret;