Re: drivers/gpu/drm/i915/gem/i915_gem_throttle.c:59 i915_gem_throttle_ioctl() error: double locked 'ctx->engines_mutex' (orig line 59)

From: Chris Wilson
Date: Mon Nov 16 2020 - 05:15:26 EST


Quoting Dan Carpenter (2020-11-16 10:08:38)
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 0062442ecfef0d82cd69e3e600d5006357f8d8e4
> commit: 27a5dcfe73f4b696b3de8c23a560199bb1c193a4 drm/i915/gem: Remove disordered per-file request list for throttling
> config: i386-randconfig-m021-20201115 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>
> smatch warnings:
> drivers/gpu/drm/i915/gem/i915_gem_throttle.c:59 i915_gem_throttle_ioctl() error: double locked 'ctx->engines_mutex' (orig line 59)
>
> vim +59 drivers/gpu/drm/i915/gem/i915_gem_throttle.c
>
> 35 int
> 36 i915_gem_throttle_ioctl(struct drm_device *dev, void *data,
> 37 struct drm_file *file)
> 38 {
> 39 const unsigned long recent_enough = jiffies - DRM_I915_THROTTLE_JIFFIES;
> 40 struct drm_i915_file_private *file_priv = file->driver_priv;
> 41 struct i915_gem_context *ctx;
> 42 unsigned long idx;
> 43 long ret;
> 44
> 45 /* ABI: return -EIO if already wedged */
> 46 ret = intel_gt_terminally_wedged(&to_i915(dev)->gt);
> 47 if (ret)
> 48 return ret;
> 49
> 50 rcu_read_lock();
> 51 xa_for_each(&file_priv->context_xa, idx, ctx) {
> 52 struct i915_gem_engines_iter it;
> 53 struct intel_context *ce;
> 54
> 55 if (!kref_get_unless_zero(&ctx->ref))
> 56 continue;
> 57 rcu_read_unlock();
> 58
> 59 for_each_gem_engine(ce,
> 60 i915_gem_context_lock_engines(ctx),
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> I don't understand why this takes the lock every iteration through the
> loop

It doesn't.

static inline struct i915_gem_engines *
i915_gem_context_lock_engines(struct i915_gem_context *ctx)
__acquires(&ctx->engines_mutex)
{
mutex_lock(&ctx->engines_mutex);
return i915_gem_context_engines(ctx);
}

static inline void
i915_gem_context_unlock_engines(struct i915_gem_context *ctx)
__releases(&ctx->engines_mutex)
{
mutex_unlock(&ctx->engines_mutex);
}

with the i915_gem_engines stored as a local in the iterator at the start
of the for loop.
-Chris