| Message ID | 20230622085335.77010-6-zhengqi.arch@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | use refcount+RCU method to implement lockless slab shrink | expand |
Hi Steven, The email you replied to was the failed version (due to the error below), so I copied your reply and replied to you on this successful version. (4.7.1 Error: too many recipients from 49.7.199.173) On 2023/6/23 18:01, Steven Price wrote: > On 22/06/2023 09:39, Qi Zheng wrote: >> From: Qi Zheng <zhengqi.arch@bytedance.com> >> >> In preparation for implementing lockless slab shrink, >> we need to dynamically allocate the drm-panfrost shrinker, >> so that it can be freed asynchronously using kfree_rcu(). >> Then it doesn't need to wait for RCU read-side critical >> section when releasing the struct panfrost_device. >> >> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >> --- >> drivers/gpu/drm/panfrost/panfrost_device.h | 2 +- >> .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 24 ++++++++++--------- >> 2 files changed, 14 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h >> index b0126b9fbadc..e667e5689353 100644 >> --- a/drivers/gpu/drm/panfrost/panfrost_device.h >> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h >> @@ -118,7 +118,7 @@ struct panfrost_device { >> >> struct mutex shrinker_lock; >> struct list_head shrinker_list; >> - struct shrinker shrinker; >> + struct shrinker *shrinker; >> >> struct panfrost_devfreq pfdevfreq; >> }; >> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c >> index bf0170782f25..2a5513eb9e1f 100644 >> --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c >> +++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c >> @@ -18,8 +18,7 @@ >> static unsigned long >> panfrost_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc) >> { >> - struct panfrost_device *pfdev = >> - container_of(shrinker, struct panfrost_device, shrinker); >> + struct panfrost_device *pfdev = shrinker->private_data; >> struct drm_gem_shmem_object *shmem; >> unsigned long count = 0; >> >> @@ -65,8 +64,7 @@ static bool panfrost_gem_purge(struct drm_gem_object *obj) >> static unsigned long >> panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) >> { >> - struct panfrost_device *pfdev = >> - container_of(shrinker, struct panfrost_device, shrinker); >> + struct panfrost_device *pfdev = shrinker->private_data; >> struct drm_gem_shmem_object *shmem, *tmp; >> unsigned long freed = 0; >> >> @@ -100,10 +98,15 @@ panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) >> void panfrost_gem_shrinker_init(struct drm_device *dev) >> { >> struct panfrost_device *pfdev = dev->dev_private; >> - pfdev->shrinker.count_objects = panfrost_gem_shrinker_count; >> - pfdev->shrinker.scan_objects = panfrost_gem_shrinker_scan; >> - pfdev->shrinker.seeks = DEFAULT_SEEKS; >> - WARN_ON(register_shrinker(&pfdev->shrinker, "drm-panfrost")); >> + >> + pfdev->shrinker = shrinker_alloc_and_init(panfrost_gem_shrinker_count, >> + panfrost_gem_shrinker_scan, 0, >> + DEFAULT_SEEKS, 0, pfdev); >> + if (pfdev->shrinker && >> + register_shrinker(pfdev->shrinker, "drm-panfrost")) { >> + shrinker_free(pfdev->shrinker); >> + WARN_ON(1); >> + } > > So we didn't have good error handling here before, but this is > significantly worse. Previously if register_shrinker() failed then the > driver could safely continue without a shrinker - it would waste memory > but still function. > > However we now have two failure conditions: > * shrinker_alloc_init() returns NULL. No warning and NULL deferences > will happen later. > > * register_shrinker() fails, shrinker_free() will free pdev->shrinker > we get a warning, but followed by a use-after-free later. > > I think we need to modify panfrost_gem_shrinker_init() to be able to > return an error, so a change something like the below (untested) before > your change. Indeed. I will fix it in the v2. Thanks, Qi > > Steve > > ----8<--- > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > b/drivers/gpu/drm/panfrost/panfrost_drv.c > index bbada731bbbd..f705bbdea360 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -598,10 +598,14 @@ static int panfrost_probe(struct platform_device > *pdev) > if (err < 0) > goto err_out1; > > - panfrost_gem_shrinker_init(ddev); > + err = panfrost_gem_shrinker_init(ddev); > + if (err) > + goto err_out2; > > return 0; > > +err_out2: > + drm_dev_unregister(ddev); > err_out1: > pm_runtime_disable(pfdev->dev); > panfrost_device_fini(pfdev); > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h > b/drivers/gpu/drm/panfrost/panfrost_gem.h > index ad2877eeeccd..863d2ec8d4f0 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem.h > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h > @@ -81,7 +81,7 @@ panfrost_gem_mapping_get(struct panfrost_gem_object *bo, > void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping); > void panfrost_gem_teardown_mappings_locked(struct panfrost_gem_object *bo); > > -void panfrost_gem_shrinker_init(struct drm_device *dev); > +int panfrost_gem_shrinker_init(struct drm_device *dev); > void panfrost_gem_shrinker_cleanup(struct drm_device *dev); > > #endif /* __PANFROST_GEM_H__ */ > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c > b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c > index bf0170782f25..90265b37636f 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c > +++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c > @@ -97,13 +97,17 @@ panfrost_gem_shrinker_scan(struct shrinker > *shrinker, struct shrink_control *sc) > * > * This function registers and sets up the panfrost shrinker. > */ > -void panfrost_gem_shrinker_init(struct drm_device *dev) > +int panfrost_gem_shrinker_init(struct drm_device *dev) > { > struct panfrost_device *pfdev = dev->dev_private; > + int ret; > + > pfdev->shrinker.count_objects = panfrost_gem_shrinker_count; > pfdev->shrinker.scan_objects = panfrost_gem_shrinker_scan; > pfdev->shrinker.seeks = DEFAULT_SEEKS; > - WARN_ON(register_shrinker(&pfdev->shrinker, "drm-panfrost")); > + ret = register_shrinker(&pfdev->shrinker, "drm-panfrost"); > + > + return ret; > } > > /** >
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h index b0126b9fbadc..e667e5689353 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.h +++ b/drivers/gpu/drm/panfrost/panfrost_device.h @@ -118,7 +118,7 @@ struct panfrost_device { struct mutex shrinker_lock; struct list_head shrinker_list; - struct shrinker shrinker; + struct shrinker *shrinker; struct panfrost_devfreq pfdevfreq; }; diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c index bf0170782f25..2a5513eb9e1f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c @@ -18,8 +18,7 @@ static unsigned long panfrost_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc) { - struct panfrost_device *pfdev = - container_of(shrinker, struct panfrost_device, shrinker); + struct panfrost_device *pfdev = shrinker->private_data; struct drm_gem_shmem_object *shmem; unsigned long count = 0; @@ -65,8 +64,7 @@ static bool panfrost_gem_purge(struct drm_gem_object *obj) static unsigned long panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) { - struct panfrost_device *pfdev = - container_of(shrinker, struct panfrost_device, shrinker); + struct panfrost_device *pfdev = shrinker->private_data; struct drm_gem_shmem_object *shmem, *tmp; unsigned long freed = 0; @@ -100,10 +98,15 @@ panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) void panfrost_gem_shrinker_init(struct drm_device *dev) { struct panfrost_device *pfdev = dev->dev_private; - pfdev->shrinker.count_objects = panfrost_gem_shrinker_count; - pfdev->shrinker.scan_objects = panfrost_gem_shrinker_scan; - pfdev->shrinker.seeks = DEFAULT_SEEKS; - WARN_ON(register_shrinker(&pfdev->shrinker, "drm-panfrost")); + + pfdev->shrinker = shrinker_alloc_and_init(panfrost_gem_shrinker_count, + panfrost_gem_shrinker_scan, 0, + DEFAULT_SEEKS, 0, pfdev); + if (pfdev->shrinker && + register_shrinker(pfdev->shrinker, "drm-panfrost")) { + shrinker_free(pfdev->shrinker); + WARN_ON(1); + } } /** @@ -116,7 +119,6 @@ void panfrost_gem_shrinker_cleanup(struct drm_device *dev) { struct panfrost_device *pfdev = dev->dev_private; - if (pfdev->shrinker.nr_deferred) { - unregister_shrinker(&pfdev->shrinker); - } + if (pfdev->shrinker->nr_deferred) + unregister_and_free_shrinker(pfdev->shrinker); }
In preparation for implementing lockless slab shrink, we need to dynamically allocate the drm-panfrost shrinker, so that it can be freed asynchronously using kfree_rcu(). Then it doesn't need to wait for RCU read-side critical section when releasing the struct panfrost_device. Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> --- drivers/gpu/drm/panfrost/panfrost_device.h | 2 +- .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 24 ++++++++++--------- 2 files changed, 14 insertions(+), 12 deletions(-)