[bug report] drm/i915: Trim struct_mutex hold duration for i915_gem_free_objects

[bug report] drm/i915: Trim struct_mutex hold duration for i915_gem_free_objects

[Dan Carpenter]

Hello Chris Wilson,

The patch cc731f5a3b1f: "drm/i915: Trim struct_mutex hold duration
for i915_gem_free_objects" from Oct 13, 2017, leads to the following
static checker warning:

	drivers/gpu/drm/i915/gem/i915_gem_object.c:195 __i915_gem_free_objects()
	error: we previously assumed 'obj' could be null (see line 195)

drivers/gpu/drm/i915/gem/i915_gem_object.c
   188  static void __i915_gem_free_objects(struct drm_i915_private *i915,
   189                                      struct llist_node *freed)
   190  {
   191          struct drm_i915_gem_object *obj, *on;
   192          intel_wakeref_t wakeref;
   193  
   194          wakeref = intel_runtime_pm_get(i915);
   195          llist_for_each_entry_safe(obj, on, freed, freed) {
                                               ^^
&on->freed is a pointer to the next item in the list?

   196                  struct i915_vma *vma, *vn;
   197  
   198                  trace_i915_gem_object_destroy(obj);
   199  
   200                  mutex_lock(&i915->drm.struct_mutex);
   201  
   202                  GEM_BUG_ON(i915_gem_object_is_active(obj));
   203                  list_for_each_entry_safe(vma, vn, &obj->vma.list, obj_link) {
   204                          GEM_BUG_ON(i915_vma_is_active(vma));
   205                          vma->flags &= ~I915_VMA_PIN_MASK;
   206                          i915_vma_destroy(vma);
   207                  }
   208                  GEM_BUG_ON(!list_empty(&obj->vma.list));
   209                  GEM_BUG_ON(!RB_EMPTY_ROOT(&obj->vma.tree));
   210  
   211                  /* This serializes freeing with the shrinker. Since the free
   212                   * is delayed, first by RCU then by the workqueue, we want the
   213                   * shrinker to be able to free pages of unreferenced objects,
   214                   * or else we may oom whilst there are plenty of deferred
   215                   * freed objects.
   216                   */
   217                  if (i915_gem_object_has_pages(obj)) {
   218                          spin_lock(&i915->mm.obj_lock);
   219                          list_del_init(&obj->mm.link);
   220                          spin_unlock(&i915->mm.obj_lock);
   221                  }
   222  
   223                  mutex_unlock(&i915->drm.struct_mutex);
   224  
   225                  GEM_BUG_ON(obj->bind_count);
   226                  GEM_BUG_ON(obj->userfault_count);
   227                  GEM_BUG_ON(atomic_read(&obj->frontbuffer_bits));
   228                  GEM_BUG_ON(!list_empty(&obj->lut_list));
   229  
   230                  if (obj->ops->release)
   231                          obj->ops->release(obj);
   232  
   233                  if (WARN_ON(i915_gem_object_has_pinned_pages(obj)))
   234                          atomic_set(&obj->mm.pages_pin_count, 0);
   235                  __i915_gem_object_put_pages(obj, I915_MM_NORMAL);
   236                  GEM_BUG_ON(i915_gem_object_has_pages(obj));
   237  
   238                  if (obj->base.import_attach)
   239                          drm_prime_gem_destroy(&obj->base, NULL);
   240  
   241                  reservation_object_fini(&obj->__builtin_resv);
   242                  drm_gem_object_release(&obj->base);
   243                  i915_gem_info_remove_obj(i915, obj->base.size);
   244  
   245                  bitmap_free(obj->bit_17);
   246                  i915_gem_object_free(obj);
   247  
   248                  GEM_BUG_ON(!atomic_read(&i915->mm.free_count));
   249                  atomic_dec(&i915->mm.free_count);
   250  
   251                  if (on)
                            ^^
So hopefully "on" can't be NULL here or we are toasted.

   252                          cond_resched();
   253          }
   254          intel_runtime_pm_put(i915, wakeref);
   255  }

regards,
dan carpenter
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [bug report] drm/i915: Trim struct_mutex hold duration for i915_gem_free_objects

[Chris Wilson]

Quoting Dan Carpenter (2019-05-30 09:13:01)
> Hello Chris Wilson,
> 
> The patch cc731f5a3b1f: "drm/i915: Trim struct_mutex hold duration
> for i915_gem_free_objects" from Oct 13, 2017, leads to the following
> static checker warning:
> 
>         drivers/gpu/drm/i915/gem/i915_gem_object.c:195 __i915_gem_free_objects()
>         error: we previously assumed 'obj' could be null (see line 195)
> 
> drivers/gpu/drm/i915/gem/i915_gem_object.c
>    188  static void __i915_gem_free_objects(struct drm_i915_private *i915,
>    189                                      struct llist_node *freed)
>    190  {
>    191          struct drm_i915_gem_object *obj, *on;
>    192          intel_wakeref_t wakeref;
>    193  
>    194          wakeref = intel_runtime_pm_get(i915);
>    195          llist_for_each_entry_safe(obj, on, freed, freed) {
>                                                ^^
> &on->freed is a pointer to the next item in the list?

Hmm. I was looking at llist_for_each_safe where the next pointer would be
NULL. But for _entry, it will indeed be offset.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx