| Message ID | 20230314022659.1816246-4-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 |
Hi Dmitry, Sorry for chiming in only now :-/. On Tue, 14 Mar 2023 05:26:52 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > And new pages_pin_count field to struct drm_gem_shmem_object that will > determine whether pages are evictable by memory shrinker. The pages will > be evictable only when pages_pin_count=0. This patch prepares code for > addition of the memory shrinker that will utilize the new field. > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 7 +++++++ > include/drm/drm_gem_shmem_helper.h | 9 +++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 4da9c9c39b9a..81d61791f874 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -277,6 +277,8 @@ static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem) > drm_WARN_ON(obj->dev, obj->import_attach); > > ret = drm_gem_shmem_get_pages(shmem); > + if (!ret) > + shmem->pages_pin_count++; > > return ret; > } > @@ -289,7 +291,12 @@ static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem) > > drm_WARN_ON(obj->dev, obj->import_attach); > > + if (drm_WARN_ON_ONCE(obj->dev, !shmem->pages_pin_count)) > + return; > + > drm_gem_shmem_put_pages(shmem); > + > + shmem->pages_pin_count--; > } > > /** > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > index 20ddcd799df9..7d823c9fc480 100644 > --- a/include/drm/drm_gem_shmem_helper.h > +++ b/include/drm/drm_gem_shmem_helper.h > @@ -39,6 +39,15 @@ struct drm_gem_shmem_object { > */ > unsigned int pages_use_count; > > + /** > + * @pages_pin_count: > + * > + * Reference count on the pinned pages table. > + * The pages allowed to be evicted by memory shrinker > + * only when the count is zero. > + */ > + unsigned int pages_pin_count; s/pages_pin_count/pin_count/ ? And do we really need both pages_pin_count and pages_use_count. Looks like they both serve the same purpose, with one exception: pages_use_count is also incremented in the get_pages_sgt_locked() path, but you probably don't want it to prevent GEM eviction. Assuming your goal with this pin_count field is to check if a GEM object is evictable, it can be done with something like bool drm_gem_shmem_is_evictable_locked(struct drm_gem_shmem_object *shmem) { dma_resv_assert_held(shmem->base.resv); return shmem->pages_use_count == (shmem->sgt ? 1 : 0); } I mean, I'm not against renaming pages_use_count into pin_count, but, unless I'm missing something, I don't see a good reason to keep both. Regards, Boris
On Mon, 26 Jun 2023 17:04:57 +0200 Boris Brezillon <boris.brezillon@collabora.com> wrote: > Hi Dmitry, > > Sorry for chiming in only now :-/. > > On Tue, 14 Mar 2023 05:26:52 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > > And new pages_pin_count field to struct drm_gem_shmem_object that will > > determine whether pages are evictable by memory shrinker. The pages will > > be evictable only when pages_pin_count=0. This patch prepares code for > > addition of the memory shrinker that will utilize the new field. > > > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > --- > > drivers/gpu/drm/drm_gem_shmem_helper.c | 7 +++++++ > > include/drm/drm_gem_shmem_helper.h | 9 +++++++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > > index 4da9c9c39b9a..81d61791f874 100644 > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > > @@ -277,6 +277,8 @@ static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem) > > drm_WARN_ON(obj->dev, obj->import_attach); > > > > ret = drm_gem_shmem_get_pages(shmem); > > + if (!ret) > > + shmem->pages_pin_count++; > > > > return ret; > > } > > @@ -289,7 +291,12 @@ static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem) > > > > drm_WARN_ON(obj->dev, obj->import_attach); > > > > + if (drm_WARN_ON_ONCE(obj->dev, !shmem->pages_pin_count)) > > + return; > > + > > drm_gem_shmem_put_pages(shmem); > > + > > + shmem->pages_pin_count--; > > } > > > > /** > > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > > index 20ddcd799df9..7d823c9fc480 100644 > > --- a/include/drm/drm_gem_shmem_helper.h > > +++ b/include/drm/drm_gem_shmem_helper.h > > @@ -39,6 +39,15 @@ struct drm_gem_shmem_object { > > */ > > unsigned int pages_use_count; > > > > + /** > > + * @pages_pin_count: > > + * > > + * Reference count on the pinned pages table. > > + * The pages allowed to be evicted by memory shrinker > > + * only when the count is zero. > > + */ > > + unsigned int pages_pin_count; > > s/pages_pin_count/pin_count/ ? > > And do we really need both pages_pin_count and pages_use_count. Looks > like they both serve the same purpose, with one exception: > pages_use_count is also incremented in the get_pages_sgt_locked() path, > but you probably don't want it to prevent GEM eviction. Assuming > your goal with this pin_count field is to check if a GEM object is > evictable, it can be done with something like > > bool > drm_gem_shmem_is_evictable_locked(struct drm_gem_shmem_object *shmem) > { > dma_resv_assert_held(shmem->base.resv); > > return shmem->pages_use_count == (shmem->sgt ? 1 : 0); > } > > I mean, I'm not against renaming pages_use_count into pin_count, but, > unless I'm missing something, I don't see a good reason to keep both. My bad, I think I found one place calling drm_gem_shmem_get_pages() where we want pin_count and pages_use_count to differ: drm_gem_shmem_mmap(). We certainly don't want userspace mappings to prevent eviction.
On 6/26/23 18:21, Boris Brezillon wrote: > On Mon, 26 Jun 2023 17:04:57 +0200 > Boris Brezillon <boris.brezillon@collabora.com> wrote: > >> Hi Dmitry, >> >> Sorry for chiming in only now :-/. >> >> On Tue, 14 Mar 2023 05:26:52 +0300 >> Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: >> >>> And new pages_pin_count field to struct drm_gem_shmem_object that will >>> determine whether pages are evictable by memory shrinker. The pages will >>> be evictable only when pages_pin_count=0. This patch prepares code for >>> addition of the memory shrinker that will utilize the new field. >>> >>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>> --- >>> drivers/gpu/drm/drm_gem_shmem_helper.c | 7 +++++++ >>> include/drm/drm_gem_shmem_helper.h | 9 +++++++++ >>> 2 files changed, 16 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >>> index 4da9c9c39b9a..81d61791f874 100644 >>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >>> @@ -277,6 +277,8 @@ static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem) >>> drm_WARN_ON(obj->dev, obj->import_attach); >>> >>> ret = drm_gem_shmem_get_pages(shmem); >>> + if (!ret) >>> + shmem->pages_pin_count++; >>> >>> return ret; >>> } >>> @@ -289,7 +291,12 @@ static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem) >>> >>> drm_WARN_ON(obj->dev, obj->import_attach); >>> >>> + if (drm_WARN_ON_ONCE(obj->dev, !shmem->pages_pin_count)) >>> + return; >>> + >>> drm_gem_shmem_put_pages(shmem); >>> + >>> + shmem->pages_pin_count--; >>> } >>> >>> /** >>> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h >>> index 20ddcd799df9..7d823c9fc480 100644 >>> --- a/include/drm/drm_gem_shmem_helper.h >>> +++ b/include/drm/drm_gem_shmem_helper.h >>> @@ -39,6 +39,15 @@ struct drm_gem_shmem_object { >>> */ >>> unsigned int pages_use_count; >>> >>> + /** >>> + * @pages_pin_count: >>> + * >>> + * Reference count on the pinned pages table. >>> + * The pages allowed to be evicted by memory shrinker >>> + * only when the count is zero. >>> + */ >>> + unsigned int pages_pin_count; >> >> s/pages_pin_count/pin_count/ ? >> >> And do we really need both pages_pin_count and pages_use_count. Looks >> like they both serve the same purpose, with one exception: >> pages_use_count is also incremented in the get_pages_sgt_locked() path, >> but you probably don't want it to prevent GEM eviction. Assuming >> your goal with this pin_count field is to check if a GEM object is >> evictable, it can be done with something like >> >> bool >> drm_gem_shmem_is_evictable_locked(struct drm_gem_shmem_object *shmem) >> { >> dma_resv_assert_held(shmem->base.resv); >> >> return shmem->pages_use_count == (shmem->sgt ? 1 : 0); >> } >> >> I mean, I'm not against renaming pages_use_count into pin_count, but, >> unless I'm missing something, I don't see a good reason to keep both. > > My bad, I think I found one place calling drm_gem_shmem_get_pages() > where we want pin_count and pages_use_count to differ: > drm_gem_shmem_mmap(). We certainly don't want userspace mappings to > prevent eviction. That's correct, thanks for the review :)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 4da9c9c39b9a..81d61791f874 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -277,6 +277,8 @@ static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem) drm_WARN_ON(obj->dev, obj->import_attach); ret = drm_gem_shmem_get_pages(shmem); + if (!ret) + shmem->pages_pin_count++; return ret; } @@ -289,7 +291,12 @@ static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem) drm_WARN_ON(obj->dev, obj->import_attach); + if (drm_WARN_ON_ONCE(obj->dev, !shmem->pages_pin_count)) + return; + drm_gem_shmem_put_pages(shmem); + + shmem->pages_pin_count--; } /** diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h index 20ddcd799df9..7d823c9fc480 100644 --- a/include/drm/drm_gem_shmem_helper.h +++ b/include/drm/drm_gem_shmem_helper.h @@ -39,6 +39,15 @@ struct drm_gem_shmem_object { */ unsigned int pages_use_count; + /** + * @pages_pin_count: + * + * Reference count on the pinned pages table. + * The pages allowed to be evicted by memory shrinker + * only when the count is zero. + */ + unsigned int pages_pin_count; + /** * @madv: State for madvise *
And new pages_pin_count field to struct drm_gem_shmem_object that will determine whether pages are evictable by memory shrinker. The pages will be evictable only when pages_pin_count=0. This patch prepares code for addition of the memory shrinker that will utilize the new field. Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> --- drivers/gpu/drm/drm_gem_shmem_helper.c | 7 +++++++ include/drm/drm_gem_shmem_helper.h | 9 +++++++++ 2 files changed, 16 insertions(+)