| Message ID | 20231029230205.93277-9-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers | expand |
On Mon, Oct 30, 2023 at 02:01:47AM +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. > > 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 6e02643ed87e..41b749bedb11 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; > +} > + Wait, so the locked suffix is to indicate that we need to take the lock before we call it? I think that's the opposite to all(?) the naming convention we have Especially since the function name doesn't describe what the function does anymore, but the context in which to call it. I'm sure if I was to use it, I would have gotten it wrong, or at the very least been very confused about it. Maxime
On Fri, 24 Nov 2023 11:47:57 +0100 Maxime Ripard <mripard@kernel.org> wrote: > On Mon, Oct 30, 2023 at 02:01:47AM +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. > > > > 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 6e02643ed87e..41b749bedb11 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; > > +} > > + > > Wait, so the locked suffix is to indicate that we need to take the lock > before we call it? I think that's the opposite to all(?) the naming > convention we have If you grep for "_locked(" and "_unlocked(" in the DRM sub-tree, you'll see it's actually mixed, with maybe a few more helpers suffixed _locked() than we have suffixed with _unlocked(). > > Especially since the function name doesn't describe what the function > does anymore, but the context in which to call it. Well, that's the same for "_unlocked", and we do have to pick one of the _locked/_unlocked pattern if we want to expose both flavors. > I'm sure if I was to > use it, I would have gotten it wrong, or at the very least been very > confused about it. I personally find both equally confusing tbh, but we do have cases where we need to expose the exact same functionality without the extra locking. I do have a slight preference for _locked though, because it's two characters shorter ;-).
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 6e02643ed87e..41b749bedb11 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;