Hi Rob,
I can see how we can trigger the shrinker on objB while holding objA->lock. So, the nested lock with class SHRINKER makes sense. However, I’m trying to figure how the get_pages/vmap/fault path on an objA can end up triggering the shrinker on objA itself. As objA itself would not be purgeable (msm_obj->sgt would not be set)/vunmappable (msm_obj->vaddr would not be set) yet at that point, we would never end up calling msm_gem_purge or msm_gem_vunmap on objA itself right? If that is the case, we may not need the (msm_obj->madv == MSM_MADV_WILLNEED) check? Or am I missing something here?
I think shrinker_vmap would also need the nested SHRINKER lock before it calls msm_gem_vunmap because a vmap on objA could trigger msm_gem_vunmap on objB.
I really like the idea of protecting priv->inactive_list with a separate lock as that is pretty much why the shrinker needs to hold struct_mutex.
Thanks, Sushmita
---
This is roughly based on Chris's suggestion, in particular the part
about using mutex_lock_nested(). It's not *exactly* the same, in
particular msm_obj->lock protects a bit more than just backing store
and we don't currently track a pin_count. (Instead we currently
keep pages pinned until the object is purged or freed.)
Instead of making msm_obj->lock only cover backing store, it is
easier to split out madv, which is still protected by struct_mutex,
which is still held by the shrinker, so the shrinker does not need
to grab msm_obj->lock until it purges an object. We avoid going
down any path that could trigger shrinker by ensuring that
msm_obj->madv == WILLNEED. To synchronize access to msm_obj->madv
it is protected by msm_obj->lock inside struct_mutex.
This seems to keep lockdep happy in my testing so far.
drivers/gpu/drm/msm/msm_gem.c | 54 ++++++++++++++++++++++++++++++++--
drivers/gpu/drm/msm/msm_gem.h | 1 +
drivers/gpu/drm/msm/msm_gem_shrinker.c | 12 ++++++++
3 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index e132548..f5d1f84 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -26,6 +26,22 @@
#include "msm_gpu.h"
#include "msm_mmu.h"
+/* The shrinker can be triggered while we hold objA->lock, and need
+ * to grab objB->lock to purge it. Lockdep just sees these as a single
+ * class of lock, so we use subclasses to teach it the difference.
+ *
+ * OBJ_LOCK_NORMAL is implicit (ie. normal mutex_lock() call), and
+ * OBJ_LOCK_SHRINKER is used in msm_gem_purge().
+ *
+ * It is *essential* that we never go down paths that could trigger the
+ * shrinker for a purgable object. This is ensured by checking that
+ * msm_obj->madv == MSM_MADV_WILLNEED.
+ */
+enum {
+ OBJ_LOCK_NORMAL,
+ OBJ_LOCK_SHRINKER,
+};
+
static dma_addr_t physaddr(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
@@ -150,6 +166,12 @@ struct page **msm_gem_get_pages(struct drm_gem_object *obj)
struct page **p;
mutex_lock(&msm_obj->lock);
+
+ if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
+ mutex_unlock(&msm_obj->lock);
+ return ERR_PTR(-EBUSY);
+ }
+
p = get_pages(obj);
mutex_unlock(&msm_obj->lock);
return p;
@@ -220,6 +242,11 @@ int msm_gem_fault(struct vm_fault *vmf)
if (ret)
goto out;
+ if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
+ mutex_unlock(&msm_obj->lock);
+ return VM_FAULT_SIGBUS;
+ }
+
/* make sure we have pages attached now */
pages = get_pages(obj);
if (IS_ERR(pages)) {
@@ -358,6 +385,11 @@ int msm_gem_get_iova(struct drm_gem_object *obj,
mutex_lock(&msm_obj->lock);
+ if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
+ mutex_unlock(&msm_obj->lock);
+ return -EBUSY;
+ }
+
vma = lookup_vma(obj, aspace);
if (!vma) {
@@ -454,6 +486,12 @@ void *msm_gem_get_vaddr(struct drm_gem_object *obj)
struct msm_gem_object *msm_obj = to_msm_bo(obj);
mutex_lock(&msm_obj->lock);
+
+ if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
+ mutex_unlock(&msm_obj->lock);
+ return ERR_PTR(-EBUSY);
+ }
+
if (!msm_obj->vaddr) {
struct page **pages = get_pages(obj);
if (IS_ERR(pages)) {
@@ -489,12 +527,18 @@ int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
+ mutex_lock(&msm_obj->lock);
+
WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
if (msm_obj->madv != __MSM_MADV_PURGED)
msm_obj->madv = madv;
- return (msm_obj->madv != __MSM_MADV_PURGED);
+ madv = msm_obj->madv;
+
+ mutex_unlock(&msm_obj->lock);
+
+ return (madv != __MSM_MADV_PURGED);
}
void msm_gem_purge(struct drm_gem_object *obj)
@@ -506,6 +550,8 @@ void msm_gem_purge(struct drm_gem_object *obj)
WARN_ON(!is_purgeable(msm_obj));
WARN_ON(obj->import_attach);
+ mutex_lock_nested(&msm_obj->lock, OBJ_LOCK_SHRINKER);
+
put_iova(obj);
msm_gem_vunmap(obj);
@@ -526,6 +572,8 @@ void msm_gem_purge(struct drm_gem_object *obj)
invalidate_mapping_pages(file_inode(obj->filp)->i_mapping,
0, (loff_t)-1);
+
+ mutex_unlock(&msm_obj->lock);
}
void msm_gem_vunmap(struct drm_gem_object *obj)
@@ -660,7 +708,7 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m)
uint64_t off = drm_vma_node_start(&obj->vma_node);
const char *madv;
- WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
+ mutex_lock(&msm_obj->lock);
switch (msm_obj->madv) {
case __MSM_MADV_PURGED:
@@ -701,6 +749,8 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m)
if (fence)
describe_fence(fence, "Exclusive", m);
rcu_read_unlock();
+
+ mutex_unlock(&msm_obj->lock);
}
void msm_gem_describe_objects(struct list_head *list, struct seq_file *m)
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 9ad5ba4c..2b9b8e9 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -101,6 +101,7 @@ static inline bool is_active(struct msm_gem_object *msm_obj)
static inline bool is_purgeable(struct msm_gem_object *msm_obj)
{
+ WARN_ON(!mutex_is_locked(&msm_obj->base.dev->struct_mutex));
return (msm_obj->madv == MSM_MADV_DONTNEED) && msm_obj->sgt &&
!msm_obj->base.dma_buf && !msm_obj->base.import_attach;
}
diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c
index ab1dd02..e1db4ad 100644
--- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
+++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
@@ -20,6 +20,18 @@
static bool msm_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
{
+ /* NOTE: we are *closer* to being able to get rid of
+ * mutex_trylock_recursive().. the msm_gem code itself does
+ * not need struct_mutex, although codepaths that can trigger
+ * shrinker are still called in code-paths that hold the
+ * struct_mutex.
+ *
+ * Also, msm_obj->madv is protected by struct_mutex.
+ *
+ * The next step is probably split out a seperate lock for
+ * protecting inactive_list, so that shrinker does not need
+ * struct_mutex.
+ */
switch (mutex_trylock_recursive(&dev->struct_mutex)) {
case MUTEX_TRYLOCK_FAILED:
return false;
--
2.9.4
|
