[Intel-gfx] [PATCH 1/6] drm/i915: Use the full hammer when shutting down the rcu tasks

[Tvrtko Ursulin]

On 31/10/2016 10:26, Chris Wilson wrote:
> To flush all call_rcu() tasks (here from i915_gem_free_object()) we need
> to call rcu_barrier() (not synchronize_rcu()). If we don't then we may
> still have objects being freed as we continue to teardown the driver -
> in particular, the recently released rings may race with the memory
> manager shutdown resulting in sporadic:
>
> [  142.217186] WARNING: CPU: 7 PID: 6185 at drivers/gpu/drm/drm_mm.c:932 drm_mm_takedown+0x2e/0x40
> [  142.217187] Memory manager not clean during takedown.
> [  142.217187] Modules linked in: i915(-) x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel lpc_ich snd_hda_codec_realtek snd_hda_codec_generic mei_me mei snd_hda_codec_hdmi snd_hda_codec snd_hwdep snd_hda_core snd_pcm e1000e ptp pps_core [last unloaded: snd_hda_intel]
> [  142.217199] CPU: 7 PID: 6185 Comm: rmmod Not tainted 4.9.0-rc2-CI-Trybot_242+ #1
> [  142.217199] Hardware name: LENOVO 10AGS00601/SHARKBAY, BIOS FBKT34AUS 04/24/2013
> [  142.217200]  ffffc90002ecfce0 ffffffff8142dd65 ffffc90002ecfd30 0000000000000000
> [  142.217202]  ffffc90002ecfd20 ffffffff8107e4e6 000003a40778c2a8 ffff880401355c48
> [  142.217204]  ffff88040778c2a8 ffffffffa040f3c0 ffffffffa040f4a0 00005621fbf8b1f0
> [  142.217206] Call Trace:
> [  142.217209]  [<ffffffff8142dd65>] dump_stack+0x67/0x92
> [  142.217211]  [<ffffffff8107e4e6>] __warn+0xc6/0xe0
> [  142.217213]  [<ffffffff8107e54a>] warn_slowpath_fmt+0x4a/0x50
> [  142.217214]  [<ffffffff81559e3e>] drm_mm_takedown+0x2e/0x40
> [  142.217236]  [<ffffffffa035c02a>] i915_gem_cleanup_stolen+0x1a/0x20 [i915]
> [  142.217246]  [<ffffffffa034c581>] i915_ggtt_cleanup_hw+0x31/0xb0 [i915]
> [  142.217253]  [<ffffffffa0310311>] i915_driver_cleanup_hw+0x31/0x40 [i915]
> [  142.217260]  [<ffffffffa0312001>] i915_driver_unload+0x141/0x1a0 [i915]
> [  142.217268]  [<ffffffffa031c2c4>] i915_pci_remove+0x14/0x20 [i915]
> [  142.217269]  [<ffffffff8147d214>] pci_device_remove+0x34/0xb0
> [  142.217271]  [<ffffffff8157b14c>] __device_release_driver+0x9c/0x150
> [  142.217272]  [<ffffffff8157bcc6>] driver_detach+0xb6/0xc0
> [  142.217273]  [<ffffffff8157abe3>] bus_remove_driver+0x53/0xd0
> [  142.217274]  [<ffffffff8157c787>] driver_unregister+0x27/0x50
> [  142.217276]  [<ffffffff8147c265>] pci_unregister_driver+0x25/0x70
> [  142.217287]  [<ffffffffa03d764c>] i915_exit+0x1a/0x71 [i915]
> [  142.217289]  [<ffffffff811136b3>] SyS_delete_module+0x193/0x1e0
> [  142.217291]  [<ffffffff818174ae>] entry_SYSCALL_64_fastpath+0x1c/0xb1
> [  142.217292] ---[ end trace 6fd164859c154772 ]---
> [  142.217505] [drm:show_leaks] *ERROR* node [6b6b6b6b6b6b6b6b + 6b6b6b6b6b6b6b6b]: inserted at
>                 [<ffffffff81559ff3>] save_stack.isra.1+0x53/0xa0
>                 [<ffffffff8155a98d>] drm_mm_insert_node_in_range_generic+0x2ad/0x360
>                 [<ffffffffa035bf23>] i915_gem_stolen_insert_node_in_range+0x93/0xe0 [i915]
>                 [<ffffffffa035c855>] i915_gem_object_create_stolen+0x75/0xb0 [i915]
>                 [<ffffffffa036a51a>] intel_engine_create_ring+0x9a/0x140 [i915]
>                 [<ffffffffa036a921>] intel_init_ring_buffer+0xf1/0x440 [i915]
>                 [<ffffffffa036be1b>] intel_init_render_ring_buffer+0xab/0x1b0 [i915]
>                 [<ffffffffa0363d08>] intel_engines_init+0xc8/0x210 [i915]
>                 [<ffffffffa0355d7c>] i915_gem_init+0xac/0xf0 [i915]
>                 [<ffffffffa0311454>] i915_driver_load+0x9c4/0x1430 [i915]
>                 [<ffffffffa031c2f8>] i915_pci_probe+0x28/0x40 [i915]
>                 [<ffffffff8147d315>] pci_device_probe+0x85/0xf0
>                 [<ffffffff8157b7ff>] driver_probe_device+0x21f/0x430
>                 [<ffffffff8157baee>] __driver_attach+0xde/0xe0
>
> In particular note that the node was being poisoned as we inspected the
> list, a  clear indication that the object is being freed as we make the
> assertion.
>
> Fixes: fbbd37b36fa5 ("drm/i915: Move object release to a freelist + worker")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 839ce2ae38fa..ed01421e3be7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -544,8 +544,10 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
>  	i915_gem_context_fini(&dev_priv->drm);
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>
> -	synchronize_rcu();
> -	flush_work(&dev_priv->mm.free_work);
> +	do {
> +		rcu_barrier();
> +		flush_work(&dev_priv->mm.free_work);
> +	} while (!llist_empty(&dev_priv->mm.free_list));
>
>  	WARN_ON(!list_empty(&dev_priv->context_list));
>  }
>

Not sure that the loop is required - after the rcu_barrier all the 
queued up additions have been added to the list and workers queued. So 
flush_work afterwards handles those and we are done, no?

If I am right it could be written as:

rcu_barrier();
flush_work(...);
WARN_ON(!llist_empty(...);

If I am not right then it sounds pretty fishy since it implies new 
objects are being freed in parallel to i915_gem_fini. Hm?

It fixes the CI fire so r-b anyway if you want to merge it while 
considering the above:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko