<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div style="margin: 0px; line-height: normal; font-family: 'Helvetica Neue'; color: rgb(69, 69, 69);" class="">Hi Rob,</div><div style="margin: 0px; line-height: normal; font-family: 'Helvetica Neue'; color: rgb(69, 69, 69); min-height: 14px;" class=""> </div><div style="margin: 0px; line-height: normal; font-family: 'Helvetica Neue'; color: rgb(69, 69, 69);" class="">I can see how we can trigger the shrinker on objB while holding objA->lock. So, the nested lock with class SHRINKER makes sense.</div><div style="margin: 0px; line-height: normal; font-family: 'Helvetica Neue'; color: rgb(69, 69, 69);" class="">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?</div><div style="margin: 0px; line-height: normal; font-family: 'Helvetica Neue'; color: rgb(69, 69, 69); min-height: 14px;" class=""> </div><div style="margin: 0px; line-height: normal; font-family: 'Helvetica Neue'; color: rgb(69, 69, 69);" class="">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.</div><div style="margin: 0px; line-height: normal; font-family: 'Helvetica Neue'; color: rgb(69, 69, 69); min-height: 14px;" class=""> </div><div style="margin: 0px; line-height: normal; font-family: 'Helvetica Neue'; color: rgb(69, 69, 69);" class="">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.</div><div style="margin: 0px; line-height: normal; font-family: 'Helvetica Neue'; color: rgb(69, 69, 69); min-height: 14px;" class=""> </div><div style="margin: 0px; line-height: normal; font-family: 'Helvetica Neue'; color: rgb(69, 69, 69);" class="">Thanks,</div><div style="margin: 0px; line-height: normal; font-family: 'Helvetica Neue'; color: rgb(69, 69, 69);" class="">Sushmita</div><div style="margin: 0px; line-height: normal; font-family: 'Helvetica Neue'; color: rgb(69, 69, 69);" class=""> </div><div style=""><blockquote type="cite" class=""><div class="">On Jun 15, 2017, at 7:20 AM, Rob Clark <<a href="mailto:robdclark@gmail.com" class="">robdclark@gmail.com</a>> wrote:</div> <div class=""><div class="">--- 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 </div></div></blockquote></div> </body></html>