| Message ID | 1454411190-15721-11-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 02, 2016 at 11:06:28AM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Purpose is to catch places which iterate the object VMA list > without holding the big lock. > > Implemented by open coding list_for_each_entry to make the > macro compatible with existing call sites. > > v2: Error capture runs without the mutex so iterate directly from there. > v3: Replace WARN_ON with lockdep_assert_held. (Chris Wilson, Daniel Vetter) > v4: Moved under dedicated CONFIG_DRM_I915_DEBUG and back to WARN_ON. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 8 ++++---- > drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++ > drivers/gpu/drm/i915/i915_gem.c | 24 ++++++++++++------------ > drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- > drivers/gpu/drm/i915/i915_gem_shrinker.c | 2 +- > 5 files changed, 29 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 863012a2602e..ff444f09ea98 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj) > u64 size = 0; > struct i915_vma *vma; > > - list_for_each_entry(vma, &obj->vma_list, vma_link) { > + i915_gem_obj_for_each_vma(vma, obj) { > if (i915_is_ggtt(vma->vm) && > drm_mm_node_allocated(&vma->node)) > size += vma->node.size; > @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) > obj->madv == I915_MADV_DONTNEED ? " purgeable" : ""); > if (obj->base.name) > seq_printf(m, " (name: %d)", obj->base.name); > - list_for_each_entry(vma, &obj->vma_list, vma_link) { > + i915_gem_obj_for_each_vma(vma, obj) { > if (vma->pin_count > 0) > pin_count++; > } > @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) > seq_printf(m, " (display)"); > if (obj->fence_reg != I915_FENCE_REG_NONE) > seq_printf(m, " (fence: %d)", obj->fence_reg); > - list_for_each_entry(vma, &obj->vma_list, vma_link) { > + i915_gem_obj_for_each_vma(vma, obj) { > seq_printf(m, " (%sgtt offset: %08llx, size: %08llx", > i915_is_ggtt(vma->vm) ? "g" : "pp", > vma->node.start, vma->node.size); > @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data) > stats->shared += obj->base.size; > > if (USES_FULL_PPGTT(obj->base.dev)) { > - list_for_each_entry(vma, &obj->vma_list, vma_link) { > + i915_gem_obj_for_each_vma(vma, obj) { > struct i915_hw_ppgtt *ppgtt; > > if (!drm_mm_node_allocated(&vma->node)) > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 905e90f25957..05ef750386df 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2861,6 +2861,17 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data( > void i915_gem_free_object(struct drm_gem_object *obj); > void i915_gem_vma_destroy(struct i915_vma *vma); > > +#ifdef CONFIG_DRM_I915_DEBUG > + #define i915_gem_obj_for_each_vma(vma, obj) \ This is the wrong name, i915_gem_object is the object on which we are iterating over, so i915_gem_object_. > + for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \ I disagree that we should be doing i915-specific lockdep debugging and *ignoring* the core facilities. If we locked at RCU, they introduced a new set of _protected/_check interfaces, an idea that could also be used here to add this or something like this to the core. Something like #define list_for_each_entry_check(pos, list, member, lock) \ for (typeof(*lock) == typeof(struct mutex) ? assert_lockdep_held(lock) : assert_spin_locked(lock); \ pos = list_first_entry(head, typeof(*pos), member); \ &pos->member != (head); \ pos = list_next_entry(pos, member)) #define i915_gem_object_for_each_vma(vma, obj) \ list_for_each_entry_check(vma, &(obj)->vma_list, vma_link, &(obj)->base.dev->struct_mutex) could be lifted easily, and makes i915_gem_object_for_each_vma() easier to understand (i.e. serves better as documentation). > + vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\ > + &vma->vma_link != (&(obj)->vma_list); \ > + vma = list_next_entry(vma, vma_link)) > +#else > + #define i915_gem_obj_for_each_vma(vma, obj) \ > + list_for_each_entry((vma), &(obj)->vma_list, vma_link) > +#endif > + > /* Flags used by pin/bind&friends. */ > #define PIN_MAPPABLE (1<<0) > #define PIN_NONBLOCK (1<<1) > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index c558887b2084..ce9d0544b42c 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2454,7 +2454,7 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring) > list_move_tail(&obj->global_list, > &to_i915(obj->base.dev)->mm.bound_list); > > - list_for_each_entry(vma, &obj->vma_list, vma_link) { > + i915_gem_obj_for_each_vma(vma, obj) { This and the majority of the conversions simply should not exist and only do so because of what I consider to be bad API. After they are removed, there are only a few list iterators left. That said, there is value in documenting the current locking. -Chris
On 02/02/16 11:36, Chris Wilson wrote: > On Tue, Feb 02, 2016 at 11:06:28AM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Purpose is to catch places which iterate the object VMA list >> without holding the big lock. >> >> Implemented by open coding list_for_each_entry to make the >> macro compatible with existing call sites. >> >> v2: Error capture runs without the mutex so iterate directly from there. >> v3: Replace WARN_ON with lockdep_assert_held. (Chris Wilson, Daniel Vetter) >> v4: Moved under dedicated CONFIG_DRM_I915_DEBUG and back to WARN_ON. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 8 ++++---- >> drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++ >> drivers/gpu/drm/i915/i915_gem.c | 24 ++++++++++++------------ >> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- >> drivers/gpu/drm/i915/i915_gem_shrinker.c | 2 +- >> 5 files changed, 29 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index 863012a2602e..ff444f09ea98 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj) >> u64 size = 0; >> struct i915_vma *vma; >> >> - list_for_each_entry(vma, &obj->vma_list, vma_link) { >> + i915_gem_obj_for_each_vma(vma, obj) { >> if (i915_is_ggtt(vma->vm) && >> drm_mm_node_allocated(&vma->node)) >> size += vma->node.size; >> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) >> obj->madv == I915_MADV_DONTNEED ? " purgeable" : ""); >> if (obj->base.name) >> seq_printf(m, " (name: %d)", obj->base.name); >> - list_for_each_entry(vma, &obj->vma_list, vma_link) { >> + i915_gem_obj_for_each_vma(vma, obj) { >> if (vma->pin_count > 0) >> pin_count++; >> } >> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) >> seq_printf(m, " (display)"); >> if (obj->fence_reg != I915_FENCE_REG_NONE) >> seq_printf(m, " (fence: %d)", obj->fence_reg); >> - list_for_each_entry(vma, &obj->vma_list, vma_link) { >> + i915_gem_obj_for_each_vma(vma, obj) { >> seq_printf(m, " (%sgtt offset: %08llx, size: %08llx", >> i915_is_ggtt(vma->vm) ? "g" : "pp", >> vma->node.start, vma->node.size); >> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data) >> stats->shared += obj->base.size; >> >> if (USES_FULL_PPGTT(obj->base.dev)) { >> - list_for_each_entry(vma, &obj->vma_list, vma_link) { >> + i915_gem_obj_for_each_vma(vma, obj) { >> struct i915_hw_ppgtt *ppgtt; >> >> if (!drm_mm_node_allocated(&vma->node)) >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 905e90f25957..05ef750386df 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2861,6 +2861,17 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data( >> void i915_gem_free_object(struct drm_gem_object *obj); >> void i915_gem_vma_destroy(struct i915_vma *vma); >> >> +#ifdef CONFIG_DRM_I915_DEBUG >> + #define i915_gem_obj_for_each_vma(vma, obj) \ > > This is the wrong name, i915_gem_object is the object on which we are > iterating over, so i915_gem_object_. To me obj is just a shorthand for object, sorry. :) It sounds like bad naming if obj and object mean something functionally different. Imagine if request, req and rq meant something different. :D >> + for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \ > > I disagree that we should be doing i915-specific lockdep debugging and > *ignoring* the core facilities. If we locked at RCU, they introduced a > new set of _protected/_check interfaces, an idea that could also be used > here to add this or something like this to the core. > Something like > > #define list_for_each_entry_check(pos, list, member, lock) \ > for (typeof(*lock) == typeof(struct mutex) ? assert_lockdep_held(lock) : assert_spin_locked(lock); \ > pos = list_first_entry(head, typeof(*pos), member); \ > &pos->member != (head); \ > pos = list_next_entry(pos, member)) > > #define i915_gem_object_for_each_vma(vma, obj) \ > list_for_each_entry_check(vma, &(obj)->vma_list, vma_link, &(obj)->base.dev->struct_mutex) > > could be lifted easily, and makes i915_gem_object_for_each_vma() easier > to understand (i.e. serves better as documentation). Don't know, needs buy-in from the relevant people, and depends on how useful to outside of i915 it would be. And if you can make lockdep_assert_held work in this context. But I am also not sure all i915 debugging should be tied to lockdep debugging so to me it is not so clear-cut. >> + vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\ >> + &vma->vma_link != (&(obj)->vma_list); \ >> + vma = list_next_entry(vma, vma_link)) >> +#else >> + #define i915_gem_obj_for_each_vma(vma, obj) \ >> + list_for_each_entry((vma), &(obj)->vma_list, vma_link) >> +#endif >> + >> /* Flags used by pin/bind&friends. */ >> #define PIN_MAPPABLE (1<<0) >> #define PIN_NONBLOCK (1<<1) >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index c558887b2084..ce9d0544b42c 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -2454,7 +2454,7 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring) >> list_move_tail(&obj->global_list, >> &to_i915(obj->base.dev)->mm.bound_list); >> >> - list_for_each_entry(vma, &obj->vma_list, vma_link) { >> + i915_gem_obj_for_each_vma(vma, obj) { > > This and the majority of the conversions simply should not exist and > only do so because of what I consider to be bad API. After they are Bad API or what you really meant was GEM internals should not do it? What is the harm anyway? More use, if it is painless, means less likelyhood for copy&paste errors in the future. > removed, there are only a few list iterators left. That said, there is > value in documenting the current locking. The last bit meaning exactly? Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 863012a2602e..ff444f09ea98 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj) u64 size = 0; struct i915_vma *vma; - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (i915_is_ggtt(vma->vm) && drm_mm_node_allocated(&vma->node)) size += vma->node.size; @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) obj->madv == I915_MADV_DONTNEED ? " purgeable" : ""); if (obj->base.name) seq_printf(m, " (name: %d)", obj->base.name); - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (vma->pin_count > 0) pin_count++; } @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) seq_printf(m, " (display)"); if (obj->fence_reg != I915_FENCE_REG_NONE) seq_printf(m, " (fence: %d)", obj->fence_reg); - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { seq_printf(m, " (%sgtt offset: %08llx, size: %08llx", i915_is_ggtt(vma->vm) ? "g" : "pp", vma->node.start, vma->node.size); @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data) stats->shared += obj->base.size; if (USES_FULL_PPGTT(obj->base.dev)) { - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { struct i915_hw_ppgtt *ppgtt; if (!drm_mm_node_allocated(&vma->node)) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 905e90f25957..05ef750386df 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2861,6 +2861,17 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data( void i915_gem_free_object(struct drm_gem_object *obj); void i915_gem_vma_destroy(struct i915_vma *vma); +#ifdef CONFIG_DRM_I915_DEBUG + #define i915_gem_obj_for_each_vma(vma, obj) \ + for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \ + vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\ + &vma->vma_link != (&(obj)->vma_list); \ + vma = list_next_entry(vma, vma_link)) +#else + #define i915_gem_obj_for_each_vma(vma, obj) \ + list_for_each_entry((vma), &(obj)->vma_list, vma_link) +#endif + /* Flags used by pin/bind&friends. */ #define PIN_MAPPABLE (1<<0) #define PIN_NONBLOCK (1<<1) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c558887b2084..ce9d0544b42c 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2454,7 +2454,7 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring) list_move_tail(&obj->global_list, &to_i915(obj->base.dev)->mm.bound_list); - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (!list_empty(&vma->mm_list)) list_move_tail(&vma->mm_list, &vma->vm->inactive_list); } @@ -3873,7 +3873,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, */ } - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (!drm_mm_node_allocated(&vma->node)) continue; @@ -3883,7 +3883,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, } } - list_for_each_entry(vma, &obj->vma_list, vma_link) + i915_gem_obj_for_each_vma(vma, obj) vma->node.color = cache_level; obj->cache_level = cache_level; @@ -4613,7 +4613,7 @@ struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj, struct i915_address_space *vm) { struct i915_vma *vma; - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL && vma->vm == vm) return vma; @@ -4630,7 +4630,7 @@ struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj, if (WARN_ONCE(!view, "no view specified")) return ERR_PTR(-EINVAL); - list_for_each_entry(vma, &obj->vma_list, vma_link) + i915_gem_obj_for_each_vma(vma, obj) if (vma->vm == ggtt && i915_ggtt_view_equal(&vma->ggtt_view, view)) return vma; @@ -5201,7 +5201,7 @@ u64 i915_gem_obj_offset(struct drm_i915_gem_object *o, WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base); - list_for_each_entry(vma, &o->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, o) { if (i915_is_ggtt(vma->vm) && vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) continue; @@ -5220,7 +5220,7 @@ u64 i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o, struct i915_address_space *ggtt = i915_obj_to_ggtt(o); struct i915_vma *vma; - list_for_each_entry(vma, &o->vma_list, vma_link) + i915_gem_obj_for_each_vma(vma, o) if (vma->vm == ggtt && i915_ggtt_view_equal(&vma->ggtt_view, view)) return vma->node.start; @@ -5234,7 +5234,7 @@ bool i915_gem_obj_bound(struct drm_i915_gem_object *o, { struct i915_vma *vma; - list_for_each_entry(vma, &o->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, o) { if (i915_is_ggtt(vma->vm) && vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) continue; @@ -5251,7 +5251,7 @@ bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o, struct i915_address_space *ggtt = i915_obj_to_ggtt(o); struct i915_vma *vma; - list_for_each_entry(vma, &o->vma_list, vma_link) + i915_gem_obj_for_each_vma(vma, o) if (vma->vm == ggtt && i915_ggtt_view_equal(&vma->ggtt_view, view) && drm_mm_node_allocated(&vma->node)) @@ -5264,7 +5264,7 @@ bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o) { struct i915_vma *vma; - list_for_each_entry(vma, &o->vma_list, vma_link) + i915_gem_obj_for_each_vma(vma, o) if (drm_mm_node_allocated(&vma->node)) return true; @@ -5281,7 +5281,7 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, BUG_ON(list_empty(&o->vma_list)); - list_for_each_entry(vma, &o->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, o) { if (i915_is_ggtt(vma->vm) && vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) continue; @@ -5294,7 +5294,7 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) { struct i915_vma *vma; - list_for_each_entry(vma, &obj->vma_list, vma_link) + i915_gem_obj_for_each_vma(vma, obj) if (vma->pin_count > 0) return true; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 715a771f0b31..ab00b2a3c035 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3237,7 +3237,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) vm = &dev_priv->gtt.base; list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { flush = false; - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (vma->vm != vm) continue; diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c index 58c1e592bbdb..4c89bd45dbde 100644 --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c @@ -52,7 +52,7 @@ static int num_vma_bound(struct drm_i915_gem_object *obj) struct i915_vma *vma; int count = 0; - list_for_each_entry(vma, &obj->vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (drm_mm_node_allocated(&vma->node)) count++; if (vma->pin_count)