| Message ID | 20230903170736.513347-3-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 Sun, 3 Sep 2023 20:07:18 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > Use separate flag for tracking page count bumped by shmem->sgt to avoid > imbalanced page counter during of drm_gem_shmem_free() time. It's fragile > to assume that populated shmem->pages at a freeing time means that the > count was bumped by drm_gem_shmem_get_pages_sgt(), using a flag removes > the ambiguity. > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 11 ++++++++++- > drivers/gpu/drm/lima/lima_gem.c | 1 + > include/drm/drm_gem_shmem_helper.h | 7 +++++++ > 3 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 6693d4061ca1..848435e08eb2 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -152,8 +152,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > sg_free_table(shmem->sgt); > kfree(shmem->sgt); > } > - if (shmem->pages) > + if (shmem->pages) { > drm_gem_shmem_put_pages(shmem); > + drm_WARN_ON(obj->dev, !shmem->got_pages_sgt); > + } Already mentioned in v15, but I keep thinking the following: if (shmem->sgt) { // existing code in the preceding // if (shmem->sgt) branch ... /* * Release the implicit pages ref taken in * drm_gem_shmem_get_pages_sgt_locked(). */ drm_gem_shmem_put_pages(shmem); } does exactly the same without requiring the addition of a new field. > > drm_WARN_ON(obj->dev, shmem->pages_use_count); > > @@ -693,6 +695,13 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_ > if (ret) > goto err_free_sgt; > > + /* > + * This flag prevents imbalanced pages_use_count during > + * drm_gem_shmem_free(), where pages_use_count=1 only if > + * drm_gem_shmem_get_pages_sgt() was used by a driver. > + */ > + shmem->got_pages_sgt = true; > + > shmem->sgt = sgt; > > return sgt; > diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c > index 4f9736e5f929..67c39b95e30e 100644 > --- a/drivers/gpu/drm/lima/lima_gem.c > +++ b/drivers/gpu/drm/lima/lima_gem.c > @@ -48,6 +48,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm) > > bo->base.pages = pages; > bo->base.pages_use_count = 1; > + bo->base.got_pages_sgt = true; > > mapping_set_unevictable(mapping); > } > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > index ec70a98a8fe1..a53c0874b3c4 100644 > --- a/include/drm/drm_gem_shmem_helper.h > +++ b/include/drm/drm_gem_shmem_helper.h > @@ -73,6 +73,13 @@ struct drm_gem_shmem_object { > */ > unsigned int vmap_use_count; > > + /** > + * @got_pages_sgt: > + * > + * True if SG table was retrieved using drm_gem_shmem_get_pages_sgt() > + */ > + bool got_pages_sgt : 1; > + > /** > * @imported_sgt: > *
On 9/5/23 10:40, Boris Brezillon wrote: > On Sun, 3 Sep 2023 20:07:18 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > >> Use separate flag for tracking page count bumped by shmem->sgt to avoid >> imbalanced page counter during of drm_gem_shmem_free() time. It's fragile >> to assume that populated shmem->pages at a freeing time means that the >> count was bumped by drm_gem_shmem_get_pages_sgt(), using a flag removes >> the ambiguity. >> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> --- >> drivers/gpu/drm/drm_gem_shmem_helper.c | 11 ++++++++++- >> drivers/gpu/drm/lima/lima_gem.c | 1 + >> include/drm/drm_gem_shmem_helper.h | 7 +++++++ >> 3 files changed, 18 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >> index 6693d4061ca1..848435e08eb2 100644 >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >> @@ -152,8 +152,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >> sg_free_table(shmem->sgt); >> kfree(shmem->sgt); >> } >> - if (shmem->pages) >> + if (shmem->pages) { >> drm_gem_shmem_put_pages(shmem); >> + drm_WARN_ON(obj->dev, !shmem->got_pages_sgt); >> + } > > Already mentioned in v15, but I keep thinking the following: > > if (shmem->sgt) { > // existing code in the preceding > // if (shmem->sgt) branch > ... > > /* > * Release the implicit pages ref taken in > * drm_gem_shmem_get_pages_sgt_locked(). > */ > drm_gem_shmem_put_pages(shmem); > } > > does exactly the same without requiring the addition of a new field. I'll factor out these "flag" patches into separate patchset since they cause too many questions. This is a fix for a minor bug that existed for many years and is difficult to trigger in practice, it can wait. For now will be better to focus on finishing and landing the refcnt and shrinker patches, the rest of drm-shmem core improvements can be done afterwards.
On Tue, 12 Sep 2023 02:41:58 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > On 9/5/23 10:40, Boris Brezillon wrote: > > On Sun, 3 Sep 2023 20:07:18 +0300 > > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > > >> Use separate flag for tracking page count bumped by shmem->sgt to avoid > >> imbalanced page counter during of drm_gem_shmem_free() time. It's fragile > >> to assume that populated shmem->pages at a freeing time means that the > >> count was bumped by drm_gem_shmem_get_pages_sgt(), using a flag removes > >> the ambiguity. > >> > >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >> --- > >> drivers/gpu/drm/drm_gem_shmem_helper.c | 11 ++++++++++- > >> drivers/gpu/drm/lima/lima_gem.c | 1 + > >> include/drm/drm_gem_shmem_helper.h | 7 +++++++ > >> 3 files changed, 18 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > >> index 6693d4061ca1..848435e08eb2 100644 > >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > >> @@ -152,8 +152,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > >> sg_free_table(shmem->sgt); > >> kfree(shmem->sgt); > >> } > >> - if (shmem->pages) > >> + if (shmem->pages) { > >> drm_gem_shmem_put_pages(shmem); > >> + drm_WARN_ON(obj->dev, !shmem->got_pages_sgt); > >> + } > > > > Already mentioned in v15, but I keep thinking the following: > > > > if (shmem->sgt) { > > // existing code in the preceding > > // if (shmem->sgt) branch > > ... > > > > /* > > * Release the implicit pages ref taken in > > * drm_gem_shmem_get_pages_sgt_locked(). > > */ > > drm_gem_shmem_put_pages(shmem); > > } > > > > does exactly the same without requiring the addition of a new field. > > I'll factor out these "flag" patches into separate patchset since they > cause too many questions. So patch 1 and 2 in this series right? > This is a fix for a minor bug that existed for > many years I'm not denying the fact there's a bug, but I'm convinced this can be fixed without adding new flags. If there's something wrong with the suggested solution, I'd be interested to hear more about it. > and is difficult to trigger in practice, it can wait. Triggering it with a real fault is hard, but you can manually fake errors at any point in the initialization process and check what happens. > > For now will be better to focus on finishing and landing the refcnt and > shrinker patches, the rest of drm-shmem core improvements can be done > afterwards. > Sounds good to me.
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 6693d4061ca1..848435e08eb2 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -152,8 +152,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) sg_free_table(shmem->sgt); kfree(shmem->sgt); } - if (shmem->pages) + if (shmem->pages) { drm_gem_shmem_put_pages(shmem); + drm_WARN_ON(obj->dev, !shmem->got_pages_sgt); + } drm_WARN_ON(obj->dev, shmem->pages_use_count); @@ -693,6 +695,13 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_ if (ret) goto err_free_sgt; + /* + * This flag prevents imbalanced pages_use_count during + * drm_gem_shmem_free(), where pages_use_count=1 only if + * drm_gem_shmem_get_pages_sgt() was used by a driver. + */ + shmem->got_pages_sgt = true; + shmem->sgt = sgt; return sgt; diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c index 4f9736e5f929..67c39b95e30e 100644 --- a/drivers/gpu/drm/lima/lima_gem.c +++ b/drivers/gpu/drm/lima/lima_gem.c @@ -48,6 +48,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm) bo->base.pages = pages; bo->base.pages_use_count = 1; + bo->base.got_pages_sgt = true; mapping_set_unevictable(mapping); } diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h index ec70a98a8fe1..a53c0874b3c4 100644 --- a/include/drm/drm_gem_shmem_helper.h +++ b/include/drm/drm_gem_shmem_helper.h @@ -73,6 +73,13 @@ struct drm_gem_shmem_object { */ unsigned int vmap_use_count; + /** + * @got_pages_sgt: + * + * True if SG table was retrieved using drm_gem_shmem_get_pages_sgt() + */ + bool got_pages_sgt : 1; + /** * @imported_sgt: *
Use separate flag for tracking page count bumped by shmem->sgt to avoid imbalanced page counter during of drm_gem_shmem_free() time. It's fragile to assume that populated shmem->pages at a freeing time means that the count was bumped by drm_gem_shmem_get_pages_sgt(), using a flag removes the ambiguity. Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> --- drivers/gpu/drm/drm_gem_shmem_helper.c | 11 ++++++++++- drivers/gpu/drm/lima/lima_gem.c | 1 + include/drm/drm_gem_shmem_helper.h | 7 +++++++ 3 files changed, 18 insertions(+), 1 deletion(-)