* [PATCH] drm/i915: Lock llist_del_first() vs llist_del_all()
@ 2017-11-06 11:15 Chris Wilson
2017-11-06 11:46 ` Mika Kuoppala
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Chris Wilson @ 2017-11-06 11:15 UTC (permalink / raw)
To: intel-gfx
An oversight in commit 87701b4b5593 ("drm/i915: Only free the oldest
stale object before a fresh allocation") was that not only do we have to
serialise concurrent users of llist_del_first(), but we also have to
lock llist_del_first() vs llist_del_all().
From llist.h,
* This can be summarized as follows:
*
* | add | del_first | del_all
* add | - | - | -
* del_first | | L | L
* del_all | | | -
*
* Where, a particular row's operation can happen concurrently with a column's
* operation, with "-" being no lock needed, while "L" being lock is needed.
This should hopefully explain:
<4>[ 89.287106] general protection fault: 0000 [#1] PREEMPT SMP
<4>[ 89.287126] Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal intel_powerclamp coretemp i915 crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core r8169 mii mei_me mei snd_pcm prime_numbers i2c_hid pinctrl_geminilake pinctrl_intel
<4>[ 89.287226] CPU: 2 PID: 23 Comm: ksoftirqd/2 Tainted: G U 4.14.0-rc8-CI-CI_DRM_3315+ #1
<4>[ 89.287247] Hardware name: Intel Corp. Geminilake/GLK RVP2 LP4SD (07), BIOS GELKRVPA.X64.0062.B30.1708222146 08/22/2017
<4>[ 89.287270] task: ffff88017ab34ec0 task.stack: ffffc90000128000
<4>[ 89.287290] RIP: 0010:llist_add_batch+0x4/0x20
<4>[ 89.287301] RSP: 0018:ffffc9000012bdb8 EFLAGS: 00010296
<4>[ 89.287314] RAX: ffffffff811017ad RBX: 6e468801a1560000 RCX: ef3e53fceecdeb81
<4>[ 89.287330] RDX: 6e468801a1566130 RSI: ffff880103d73d98 RDI: ffff880103d73d98
<4>[ 89.287346] RBP: ffffc9000012bdb8 R08: ffff88017ab35780 R09: 0000000000000000
<4>[ 89.287361] R10: ffffc9000012bd68 R11: 00000000abb18c3d R12: ffffffffa01369e0
<4>[ 89.287377] R13: ffff88017fd1b8f8 R14: ffff88017ab34ec0 R15: 000000000000000a
<4>[ 89.287393] FS: 0000000000000000(0000) GS:ffff88017fd00000(0000) knlGS:0000000000000000
<4>[ 89.287411] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[ 89.287424] CR2: 00007ff0c0755018 CR3: 000000016df9b000 CR4: 00000000003406e0
<4>[ 89.287440] Call Trace:
<4>[ 89.287511] __i915_gem_free_object_rcu+0x20/0x40 [i915]
<4>[ 89.287527] rcu_process_callbacks+0x27a/0x730
<4>[ 89.287544] __do_softirq+0xc0/0x4ae
<4>[ 89.287559] ? smpboot_thread_fn+0x2d/0x280
<4>[ 89.287571] run_ksoftirqd+0x1f/0x70
<4>[ 89.287582] smpboot_thread_fn+0x18a/0x280
<4>[ 89.287595] kthread+0x114/0x150
<4>[ 89.287605] ? sort_range+0x30/0x30
<4>[ 89.287615] ? kthread_create_on_node+0x40/0x40
<4>[ 89.287628] ret_from_fork+0x27/0x40
<4>[ 89.287641] Code: 0d 48 83 ea 01 4c 89 c1 48 83 fa ff 74 12 48 23 0c d7 74 ed 48 c1 e2 06 48 0f bd c9 48 8d 04 0a 5d c3 90 90 90 90 90 55 48 89 e5 <48> 8b 0a 48 89 0e 48 89 c8 f0 48 0f b1 3a 48 39 c1 75 ed 48 85
<1>[ 89.287774] RIP: llist_add_batch+0x4/0x20 RSP: ffffc9000012bdb8
<4>[ 89.287826] ---[ end trace e775d15174d8ae02 ]---
(Lockless lists are only easy (and lockless) when using
llist_add/llist_del_all!)
Fixes: 87701b4b5593 ("drm/i915: Only free the oldest stale object before
a fresh allocation")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6a71805be389..889ae8810d5f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4636,11 +4636,17 @@ static void __i915_gem_free_work(struct work_struct *work)
* unbound now.
*/
+ spin_lock(&i915->mm.free_lock);
while ((freed = llist_del_all(&i915->mm.free_list))) {
+ spin_unlock(&i915->mm.free_lock);
+
__i915_gem_free_objects(i915, freed);
if (need_resched())
- break;
+ return;
+
+ spin_lock(&i915->mm.free_lock);
}
+ spin_unlock(&i915->mm.free_lock);
}
static void __i915_gem_free_object_rcu(struct rcu_head *head)
--
2.15.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Lock llist_del_first() vs llist_del_all()
2017-11-06 11:15 [PATCH] drm/i915: Lock llist_del_first() vs llist_del_all() Chris Wilson
@ 2017-11-06 11:46 ` Mika Kuoppala
2017-11-06 13:22 ` Chris Wilson
2017-11-06 12:03 ` ✓ Fi.CI.BAT: success for drm/i915: Lock llist_del_first() vs llist_del_all() (rev2) Patchwork
2017-11-06 13:16 ` ✓ Fi.CI.IGT: " Patchwork
2 siblings, 1 reply; 6+ messages in thread
From: Mika Kuoppala @ 2017-11-06 11:46 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> An oversight in commit 87701b4b5593 ("drm/i915: Only free the oldest
> stale object before a fresh allocation") was that not only do we have to
> serialise concurrent users of llist_del_first(), but we also have to
> lock llist_del_first() vs llist_del_all().
>
> From llist.h,
>
> * This can be summarized as follows:
> *
> * | add | del_first | del_all
> * add | - | - | -
> * del_first | | L | L
> * del_all | | | -
> *
> * Where, a particular row's operation can happen concurrently with a column's
> * operation, with "-" being no lock needed, while "L" being lock is needed.
>
> This should hopefully explain:
>
> <4>[ 89.287106] general protection fault: 0000 [#1] PREEMPT SMP
> <4>[ 89.287126] Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal intel_powerclamp coretemp i915 crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core r8169 mii mei_me mei snd_pcm prime_numbers i2c_hid pinctrl_geminilake pinctrl_intel
> <4>[ 89.287226] CPU: 2 PID: 23 Comm: ksoftirqd/2 Tainted: G U 4.14.0-rc8-CI-CI_DRM_3315+ #1
> <4>[ 89.287247] Hardware name: Intel Corp. Geminilake/GLK RVP2 LP4SD (07), BIOS GELKRVPA.X64.0062.B30.1708222146 08/22/2017
> <4>[ 89.287270] task: ffff88017ab34ec0 task.stack: ffffc90000128000
> <4>[ 89.287290] RIP: 0010:llist_add_batch+0x4/0x20
> <4>[ 89.287301] RSP: 0018:ffffc9000012bdb8 EFLAGS: 00010296
> <4>[ 89.287314] RAX: ffffffff811017ad RBX: 6e468801a1560000 RCX: ef3e53fceecdeb81
> <4>[ 89.287330] RDX: 6e468801a1566130 RSI: ffff880103d73d98 RDI: ffff880103d73d98
> <4>[ 89.287346] RBP: ffffc9000012bdb8 R08: ffff88017ab35780 R09: 0000000000000000
> <4>[ 89.287361] R10: ffffc9000012bd68 R11: 00000000abb18c3d R12: ffffffffa01369e0
> <4>[ 89.287377] R13: ffff88017fd1b8f8 R14: ffff88017ab34ec0 R15: 000000000000000a
> <4>[ 89.287393] FS: 0000000000000000(0000) GS:ffff88017fd00000(0000) knlGS:0000000000000000
> <4>[ 89.287411] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> <4>[ 89.287424] CR2: 00007ff0c0755018 CR3: 000000016df9b000 CR4: 00000000003406e0
> <4>[ 89.287440] Call Trace:
> <4>[ 89.287511] __i915_gem_free_object_rcu+0x20/0x40 [i915]
> <4>[ 89.287527] rcu_process_callbacks+0x27a/0x730
> <4>[ 89.287544] __do_softirq+0xc0/0x4ae
> <4>[ 89.287559] ? smpboot_thread_fn+0x2d/0x280
> <4>[ 89.287571] run_ksoftirqd+0x1f/0x70
> <4>[ 89.287582] smpboot_thread_fn+0x18a/0x280
> <4>[ 89.287595] kthread+0x114/0x150
> <4>[ 89.287605] ? sort_range+0x30/0x30
> <4>[ 89.287615] ? kthread_create_on_node+0x40/0x40
> <4>[ 89.287628] ret_from_fork+0x27/0x40
> <4>[ 89.287641] Code: 0d 48 83 ea 01 4c 89 c1 48 83 fa ff 74 12 48 23 0c d7 74 ed 48 c1 e2 06 48 0f bd c9 48 8d 04 0a 5d c3 90 90 90 90 90 55 48 89 e5 <48> 8b 0a 48 89 0e 48 89 c8 f0 48 0f b1 3a 48 39 c1 75 ed 48 85
> <1>[ 89.287774] RIP: llist_add_batch+0x4/0x20 RSP: ffffc9000012bdb8
> <4>[ 89.287826] ---[ end trace e775d15174d8ae02 ]---
>
> (Lockless lists are only easy (and lockless) when using
> llist_add/llist_del_all!)
>
> Fixes: 87701b4b5593 ("drm/i915: Only free the oldest stale object before
> a fresh allocation")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Similar usage pattern in contexts but the paths are
under mutex.
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6a71805be389..889ae8810d5f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4636,11 +4636,17 @@ static void __i915_gem_free_work(struct work_struct *work)
> * unbound now.
> */
>
> + spin_lock(&i915->mm.free_lock);
> while ((freed = llist_del_all(&i915->mm.free_list))) {
> + spin_unlock(&i915->mm.free_lock);
> +
> __i915_gem_free_objects(i915, freed);
> if (need_resched())
> - break;
> + return;
> +
> + spin_lock(&i915->mm.free_lock);
> }
> + spin_unlock(&i915->mm.free_lock);
> }
>
> static void __i915_gem_free_object_rcu(struct rcu_head *head)
> --
> 2.15.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Lock llist_del_first() vs llist_del_all() (rev2)
2017-11-06 11:15 [PATCH] drm/i915: Lock llist_del_first() vs llist_del_all() Chris Wilson
2017-11-06 11:46 ` Mika Kuoppala
@ 2017-11-06 12:03 ` Patchwork
2017-11-06 13:16 ` ✓ Fi.CI.IGT: " Patchwork
2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2017-11-06 12:03 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Lock llist_del_first() vs llist_del_all() (rev2)
URL : https://patchwork.freedesktop.org/series/33241/
State : success
== Summary ==
Series 33241v2 drm/i915: Lock llist_del_first() vs llist_del_all()
https://patchwork.freedesktop.org/api/1.0/series/33241/revisions/2/mbox/
fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:454s
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:455s
fi-blb-e6850 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:380s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:557s
fi-bwr-2160 total:289 pass:183 dwarn:0 dfail:0 fail:0 skip:106 time:278s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:508s
fi-byt-j1900 total:289 pass:253 dwarn:1 dfail:0 fail:0 skip:35 time:507s
fi-byt-n2820 total:289 pass:249 dwarn:1 dfail:0 fail:0 skip:39 time:488s
fi-cfl-s total:289 pass:254 dwarn:3 dfail:0 fail:0 skip:32 time:544s
fi-cnl-y total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:616s
fi-elk-e7500 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:424s
fi-gdg-551 total:289 pass:178 dwarn:1 dfail:0 fail:1 skip:109 time:271s
fi-glk-1 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:586s
fi-glk-dsi total:289 pass:258 dwarn:0 dfail:0 fail:1 skip:30 time:493s
fi-hsw-4770 total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:436s
fi-hsw-4770r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:431s
fi-ilk-650 total:289 pass:228 dwarn:0 dfail:0 fail:0 skip:61 time:435s
fi-ivb-3520m total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:500s
fi-ivb-3770 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:463s
fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:575s
fi-kbl-7567u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:480s
fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:585s
fi-pnv-d510 total:289 pass:222 dwarn:1 dfail:0 fail:0 skip:66 time:576s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:454s
fi-skl-6600u total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:603s
fi-skl-6700hq total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:651s
fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:521s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:511s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:465s
fi-snb-2520m total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:569s
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:0 skip:40 time:431s
9cc9686b44e192d2561a513f84d5ac95518cad73 drm-tip: 2017y-11m-06d-10h-10m-17s UTC integration manifest
7d117513f2a4 drm/i915: Lock llist_del_first() vs llist_del_all()
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6970/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Lock llist_del_first() vs llist_del_all() (rev2)
2017-11-06 11:15 [PATCH] drm/i915: Lock llist_del_first() vs llist_del_all() Chris Wilson
2017-11-06 11:46 ` Mika Kuoppala
2017-11-06 12:03 ` ✓ Fi.CI.BAT: success for drm/i915: Lock llist_del_first() vs llist_del_all() (rev2) Patchwork
@ 2017-11-06 13:16 ` Patchwork
2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2017-11-06 13:16 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Lock llist_del_first() vs llist_del_all() (rev2)
URL : https://patchwork.freedesktop.org/series/33241/
State : success
== Summary ==
Test drv_module_reload:
Subgroup basic-reload-inject:
pass -> DMESG-WARN (shard-hsw) fdo#102707
Test kms_setmode:
Subgroup basic:
pass -> FAIL (shard-hsw) fdo#99912
Test kms_flip:
Subgroup wf_vblank-vs-dpms-interruptible:
incomplete -> PASS (shard-hsw) fdo#102614
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
shard-hsw total:2539 pass:1432 dwarn:1 dfail:0 fail:9 skip:1097 time:9313s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6970/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Lock llist_del_first() vs llist_del_all()
2017-11-06 11:46 ` Mika Kuoppala
@ 2017-11-06 13:22 ` Chris Wilson
0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2017-11-06 13:22 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx
Quoting Mika Kuoppala (2017-11-06 11:46:16)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > An oversight in commit 87701b4b5593 ("drm/i915: Only free the oldest
> > stale object before a fresh allocation") was that not only do we have to
> > serialise concurrent users of llist_del_first(), but we also have to
> > lock llist_del_first() vs llist_del_all().
> >
> > From llist.h,
> >
> > * This can be summarized as follows:
> > *
> > * | add | del_first | del_all
> > * add | - | - | -
> > * del_first | | L | L
> > * del_all | | | -
> > *
> > * Where, a particular row's operation can happen concurrently with a column's
> > * operation, with "-" being no lock needed, while "L" being lock is needed.
> >
> > This should hopefully explain:
> >
> > <4>[ 89.287106] general protection fault: 0000 [#1] PREEMPT SMP
> > <4>[ 89.287126] Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal intel_powerclamp coretemp i915 crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core r8169 mii mei_me mei snd_pcm prime_numbers i2c_hid pinctrl_geminilake pinctrl_intel
> > <4>[ 89.287226] CPU: 2 PID: 23 Comm: ksoftirqd/2 Tainted: G U 4.14.0-rc8-CI-CI_DRM_3315+ #1
> > <4>[ 89.287247] Hardware name: Intel Corp. Geminilake/GLK RVP2 LP4SD (07), BIOS GELKRVPA.X64.0062.B30.1708222146 08/22/2017
> > <4>[ 89.287270] task: ffff88017ab34ec0 task.stack: ffffc90000128000
> > <4>[ 89.287290] RIP: 0010:llist_add_batch+0x4/0x20
> > <4>[ 89.287301] RSP: 0018:ffffc9000012bdb8 EFLAGS: 00010296
> > <4>[ 89.287314] RAX: ffffffff811017ad RBX: 6e468801a1560000 RCX: ef3e53fceecdeb81
> > <4>[ 89.287330] RDX: 6e468801a1566130 RSI: ffff880103d73d98 RDI: ffff880103d73d98
> > <4>[ 89.287346] RBP: ffffc9000012bdb8 R08: ffff88017ab35780 R09: 0000000000000000
> > <4>[ 89.287361] R10: ffffc9000012bd68 R11: 00000000abb18c3d R12: ffffffffa01369e0
> > <4>[ 89.287377] R13: ffff88017fd1b8f8 R14: ffff88017ab34ec0 R15: 000000000000000a
> > <4>[ 89.287393] FS: 0000000000000000(0000) GS:ffff88017fd00000(0000) knlGS:0000000000000000
> > <4>[ 89.287411] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > <4>[ 89.287424] CR2: 00007ff0c0755018 CR3: 000000016df9b000 CR4: 00000000003406e0
> > <4>[ 89.287440] Call Trace:
> > <4>[ 89.287511] __i915_gem_free_object_rcu+0x20/0x40 [i915]
> > <4>[ 89.287527] rcu_process_callbacks+0x27a/0x730
> > <4>[ 89.287544] __do_softirq+0xc0/0x4ae
> > <4>[ 89.287559] ? smpboot_thread_fn+0x2d/0x280
> > <4>[ 89.287571] run_ksoftirqd+0x1f/0x70
> > <4>[ 89.287582] smpboot_thread_fn+0x18a/0x280
> > <4>[ 89.287595] kthread+0x114/0x150
> > <4>[ 89.287605] ? sort_range+0x30/0x30
> > <4>[ 89.287615] ? kthread_create_on_node+0x40/0x40
> > <4>[ 89.287628] ret_from_fork+0x27/0x40
> > <4>[ 89.287641] Code: 0d 48 83 ea 01 4c 89 c1 48 83 fa ff 74 12 48 23 0c d7 74 ed 48 c1 e2 06 48 0f bd c9 48 8d 04 0a 5d c3 90 90 90 90 90 55 48 89 e5 <48> 8b 0a 48 89 0e 48 89 c8 f0 48 0f b1 3a 48 39 c1 75 ed 48 85
> > <1>[ 89.287774] RIP: llist_add_batch+0x4/0x20 RSP: ffffc9000012bdb8
> > <4>[ 89.287826] ---[ end trace e775d15174d8ae02 ]---
> >
> > (Lockless lists are only easy (and lockless) when using
> > llist_add/llist_del_all!)
> >
> > Fixes: 87701b4b5593 ("drm/i915: Only free the oldest stale object before
> > a fresh allocation")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> Similar usage pattern in contexts but the paths are
> under mutex.
>
> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Thanks for the review, pushed. Hopefully that catches it before it crops
up too many times.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] drm/i915: Lock llist_del_first() vs llist_del_all()
@ 2017-11-06 11:11 Chris Wilson
0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2017-11-06 11:11 UTC (permalink / raw)
To: intel-gfx
An oversight in commit 87701b4b5593 ("drm/i915: Only free the oldest
stale object before a fresh allocation") was that not only do we have to
serialise concurrent users of llist_del_first(), but we also have to
lock llist_del_first() vs llist_del_all().
From llist.h,
* This can be summarized as follows:
*
* | add | del_first | del_all
* add | - | - | -
* del_first | | L | L
* del_all | | | -
*
* Where, a particular row's operation can happen concurrently with a column's
* operation, with "-" being no lock needed, while "L" being lock is needed.
This should hopefully explain:
<4>[ 89.287106] general protection fault: 0000 [#1] PREEMPT SMP
<4>[ 89.287126] Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal intel_powerclamp coretemp i915 crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core r8169 mii mei_me mei snd_pcm prime_numbers i2c_hid pinctrl_geminilake pinctrl_intel
<4>[ 89.287226] CPU: 2 PID: 23 Comm: ksoftirqd/2 Tainted: G U 4.14.0-rc8-CI-CI_DRM_3315+ #1
<4>[ 89.287247] Hardware name: Intel Corp. Geminilake/GLK RVP2 LP4SD (07), BIOS GELKRVPA.X64.0062.B30.1708222146 08/22/2017
<4>[ 89.287270] task: ffff88017ab34ec0 task.stack: ffffc90000128000
<4>[ 89.287290] RIP: 0010:llist_add_batch+0x4/0x20
<4>[ 89.287301] RSP: 0018:ffffc9000012bdb8 EFLAGS: 00010296
<4>[ 89.287314] RAX: ffffffff811017ad RBX: 6e468801a1560000 RCX: ef3e53fceecdeb81
<4>[ 89.287330] RDX: 6e468801a1566130 RSI: ffff880103d73d98 RDI: ffff880103d73d98
<4>[ 89.287346] RBP: ffffc9000012bdb8 R08: ffff88017ab35780 R09: 0000000000000000
<4>[ 89.287361] R10: ffffc9000012bd68 R11: 00000000abb18c3d R12: ffffffffa01369e0
<4>[ 89.287377] R13: ffff88017fd1b8f8 R14: ffff88017ab34ec0 R15: 000000000000000a
<4>[ 89.287393] FS: 0000000000000000(0000) GS:ffff88017fd00000(0000) knlGS:0000000000000000
<4>[ 89.287411] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[ 89.287424] CR2: 00007ff0c0755018 CR3: 000000016df9b000 CR4: 00000000003406e0
<4>[ 89.287440] Call Trace:
<4>[ 89.287511] __i915_gem_free_object_rcu+0x20/0x40 [i915]
<4>[ 89.287527] rcu_process_callbacks+0x27a/0x730
<4>[ 89.287544] __do_softirq+0xc0/0x4ae
<4>[ 89.287559] ? smpboot_thread_fn+0x2d/0x280
<4>[ 89.287571] run_ksoftirqd+0x1f/0x70
<4>[ 89.287582] smpboot_thread_fn+0x18a/0x280
<4>[ 89.287595] kthread+0x114/0x150
<4>[ 89.287605] ? sort_range+0x30/0x30
<4>[ 89.287615] ? kthread_create_on_node+0x40/0x40
<4>[ 89.287628] ret_from_fork+0x27/0x40
<4>[ 89.287641] Code: 0d 48 83 ea 01 4c 89 c1 48 83 fa ff 74 12 48 23 0c d7 74 ed 48 c1 e2 06 48 0f bd c9 48 8d 04 0a 5d c3 90 90 90 90 90 55 48 89 e5 <48> 8b 0a 48 89 0e 48 89 c8 f0 48 0f b1 3a 48 39 c1 75 ed 48 85
<1>[ 89.287774] RIP: llist_add_batch+0x4/0x20 RSP: ffffc9000012bdb8
<4>[ 89.287826] ---[ end trace e775d15174d8ae02 ]---
(Lockless lists are only easy (and lockless) when using
llist_add/llist_del_all!)
Fixes: 87701b4b5593 ("drm/i915: Only free the oldest stale object before
a fresh allocation")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6a71805be389..889ae8810d5f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4636,11 +4636,17 @@ static void __i915_gem_free_work(struct work_struct *work)
* unbound now.
*/
+ spin_lock(&i915->mm.free_lock);
while ((freed = llist_del_all(&i915->mm.free_list))) {
+ spin_unlock(&i915->mm.free_lock);
+
__i915_gem_free_objects(i915, freed);
if (need_resched())
- break;
+ return;
+
+ spin_lock(&i915->mm.free_lock);
}
+ spin_unlock(&i915->mm.free_lock);
}
static void __i915_gem_free_object_rcu(struct rcu_head *head)
--
2.15.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-06 13:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-06 11:15 [PATCH] drm/i915: Lock llist_del_first() vs llist_del_all() Chris Wilson
2017-11-06 11:46 ` Mika Kuoppala
2017-11-06 13:22 ` Chris Wilson
2017-11-06 12:03 ` ✓ Fi.CI.BAT: success for drm/i915: Lock llist_del_first() vs llist_del_all() (rev2) Patchwork
2017-11-06 13:16 ` ✓ Fi.CI.IGT: " Patchwork
-- strict thread matches above, loose matches on Subject: below --
2017-11-06 11:11 [PATCH] drm/i915: Lock llist_del_first() vs llist_del_all() Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.