* drivers/gpu/drm/i915/gem/i915_gem_throttle.c:59 i915_gem_throttle_ioctl() error: double locked 'ctx->engines_mutex' (orig line 59)
@ 2020-11-16 10:08 Dan Carpenter
2020-11-16 10:15 ` Chris Wilson
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2020-11-16 10:08 UTC (permalink / raw)
To: kbuild, Chris Wilson
Cc: lkp, kbuild-all, linux-kernel, Joonas Lahtinen, Tvrtko Ursulin,
Rodrigo Vivi
[-- Attachment #1: Type: text/plain, Size: 4019 bytes --]
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@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
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
61 it) {
62 struct i915_request *rq, *target = NULL;
63
64 if (!ce->timeline)
65 continue;
66
67 mutex_lock(&ce->timeline->mutex);
68 list_for_each_entry_reverse(rq,
69 &ce->timeline->requests,
70 link) {
71 if (i915_request_completed(rq))
72 break;
73
74 if (time_after(rq->emitted_jiffies,
75 recent_enough))
76 continue;
77
78 target = i915_request_get(rq);
79 break;
80 }
81 mutex_unlock(&ce->timeline->mutex);
82 if (!target)
83 continue;
84
85 ret = i915_request_wait(target,
86 I915_WAIT_INTERRUPTIBLE,
87 MAX_SCHEDULE_TIMEOUT);
88 i915_request_put(target);
89 if (ret < 0)
90 break;
91 }
92 i915_gem_context_unlock_engines(ctx);
But only unlocks the last element
93 i915_gem_context_put(ctx);
94
95 rcu_read_lock();
96 }
97 rcu_read_unlock();
98
99 return ret < 0 ? ret : 0;
100 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45384 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: drivers/gpu/drm/i915/gem/i915_gem_throttle.c:59 i915_gem_throttle_ioctl() error: double locked 'ctx->engines_mutex' (orig line 59)
2020-11-16 10:08 drivers/gpu/drm/i915/gem/i915_gem_throttle.c:59 i915_gem_throttle_ioctl() error: double locked 'ctx->engines_mutex' (orig line 59) Dan Carpenter
@ 2020-11-16 10:15 ` Chris Wilson
2020-11-16 10:44 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Chris Wilson @ 2020-11-16 10:15 UTC (permalink / raw)
To: Dan Carpenter, kbuild
Cc: lkp, kbuild-all, linux-kernel, Joonas Lahtinen, Tvrtko Ursulin,
Rodrigo Vivi
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@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: drivers/gpu/drm/i915/gem/i915_gem_throttle.c:59 i915_gem_throttle_ioctl() error: double locked 'ctx->engines_mutex' (orig line 59)
2020-11-16 10:15 ` Chris Wilson
@ 2020-11-16 10:44 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2020-11-16 10:44 UTC (permalink / raw)
To: Chris Wilson
Cc: kbuild, lkp, kbuild-all, linux-kernel, Joonas Lahtinen,
Tvrtko Ursulin, Rodrigo Vivi
On Mon, Nov 16, 2020 at 10:15:04AM +0000, Chris Wilson wrote:
> 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@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > 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.
Yeah... But that's true enough. But what I think is actually causing
the static checker warning are the continues.
52 xa_for_each(&file_priv->context_xa, idx, ctx) {
53 struct i915_gem_engines_iter it;
54 struct intel_context *ce;
55
56 if (!kref_get_unless_zero(&ctx->ref))
57 continue;
58 rcu_read_unlock();
59
60 for_each_gem_engine(ce,
61 i915_gem_context_lock_engines(ctx),
62 it) {
63 struct i915_request *rq, *target = NULL;
64
65 if (!ce->timeline)
66 continue;
^^^^^^^^^
This continue is for the inside loop, so "ctx" isn't iterated. There
is another continue as well later in the loop. Potentially they could
be replaced with breaks?
67
68 mutex_lock(&ce->timeline->mutex);
69 list_for_each_entry_reverse(rq,
70 &ce->timeline->requests,
71 link) {
72 if (i915_request_completed(rq))
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-16 12:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-16 10:08 drivers/gpu/drm/i915/gem/i915_gem_throttle.c:59 i915_gem_throttle_ioctl() error: double locked 'ctx->engines_mutex' (orig line 59) Dan Carpenter
2020-11-16 10:15 ` Chris Wilson
2020-11-16 10:44 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®