| Message ID | 1396728482-25402-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Apr 05, 2014 at 01:08:02PM -0700, Ben Widawsky wrote: > Our current code cannot handle a failure to evict well. You'll get at > the very least the following splat, but usually a lot worse fallout after: > > [ 134.819441] ------------[ cut here ]------------ > [ 134.819467] WARNING: CPU: 3 PID: 442 at drivers/gpu/drm/i915/i915_gem_evict.c:230 i915_gem_evict_vm+0x8a/0x1c0 [i915]() > [ 134.819471] Modules linked in: i915 drm_kms_helper drm intel_gtt agpgart i2c_algo_bit ext4 crc16 mbcache jbd2 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32c_intel ghash_clmulni_intel aesni_intel aes_x86_64 lrw iTCO_wdt gf128mul iTCO_vendor_support glue_helper ablk_helper cryptd microcode serio_raw i2c_i801 fan thermal battery e1000e acpi_cpufreq evdev ptp ac acpi_pad pps_core processor lpc_ich mfd_core snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_timer snd soundcore sd_mod crc_t10dif crct10dif_common ahci libahci libata ehci_pci ehci_hcd usbcore scsi_mod usb_common > [ 134.819565] CPU: 3 PID: 442 Comm: glxgears Not tainted 3.14.0-BEN+ #480 > [ 134.819568] Hardware name: Intel Corporation Broadwell Client platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0063.R01.1402110503 02/11/2014 > [ 134.819571] 0000000000000009 ffff88009b10fa80 ffffffff8159e6a5 0000000000000000 > [ 134.819577] ffff88009b10fab8 ffffffff8104895d ffff880145c353c0 ffff880145f400f8 > [ 134.819584] 0000000000000000 ffff8800a274d300 ffff88009b10fb78 ffff88009b10fac8 > [ 134.819590] Call Trace: > [ 134.819599] [<ffffffff8159e6a5>] dump_stack+0x4e/0x7a > [ 134.819607] [<ffffffff8104895d>] warn_slowpath_common+0x7d/0xa0 > [ 134.819635] [<ffffffff81048a3a>] warn_slowpath_null+0x1a/0x20 > [ 134.819656] [<ffffffffa050c82a>] i915_gem_evict_vm+0x8a/0x1c0 [i915] > [ 134.819677] [<ffffffffa050a26b>] ppgtt_release+0x17b/0x1e0 [i915] > [ 134.819693] [<ffffffffa050a34d>] i915_gem_context_free+0x7d/0x180 [i915] > [ 134.819707] [<ffffffffa050a48c>] context_idr_cleanup+0x3c/0x40 [i915] > [ 134.819715] [<ffffffff81332d14>] idr_for_each+0x104/0x1a0 > [ 134.819730] [<ffffffffa050a450>] ? i915_gem_context_free+0x180/0x180 [i915] > [ 134.819735] [<ffffffff815a27fc>] ? mutex_lock_nested+0x28c/0x3d0 > [ 134.819761] [<ffffffffa057da75>] ? i915_driver_preclose+0x25/0x50 [i915] > [ 134.819778] [<ffffffffa050b715>] i915_gem_context_close+0x35/0xa0 [i915] > [ 134.819802] [<ffffffffa057da80>] i915_driver_preclose+0x30/0x50 [i915] > [ 134.819816] [<ffffffffa03e6a7d>] drm_release+0x5d/0x5f0 [drm] > [ 134.819822] [<ffffffff811aae3a>] __fput+0xea/0x240 > [ 134.819827] [<ffffffff811aafde>] ____fput+0xe/0x10 > [ 134.819832] [<ffffffff810701bc>] task_work_run+0xac/0xe0 > [ 134.819837] [<ffffffff8104b82f>] do_exit+0x2cf/0xcf0 > [ 134.819844] [<ffffffff815a5dac>] ? _raw_spin_unlock_irq+0x2c/0x60 > [ 134.819849] [<ffffffff8104c2dc>] do_group_exit+0x4c/0xc0 > [ 134.819855] [<ffffffff8105ef11>] get_signal_to_deliver+0x2d1/0x920 > [ 134.819861] [<ffffffff81002408>] do_signal+0x48/0x620 > [ 134.819867] [<ffffffff811aa0d9>] ? do_readv_writev+0x169/0x220 > [ 134.819873] [<ffffffff8109e33d>] ? trace_hardirqs_on_caller+0xfd/0x1c0 > [ 134.819879] [<ffffffff811c9b9d>] ? __fget_light+0x13d/0x160 > [ 134.819886] [<ffffffff815a744c>] ? sysret_signal+0x5/0x47 > [ 134.819892] [<ffffffff81002a45>] do_notify_resume+0x65/0x80 > [ 134.819897] [<ffffffff815a76da>] int_signal+0x12/0x17 > [ 134.819901] ---[ end trace dbf4da2122c3d683 ]--- > > At first I was going to call this a bandage to the problem. However, > upon further thought, I rather like the idea of making evictions atomic, > and less prone to failure anyway. The reason it can still somewhat be > considered a band-aid however is GPU hangs. It would be nice if we had > some way to interrupt the process when the GPU is hung. I'll leave it > for a follow patch though. No, this should be decided by the caller to i915_gem_evict_vm(), because we very much do want it to be interruptible in most cases. The filp close path is certainly one where being non-interruptible seems justifiable. However, this is very much papering over the bug that we should never be freeing an active vm in the first place. -Chris
On Sat, Apr 05, 2014 at 09:34:12PM +0100, Chris Wilson wrote: > On Sat, Apr 05, 2014 at 01:08:02PM -0700, Ben Widawsky wrote: > > Our current code cannot handle a failure to evict well. You'll get at > > the very least the following splat, but usually a lot worse fallout after: > > > > [ 134.819441] ------------[ cut here ]------------ > > [ 134.819467] WARNING: CPU: 3 PID: 442 at drivers/gpu/drm/i915/i915_gem_evict.c:230 i915_gem_evict_vm+0x8a/0x1c0 [i915]() > > [ 134.819471] Modules linked in: i915 drm_kms_helper drm intel_gtt agpgart i2c_algo_bit ext4 crc16 mbcache jbd2 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32c_intel ghash_clmulni_intel aesni_intel aes_x86_64 lrw iTCO_wdt gf128mul iTCO_vendor_support glue_helper ablk_helper cryptd microcode serio_raw i2c_i801 fan thermal battery e1000e acpi_cpufreq evdev ptp ac acpi_pad pps_core processor lpc_ich mfd_core snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_timer snd soundcore sd_mod crc_t10dif crct10dif_common ahci libahci libata ehci_pci ehci_hcd usbcore scsi_mod usb_common > > [ 134.819565] CPU: 3 PID: 442 Comm: glxgears Not tainted 3.14.0-BEN+ #480 > > [ 134.819568] Hardware name: Intel Corporation Broadwell Client platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0063.R01.1402110503 02/11/2014 > > [ 134.819571] 0000000000000009 ffff88009b10fa80 ffffffff8159e6a5 0000000000000000 > > [ 134.819577] ffff88009b10fab8 ffffffff8104895d ffff880145c353c0 ffff880145f400f8 > > [ 134.819584] 0000000000000000 ffff8800a274d300 ffff88009b10fb78 ffff88009b10fac8 > > [ 134.819590] Call Trace: > > [ 134.819599] [<ffffffff8159e6a5>] dump_stack+0x4e/0x7a > > [ 134.819607] [<ffffffff8104895d>] warn_slowpath_common+0x7d/0xa0 > > [ 134.819635] [<ffffffff81048a3a>] warn_slowpath_null+0x1a/0x20 > > [ 134.819656] [<ffffffffa050c82a>] i915_gem_evict_vm+0x8a/0x1c0 [i915] > > [ 134.819677] [<ffffffffa050a26b>] ppgtt_release+0x17b/0x1e0 [i915] > > [ 134.819693] [<ffffffffa050a34d>] i915_gem_context_free+0x7d/0x180 [i915] > > [ 134.819707] [<ffffffffa050a48c>] context_idr_cleanup+0x3c/0x40 [i915] > > [ 134.819715] [<ffffffff81332d14>] idr_for_each+0x104/0x1a0 > > [ 134.819730] [<ffffffffa050a450>] ? i915_gem_context_free+0x180/0x180 [i915] > > [ 134.819735] [<ffffffff815a27fc>] ? mutex_lock_nested+0x28c/0x3d0 > > [ 134.819761] [<ffffffffa057da75>] ? i915_driver_preclose+0x25/0x50 [i915] > > [ 134.819778] [<ffffffffa050b715>] i915_gem_context_close+0x35/0xa0 [i915] > > [ 134.819802] [<ffffffffa057da80>] i915_driver_preclose+0x30/0x50 [i915] > > [ 134.819816] [<ffffffffa03e6a7d>] drm_release+0x5d/0x5f0 [drm] > > [ 134.819822] [<ffffffff811aae3a>] __fput+0xea/0x240 > > [ 134.819827] [<ffffffff811aafde>] ____fput+0xe/0x10 > > [ 134.819832] [<ffffffff810701bc>] task_work_run+0xac/0xe0 > > [ 134.819837] [<ffffffff8104b82f>] do_exit+0x2cf/0xcf0 > > [ 134.819844] [<ffffffff815a5dac>] ? _raw_spin_unlock_irq+0x2c/0x60 > > [ 134.819849] [<ffffffff8104c2dc>] do_group_exit+0x4c/0xc0 > > [ 134.819855] [<ffffffff8105ef11>] get_signal_to_deliver+0x2d1/0x920 > > [ 134.819861] [<ffffffff81002408>] do_signal+0x48/0x620 > > [ 134.819867] [<ffffffff811aa0d9>] ? do_readv_writev+0x169/0x220 > > [ 134.819873] [<ffffffff8109e33d>] ? trace_hardirqs_on_caller+0xfd/0x1c0 > > [ 134.819879] [<ffffffff811c9b9d>] ? __fget_light+0x13d/0x160 > > [ 134.819886] [<ffffffff815a744c>] ? sysret_signal+0x5/0x47 > > [ 134.819892] [<ffffffff81002a45>] do_notify_resume+0x65/0x80 > > [ 134.819897] [<ffffffff815a76da>] int_signal+0x12/0x17 > > [ 134.819901] ---[ end trace dbf4da2122c3d683 ]--- > > > > At first I was going to call this a bandage to the problem. However, > > upon further thought, I rather like the idea of making evictions atomic, > > and less prone to failure anyway. The reason it can still somewhat be > > considered a band-aid however is GPU hangs. It would be nice if we had > > some way to interrupt the process when the GPU is hung. I'll leave it > > for a follow patch though. > > No, this should be decided by the caller to i915_gem_evict_vm(), because > we very much do want it to be interruptible in most cases. The filp close > path is certainly one where being non-interruptible seems justifiable. > > However, this is very much papering over the bug that we should never be > freeing an active vm in the first place. > -Chris > The issue I was seeing appeared to seeing from sigkill. In such a case, the process may want to die before the context/work/address space is freeable. For example: 1. evict_vm called for whatever reason 2. wait_seqno because the VMA is still active 3. receive signal break out of wait_seqno 4. return to evict_vm and the above WARN Our error handling from there just spirals. One issue I have with our current code is I'd really like eviction to not be able to fail (obviously extreme cases are unavoidable). Perhaps one other solution would be to make sure the context is idled before evicting its VM.
On Sat, Apr 05, 2014 at 07:45:28PM -0700, Ben Widawsky wrote: > On Sat, Apr 05, 2014 at 09:34:12PM +0100, Chris Wilson wrote: > > On Sat, Apr 05, 2014 at 01:08:02PM -0700, Ben Widawsky wrote: > > > Our current code cannot handle a failure to evict well. You'll get at > > > the very least the following splat, but usually a lot worse fallout after: > > > > > > [ 134.819441] ------------[ cut here ]------------ > > > [ 134.819467] WARNING: CPU: 3 PID: 442 at drivers/gpu/drm/i915/i915_gem_evict.c:230 i915_gem_evict_vm+0x8a/0x1c0 [i915]() > > > [ 134.819471] Modules linked in: i915 drm_kms_helper drm intel_gtt agpgart i2c_algo_bit ext4 crc16 mbcache jbd2 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32c_intel ghash_clmulni_intel aesni_intel aes_x86_64 lrw iTCO_wdt gf128mul iTCO_vendor_support glue_helper ablk_helper cryptd microcode serio_raw i2c_i801 fan thermal battery e1000e acpi_cpufreq evdev ptp ac acpi_pad pps_core processor lpc_ich mfd_core snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_timer snd soundcore sd_mod crc_t10dif crct10dif_common ahci libahci libata ehci_pci ehci_hcd usbcore scsi_mod usb_common > > > [ 134.819565] CPU: 3 PID: 442 Comm: glxgears Not tainted 3.14.0-BEN+ #480 > > > [ 134.819568] Hardware name: Intel Corporation Broadwell Client platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0063.R01.1402110503 02/11/2014 > > > [ 134.819571] 0000000000000009 ffff88009b10fa80 ffffffff8159e6a5 0000000000000000 > > > [ 134.819577] ffff88009b10fab8 ffffffff8104895d ffff880145c353c0 ffff880145f400f8 > > > [ 134.819584] 0000000000000000 ffff8800a274d300 ffff88009b10fb78 ffff88009b10fac8 > > > [ 134.819590] Call Trace: > > > [ 134.819599] [<ffffffff8159e6a5>] dump_stack+0x4e/0x7a > > > [ 134.819607] [<ffffffff8104895d>] warn_slowpath_common+0x7d/0xa0 > > > [ 134.819635] [<ffffffff81048a3a>] warn_slowpath_null+0x1a/0x20 > > > [ 134.819656] [<ffffffffa050c82a>] i915_gem_evict_vm+0x8a/0x1c0 [i915] > > > [ 134.819677] [<ffffffffa050a26b>] ppgtt_release+0x17b/0x1e0 [i915] > > > [ 134.819693] [<ffffffffa050a34d>] i915_gem_context_free+0x7d/0x180 [i915] > > > [ 134.819707] [<ffffffffa050a48c>] context_idr_cleanup+0x3c/0x40 [i915] > > > [ 134.819715] [<ffffffff81332d14>] idr_for_each+0x104/0x1a0 > > > [ 134.819730] [<ffffffffa050a450>] ? i915_gem_context_free+0x180/0x180 [i915] > > > [ 134.819735] [<ffffffff815a27fc>] ? mutex_lock_nested+0x28c/0x3d0 > > > [ 134.819761] [<ffffffffa057da75>] ? i915_driver_preclose+0x25/0x50 [i915] > > > [ 134.819778] [<ffffffffa050b715>] i915_gem_context_close+0x35/0xa0 [i915] > > > [ 134.819802] [<ffffffffa057da80>] i915_driver_preclose+0x30/0x50 [i915] > > > [ 134.819816] [<ffffffffa03e6a7d>] drm_release+0x5d/0x5f0 [drm] > > > [ 134.819822] [<ffffffff811aae3a>] __fput+0xea/0x240 > > > [ 134.819827] [<ffffffff811aafde>] ____fput+0xe/0x10 > > > [ 134.819832] [<ffffffff810701bc>] task_work_run+0xac/0xe0 > > > [ 134.819837] [<ffffffff8104b82f>] do_exit+0x2cf/0xcf0 > > > [ 134.819844] [<ffffffff815a5dac>] ? _raw_spin_unlock_irq+0x2c/0x60 > > > [ 134.819849] [<ffffffff8104c2dc>] do_group_exit+0x4c/0xc0 > > > [ 134.819855] [<ffffffff8105ef11>] get_signal_to_deliver+0x2d1/0x920 > > > [ 134.819861] [<ffffffff81002408>] do_signal+0x48/0x620 > > > [ 134.819867] [<ffffffff811aa0d9>] ? do_readv_writev+0x169/0x220 > > > [ 134.819873] [<ffffffff8109e33d>] ? trace_hardirqs_on_caller+0xfd/0x1c0 > > > [ 134.819879] [<ffffffff811c9b9d>] ? __fget_light+0x13d/0x160 > > > [ 134.819886] [<ffffffff815a744c>] ? sysret_signal+0x5/0x47 > > > [ 134.819892] [<ffffffff81002a45>] do_notify_resume+0x65/0x80 > > > [ 134.819897] [<ffffffff815a76da>] int_signal+0x12/0x17 > > > [ 134.819901] ---[ end trace dbf4da2122c3d683 ]--- > > > > > > At first I was going to call this a bandage to the problem. However, > > > upon further thought, I rather like the idea of making evictions atomic, > > > and less prone to failure anyway. The reason it can still somewhat be > > > considered a band-aid however is GPU hangs. It would be nice if we had > > > some way to interrupt the process when the GPU is hung. I'll leave it > > > for a follow patch though. > > > > No, this should be decided by the caller to i915_gem_evict_vm(), because > > we very much do want it to be interruptible in most cases. The filp close > > path is certainly one where being non-interruptible seems justifiable. > > > > However, this is very much papering over the bug that we should never be > > freeing an active vm in the first place. > > -Chris > > > > The issue I was seeing appeared to seeing from sigkill. In such a case, > the process may want to die before the context/work/address space is > freeable. For example: > 1. evict_vm called for whatever reason > 2. wait_seqno because the VMA is still active hmm something isn't right here. Why did I get to wait_seqno if pin_count was 0? Just FYI, this wasn't hypothetical. I did trace it all the way to exactly ERESTARTSYS from wait_seqno. By the way, another option in evict would be: while(ret = (i915_vma_unbind(vma) == -ERESTARTSYS)); WARN_ON(ret); > 3. receive signal break out of wait_seqno > 4. return to evict_vm and the above WARN > > Our error handling from there just spirals. > > One issue I have with our current code is I'd really like eviction to > not be able to fail (obviously extreme cases are unavoidable). Perhaps > one other solution would be to make sure the context is idled before > evicting its VM. > > > -- > Ben Widawsky, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sun, Apr 06, 2014 at 11:35:03AM -0700, Ben Widawsky wrote: > On Sat, Apr 05, 2014 at 07:45:28PM -0700, Ben Widawsky wrote: > > The issue I was seeing appeared to seeing from sigkill. In such a case, > > the process may want to die before the context/work/address space is > > freeable. For example: > > 1. evict_vm called for whatever reason > > 2. wait_seqno because the VMA is still active > > hmm something isn't right here. Why did I get to wait_seqno if pin_count > was 0? Just FYI, this wasn't hypothetical. I did trace it all the way to > exactly ERESTARTSYS from wait_seqno. > > By the way, another option in evict would be: > while(ret = (i915_vma_unbind(vma) == -ERESTARTSYS)); > WARN_ON(ret); > > > 3. receive signal break out of wait_seqno > > 4. return to evict_vm and the above WARN > > > > Our error handling from there just spirals. > > > > One issue I have with our current code is I'd really like eviction to > > not be able to fail (obviously extreme cases are unavoidable). This is unrealistic since we must support X which uses sigtimer. > > Perhaps > > one other solution would be to make sure the context is idled before > > evicting its VM. Indeed. Anyway, I do concur that wrapping i915_driver_preclose() with dev_priv->mm.interruptible = false; would make us both happy. -Chris
On Mon, Apr 07, 2014 at 10:42:56AM +0100, Chris Wilson wrote: > On Sun, Apr 06, 2014 at 11:35:03AM -0700, Ben Widawsky wrote: > > On Sat, Apr 05, 2014 at 07:45:28PM -0700, Ben Widawsky wrote: > > > The issue I was seeing appeared to seeing from sigkill. In such a case, > > > the process may want to die before the context/work/address space is > > > freeable. For example: > > > 1. evict_vm called for whatever reason > > > 2. wait_seqno because the VMA is still active > > > > hmm something isn't right here. Why did I get to wait_seqno if pin_count > > was 0? Just FYI, this wasn't hypothetical. I did trace it all the way to > > exactly ERESTARTSYS from wait_seqno. > > > > By the way, another option in evict would be: > > while(ret = (i915_vma_unbind(vma) == -ERESTARTSYS)); > > WARN_ON(ret); > > > > > 3. receive signal break out of wait_seqno > > > 4. return to evict_vm and the above WARN > > > > > > Our error handling from there just spirals. > > > > > > One issue I have with our current code is I'd really like eviction to > > > not be able to fail (obviously extreme cases are unavoidable). > > This is unrealistic since we must support X which uses sigtimer. > > > > Perhaps > > > one other solution would be to make sure the context is idled before > > > evicting its VM. > > Indeed. > > Anyway, I do concur that wrapping i915_driver_preclose() with > > dev_priv->mm.interruptible = false; > > would make us both happy. Isn't the backtrace just fallout from the lifetime rules being a bit funny? We didn't uninterruptibly stall for any still active bo when the drm fd gets closed, why do we suddenly need to do that with ppgtts? Iirc requests hold a ref on the context, contexts hold a ref on the ppgtt and so the entire thing should only dissipate once it's really idle. Imo just doing uninterruptible sleeps tastes way too much like duct-tape. I can be convinced of duct-tape if the tradeoffs really strongly suggests it's the right thing (e.g. the shrinker lock stealing, even though we've paid a hefty price in accidental complexity with that one), but that needs some good justification. -Daniel
On Mon, Apr 07, 2014 at 02:15:00PM +0200, Daniel Vetter wrote: > On Mon, Apr 07, 2014 at 10:42:56AM +0100, Chris Wilson wrote: > > On Sun, Apr 06, 2014 at 11:35:03AM -0700, Ben Widawsky wrote: > > > On Sat, Apr 05, 2014 at 07:45:28PM -0700, Ben Widawsky wrote: > > > > The issue I was seeing appeared to seeing from sigkill. In such a case, > > > > the process may want to die before the context/work/address space is > > > > freeable. For example: > > > > 1. evict_vm called for whatever reason > > > > 2. wait_seqno because the VMA is still active > > > > > > hmm something isn't right here. Why did I get to wait_seqno if pin_count > > > was 0? Just FYI, this wasn't hypothetical. I did trace it all the way to > > > exactly ERESTARTSYS from wait_seqno. > > > > > > By the way, another option in evict would be: > > > while(ret = (i915_vma_unbind(vma) == -ERESTARTSYS)); > > > WARN_ON(ret); > > > > > > > 3. receive signal break out of wait_seqno > > > > 4. return to evict_vm and the above WARN > > > > > > > > Our error handling from there just spirals. > > > > > > > > One issue I have with our current code is I'd really like eviction to > > > > not be able to fail (obviously extreme cases are unavoidable). > > > > This is unrealistic since we must support X which uses sigtimer. > > > > > > Perhaps > > > > one other solution would be to make sure the context is idled before > > > > evicting its VM. > > > > Indeed. > > > > Anyway, I do concur that wrapping i915_driver_preclose() with > > > > dev_priv->mm.interruptible = false; > > > > would make us both happy. > > Isn't the backtrace just fallout from the lifetime rules being a bit > funny? We didn't uninterruptibly stall for any still active bo when the > drm fd gets closed, why do we suddenly need to do that with ppgtts? Iirc > requests hold a ref on the context, contexts hold a ref on the ppgtt and > so the entire thing should only dissipate once it's really idle. > > Imo just doing uninterruptible sleeps tastes way too much like duct-tape. > I can be convinced of duct-tape if the tradeoffs really strongly suggests > it's the right thing (e.g. the shrinker lock stealing, even though we've > paid a hefty price in accidental complexity with that one), but that needs > some good justification. Yes, it is duct-tape. But it should be duct-tape against future unknown bugs (and the currently known bugs) in that the i915_driver_preclose() cannot report failure and so should not allow its callees to fail (which is more or less the contract given by .interruptible=false). The alternative is to allow preclose() to support an error-code, which has the issue that very few programs check for errors during close() and that EINTR from close() is frowned upon by most. -Chris
On Mon, Apr 07, 2014 at 01:30:04PM +0100, Chris Wilson wrote: > On Mon, Apr 07, 2014 at 02:15:00PM +0200, Daniel Vetter wrote: > > On Mon, Apr 07, 2014 at 10:42:56AM +0100, Chris Wilson wrote: > > > On Sun, Apr 06, 2014 at 11:35:03AM -0700, Ben Widawsky wrote: > > > > On Sat, Apr 05, 2014 at 07:45:28PM -0700, Ben Widawsky wrote: > > > > > The issue I was seeing appeared to seeing from sigkill. In such a case, > > > > > the process may want to die before the context/work/address space is > > > > > freeable. For example: > > > > > 1. evict_vm called for whatever reason > > > > > 2. wait_seqno because the VMA is still active > > > > > > > > hmm something isn't right here. Why did I get to wait_seqno if pin_count > > > > was 0? Just FYI, this wasn't hypothetical. I did trace it all the way to > > > > exactly ERESTARTSYS from wait_seqno. > > > > > > > > By the way, another option in evict would be: > > > > while(ret = (i915_vma_unbind(vma) == -ERESTARTSYS)); > > > > WARN_ON(ret); > > > > > > > > > 3. receive signal break out of wait_seqno > > > > > 4. return to evict_vm and the above WARN > > > > > > > > > > Our error handling from there just spirals. > > > > > > > > > > One issue I have with our current code is I'd really like eviction to > > > > > not be able to fail (obviously extreme cases are unavoidable). > > > > > > This is unrealistic since we must support X which uses sigtimer. > > > > > > > > Perhaps > > > > > one other solution would be to make sure the context is idled before > > > > > evicting its VM. > > > > > > Indeed. > > > > > > Anyway, I do concur that wrapping i915_driver_preclose() with > > > > > > dev_priv->mm.interruptible = false; > > > > > > would make us both happy. > > > > Isn't the backtrace just fallout from the lifetime rules being a bit > > funny? We didn't uninterruptibly stall for any still active bo when the > > drm fd gets closed, why do we suddenly need to do that with ppgtts? Iirc > > requests hold a ref on the context, contexts hold a ref on the ppgtt and > > so the entire thing should only dissipate once it's really idle. > > > > Imo just doing uninterruptible sleeps tastes way too much like duct-tape. > > I can be convinced of duct-tape if the tradeoffs really strongly suggests > > it's the right thing (e.g. the shrinker lock stealing, even though we've > > paid a hefty price in accidental complexity with that one), but that needs > > some good justification. > > Yes, it is duct-tape. But it should be duct-tape against future unknown > bugs (and the currently known bugs) in that the i915_driver_preclose() > cannot report failure and so should not allow its callees to fail (which > is more or less the contract given by .interruptible=false). > > The alternative is to allow preclose() to support an error-code, which > has the issue that very few programs check for errors during close() and > that EINTR from close() is frowned upon by most. > -Chris > Do we have consensus? I am good with Chris' idea. I can write and test the patch.
On Mon, Apr 07, 2014 at 11:58:28AM -0700, Ben Widawsky wrote: > On Mon, Apr 07, 2014 at 01:30:04PM +0100, Chris Wilson wrote: > > On Mon, Apr 07, 2014 at 02:15:00PM +0200, Daniel Vetter wrote: > > > On Mon, Apr 07, 2014 at 10:42:56AM +0100, Chris Wilson wrote: > > > > On Sun, Apr 06, 2014 at 11:35:03AM -0700, Ben Widawsky wrote: > > > > > On Sat, Apr 05, 2014 at 07:45:28PM -0700, Ben Widawsky wrote: > > > > > > The issue I was seeing appeared to seeing from sigkill. In such a case, > > > > > > the process may want to die before the context/work/address space is > > > > > > freeable. For example: > > > > > > 1. evict_vm called for whatever reason > > > > > > 2. wait_seqno because the VMA is still active > > > > > > > > > > hmm something isn't right here. Why did I get to wait_seqno if pin_count > > > > > was 0? Just FYI, this wasn't hypothetical. I did trace it all the way to > > > > > exactly ERESTARTSYS from wait_seqno. > > > > > > > > > > By the way, another option in evict would be: > > > > > while(ret = (i915_vma_unbind(vma) == -ERESTARTSYS)); > > > > > WARN_ON(ret); > > > > > > > > > > > 3. receive signal break out of wait_seqno > > > > > > 4. return to evict_vm and the above WARN > > > > > > > > > > > > Our error handling from there just spirals. > > > > > > > > > > > > One issue I have with our current code is I'd really like eviction to > > > > > > not be able to fail (obviously extreme cases are unavoidable). > > > > > > > > This is unrealistic since we must support X which uses sigtimer. > > > > > > > > > > Perhaps > > > > > > one other solution would be to make sure the context is idled before > > > > > > evicting its VM. > > > > > > > > Indeed. > > > > > > > > Anyway, I do concur that wrapping i915_driver_preclose() with > > > > > > > > dev_priv->mm.interruptible = false; > > > > > > > > would make us both happy. > > > > > > Isn't the backtrace just fallout from the lifetime rules being a bit > > > funny? We didn't uninterruptibly stall for any still active bo when the > > > drm fd gets closed, why do we suddenly need to do that with ppgtts? Iirc > > > requests hold a ref on the context, contexts hold a ref on the ppgtt and > > > so the entire thing should only dissipate once it's really idle. > > > > > > Imo just doing uninterruptible sleeps tastes way too much like duct-tape. > > > I can be convinced of duct-tape if the tradeoffs really strongly suggests > > > it's the right thing (e.g. the shrinker lock stealing, even though we've > > > paid a hefty price in accidental complexity with that one), but that needs > > > some good justification. > > > > Yes, it is duct-tape. But it should be duct-tape against future unknown > > bugs (and the currently known bugs) in that the i915_driver_preclose() > > cannot report failure and so should not allow its callees to fail (which > > is more or less the contract given by .interruptible=false). > > > > The alternative is to allow preclose() to support an error-code, which > > has the issue that very few programs check for errors during close() and > > that EINTR from close() is frowned upon by most. > > -Chris > > > > Do we have consensus? I am good with Chris' idea. I can write and test > the patch. If we just disable interrupts we'll never see the bug reports again, which is imo bad. And the while loop above will loop a bit too long. Adding a new special sleep mode which doesn't interrupt but WARNs badly if any sleeping actually happens is also a bit tricky, and pretty much guaranteed to blow up. I'm still for a "write testcase, wait for fix to materialize" approach. Especially for upstream. The testcase is especially important so that we can track it as a full ppgtt validation criterion. I'll add this tomorrow as a task to the relevant JIRA. -Daniel
On Mon, Apr 07, 2014 at 11:50:06PM +0200, Daniel Vetter wrote: > On Mon, Apr 07, 2014 at 11:58:28AM -0700, Ben Widawsky wrote: > > On Mon, Apr 07, 2014 at 01:30:04PM +0100, Chris Wilson wrote: > > > On Mon, Apr 07, 2014 at 02:15:00PM +0200, Daniel Vetter wrote: > > > > On Mon, Apr 07, 2014 at 10:42:56AM +0100, Chris Wilson wrote: > > > > > On Sun, Apr 06, 2014 at 11:35:03AM -0700, Ben Widawsky wrote: > > > > > > On Sat, Apr 05, 2014 at 07:45:28PM -0700, Ben Widawsky wrote: > > > > > > > The issue I was seeing appeared to seeing from sigkill. In such a case, > > > > > > > the process may want to die before the context/work/address space is > > > > > > > freeable. For example: > > > > > > > 1. evict_vm called for whatever reason > > > > > > > 2. wait_seqno because the VMA is still active > > > > > > > > > > > > hmm something isn't right here. Why did I get to wait_seqno if pin_count > > > > > > was 0? Just FYI, this wasn't hypothetical. I did trace it all the way to > > > > > > exactly ERESTARTSYS from wait_seqno. > > > > > > > > > > > > By the way, another option in evict would be: > > > > > > while(ret = (i915_vma_unbind(vma) == -ERESTARTSYS)); > > > > > > WARN_ON(ret); > > > > > > > > > > > > > 3. receive signal break out of wait_seqno > > > > > > > 4. return to evict_vm and the above WARN > > > > > > > > > > > > > > Our error handling from there just spirals. > > > > > > > > > > > > > > One issue I have with our current code is I'd really like eviction to > > > > > > > not be able to fail (obviously extreme cases are unavoidable). > > > > > > > > > > This is unrealistic since we must support X which uses sigtimer. > > > > > > > > > > > > Perhaps > > > > > > > one other solution would be to make sure the context is idled before > > > > > > > evicting its VM. > > > > > > > > > > Indeed. > > > > > > > > > > Anyway, I do concur that wrapping i915_driver_preclose() with > > > > > > > > > > dev_priv->mm.interruptible = false; > > > > > > > > > > would make us both happy. > > > > > > > > Isn't the backtrace just fallout from the lifetime rules being a bit > > > > funny? We didn't uninterruptibly stall for any still active bo when the > > > > drm fd gets closed, why do we suddenly need to do that with ppgtts? Iirc > > > > requests hold a ref on the context, contexts hold a ref on the ppgtt and > > > > so the entire thing should only dissipate once it's really idle. > > > > > > > > Imo just doing uninterruptible sleeps tastes way too much like duct-tape. > > > > I can be convinced of duct-tape if the tradeoffs really strongly suggests > > > > it's the right thing (e.g. the shrinker lock stealing, even though we've > > > > paid a hefty price in accidental complexity with that one), but that needs > > > > some good justification. > > > > > > Yes, it is duct-tape. But it should be duct-tape against future unknown > > > bugs (and the currently known bugs) in that the i915_driver_preclose() > > > cannot report failure and so should not allow its callees to fail (which > > > is more or less the contract given by .interruptible=false). > > > > > > The alternative is to allow preclose() to support an error-code, which > > > has the issue that very few programs check for errors during close() and > > > that EINTR from close() is frowned upon by most. > > > -Chris > > > > > > > Do we have consensus? I am good with Chris' idea. I can write and test > > the patch. > > If we just disable interrupts we'll never see the bug reports again, which > is imo bad. And the while loop above will loop a bit too long. > > Adding a new special sleep mode which doesn't interrupt but WARNs badly if > any sleeping actually happens is also a bit tricky, and pretty much > guaranteed to blow up. I'm still for a "write testcase, wait for fix to > materialize" approach. Especially for upstream. > > The testcase is especially important so that we can track it as a full > ppgtt validation criterion. I'll add this tomorrow as a task to the > relevant JIRA. > -Daniel Blocking important fixes for a test case is harmful to customers of our software. I won't argue past that. If you won't take it as is, add it to the JIRA task like you said. I'll carry this one around with my dynamic page table allocations since you essentially can't do any real workloads with full PPGTT without this (assuming you have signals). I'd venture to even say existing tests can hit it with full PPGTT turned on. The following will hit it on the very first iteration: #!/bin/bash while [ 1 ] ; do (glxgears) & pid[0]=$! (glxgears) & pid[1]=$! (glxgears) & pid[2]=$! sleep 3 kill ${pid[*]} done
On Mon, Apr 07, 2014 at 02:58:34PM -0700, Ben Widawsky wrote: > Blocking important fixes for a test case is harmful to customers of our > software. I won't argue past that. If you won't take it as is, add it to the > JIRA task like you said. I'll carry this one around with my dynamic page table > allocations since you essentially can't do any real workloads with full PPGTT > without this (assuming you have signals). I'd venture to even say existing > tests can hit it with full PPGTT turned on. > > The following will hit it on the very first iteration: > #!/bin/bash > > while [ 1 ] ; do > (glxgears) & pid[0]=$! > (glxgears) & pid[1]=$! > (glxgears) & pid[2]=$! > sleep 3 > kill ${pid[*]} > done You must agree that this is the fix for the above issue, but just a band aid to prevent it exploding upon the user? -Chris
On Mon, Apr 7, 2014 at 11:58 PM, Ben Widawsky <ben@bwidawsk.net> wrote: > Blocking important fixes for a test case is harmful to customers of our > software. I won't argue past that. If you won't take it as is, add it to the > JIRA task like you said. I'll carry this one around with my dynamic page table > allocations since you essentially can't do any real workloads with full PPGTT > without this (assuming you have signals). I'd venture to even say existing > tests can hit it with full PPGTT turned on. A duct-tape bugfix like this would be justified in late -rc, where the risk for disabling full ppgtt would probably outweight this hack. Earlier I'd opt of simply re-disbaling full ppgtt again if we can't come up with a properly understood fix and testcase for it in time. But atm full ppgtt is disabled, and we have a task-list with 10 subtasks on internal JIRA to knock down. Without that I wouldn't recommend anyone to try to ship full ppgtt in production, especially not our internal customers. I've updated the relevant subtask JIRA with details. -Daniel
On Tue, Apr 08, 2014 at 07:50:39AM +0100, Chris Wilson wrote: > On Mon, Apr 07, 2014 at 02:58:34PM -0700, Ben Widawsky wrote: > > Blocking important fixes for a test case is harmful to customers of our > > software. I won't argue past that. If you won't take it as is, add it to the > > JIRA task like you said. I'll carry this one around with my dynamic page table > > allocations since you essentially can't do any real workloads with full PPGTT > > without this (assuming you have signals). I'd venture to even say existing > > tests can hit it with full PPGTT turned on. > > > > The following will hit it on the very first iteration: > > #!/bin/bash > > > > while [ 1 ] ; do > > (glxgears) & pid[0]=$! > > (glxgears) & pid[1]=$! > > (glxgears) & pid[2]=$! > > sleep 3 > > kill ${pid[*]} > > done > > You must agree that this is the fix for the above issue, but just a band > aid to prevent it exploding upon the user? > -Chris > I agree, IFF any use of mm.interruptible is duct-tape. I *really* agree with your second, point wrt user explosion. We can argue until we're blue in the face about paper, and duct tape - the user impact is undeniable. Reading optional for the rest: This is no more or less flagrant than any other use. Evict CANNOT finish if we get interrupted by a signal. If we can't properly evict everything from the address space, I can't make any guarantee about anything being clean when we teardown, full stop. If you want to limit calling evict to when all vmas are idle, fine, but that's just as lame. wait_seqno() makes sense as is. Changing that is IMO is not at all preferable, and way more risky. As I said in the original patch, making eviction, which generally shouldn't have any failure paths, atomic-esque (it's just about freeing memory, and address space) makes complete sense to me. This is something I've been trying really hard to keep in the dynamic page table allocations as well. If a fini kind of function can fail, or block, it's useless.
On Tue, Apr 08, 2014 at 08:53:15AM +0200, Daniel Vetter wrote: > On Mon, Apr 7, 2014 at 11:58 PM, Ben Widawsky <ben@bwidawsk.net> wrote: > > Blocking important fixes for a test case is harmful to customers of our > > software. I won't argue past that. If you won't take it as is, add it to the > > JIRA task like you said. I'll carry this one around with my dynamic page table > > allocations since you essentially can't do any real workloads with full PPGTT > > without this (assuming you have signals). I'd venture to even say existing > > tests can hit it with full PPGTT turned on. > > A duct-tape bugfix like this would be justified in late -rc, where the > risk for disabling full ppgtt would probably outweight this hack. > Earlier I'd opt of simply re-disbaling full ppgtt again if we can't > come up with a properly understood fix and testcase for it in time. > > But atm full ppgtt is disabled, and we have a task-list with 10 > subtasks on internal JIRA to knock down. Without that I wouldn't > recommend anyone to try to ship full ppgtt in production, especially > not our internal customers. > > I've updated the relevant subtask JIRA with details. > -Daniel I felt confident when I wrote the original email it was hittable without full PPGTT. I am not quite certain now. If that is not the case, I agree with you.
On Tue, Apr 08, 2014 at 09:09:14PM -0700, Ben Widawsky wrote: > This is no more or less flagrant than any other use. Evict CANNOT finish > if we get interrupted by a signal. If we can't properly evict everything > from the address space, I can't make any guarantee about anything being > clean when we teardown, full stop. If you want to limit calling evict to > when all vmas are idle, fine, but that's just as lame. Not really. We don't block userspace just because the GPU is busy when the process closes it fd. -Chris
On Tue, Apr 08, 2014 at 09:11:39PM -0700, Ben Widawsky wrote: > On Tue, Apr 08, 2014 at 08:53:15AM +0200, Daniel Vetter wrote: > > On Mon, Apr 7, 2014 at 11:58 PM, Ben Widawsky <ben@bwidawsk.net> wrote: > > > Blocking important fixes for a test case is harmful to customers of our > > > software. I won't argue past that. If you won't take it as is, add it to the > > > JIRA task like you said. I'll carry this one around with my dynamic page table > > > allocations since you essentially can't do any real workloads with full PPGTT > > > without this (assuming you have signals). I'd venture to even say existing > > > tests can hit it with full PPGTT turned on. > > > > A duct-tape bugfix like this would be justified in late -rc, where the > > risk for disabling full ppgtt would probably outweight this hack. > > Earlier I'd opt of simply re-disbaling full ppgtt again if we can't > > come up with a properly understood fix and testcase for it in time. > > > > But atm full ppgtt is disabled, and we have a task-list with 10 > > subtasks on internal JIRA to knock down. Without that I wouldn't > > recommend anyone to try to ship full ppgtt in production, especially > > not our internal customers. > > > > I've updated the relevant subtask JIRA with details. > > -Daniel > > I felt confident when I wrote the original email it was hittable without > full PPGTT. I am not quite certain now. If that is not the case, I agree > with you. Yeah, all this is under the assumption that this only blows up with full ppgtt. Of course if we have an issue with normal operation as shipping in 3.15 then a small duct-tape patch until the real fix comes around does very much look like a legit approach. Especially if we'd have users scaling our walls already ;-) Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index 75fca63..91da738 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -225,10 +225,17 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle) i915_gem_retire_requests(vm->dev); } - list_for_each_entry_safe(vma, next, &vm->inactive_list, mm_list) + list_for_each_entry_safe(vma, next, &vm->inactive_list, mm_list) { + struct drm_i915_private *dev_priv = vm->dev->dev_private; + bool old_intr = dev_priv->mm.interruptible; + dev_priv->mm.interruptible = false; + if (vma->pin_count == 0) WARN_ON(i915_vma_unbind(vma)); + dev_priv->mm.interruptible = old_intr; + } + return 0; }
Our current code cannot handle a failure to evict well. You'll get at the very least the following splat, but usually a lot worse fallout after: [ 134.819441] ------------[ cut here ]------------ [ 134.819467] WARNING: CPU: 3 PID: 442 at drivers/gpu/drm/i915/i915_gem_evict.c:230 i915_gem_evict_vm+0x8a/0x1c0 [i915]() [ 134.819471] Modules linked in: i915 drm_kms_helper drm intel_gtt agpgart i2c_algo_bit ext4 crc16 mbcache jbd2 x86_pkg_temp_thermal coretemp kvm_intel kvm crc32c_intel ghash_clmulni_intel aesni_intel aes_x86_64 lrw iTCO_wdt gf128mul iTCO_vendor_support glue_helper ablk_helper cryptd microcode serio_raw i2c_i801 fan thermal battery e1000e acpi_cpufreq evdev ptp ac acpi_pad pps_core processor lpc_ich mfd_core snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_timer snd soundcore sd_mod crc_t10dif crct10dif_common ahci libahci libata ehci_pci ehci_hcd usbcore scsi_mod usb_common [ 134.819565] CPU: 3 PID: 442 Comm: glxgears Not tainted 3.14.0-BEN+ #480 [ 134.819568] Hardware name: Intel Corporation Broadwell Client platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0063.R01.1402110503 02/11/2014 [ 134.819571] 0000000000000009 ffff88009b10fa80 ffffffff8159e6a5 0000000000000000 [ 134.819577] ffff88009b10fab8 ffffffff8104895d ffff880145c353c0 ffff880145f400f8 [ 134.819584] 0000000000000000 ffff8800a274d300 ffff88009b10fb78 ffff88009b10fac8 [ 134.819590] Call Trace: [ 134.819599] [<ffffffff8159e6a5>] dump_stack+0x4e/0x7a [ 134.819607] [<ffffffff8104895d>] warn_slowpath_common+0x7d/0xa0 [ 134.819635] [<ffffffff81048a3a>] warn_slowpath_null+0x1a/0x20 [ 134.819656] [<ffffffffa050c82a>] i915_gem_evict_vm+0x8a/0x1c0 [i915] [ 134.819677] [<ffffffffa050a26b>] ppgtt_release+0x17b/0x1e0 [i915] [ 134.819693] [<ffffffffa050a34d>] i915_gem_context_free+0x7d/0x180 [i915] [ 134.819707] [<ffffffffa050a48c>] context_idr_cleanup+0x3c/0x40 [i915] [ 134.819715] [<ffffffff81332d14>] idr_for_each+0x104/0x1a0 [ 134.819730] [<ffffffffa050a450>] ? i915_gem_context_free+0x180/0x180 [i915] [ 134.819735] [<ffffffff815a27fc>] ? mutex_lock_nested+0x28c/0x3d0 [ 134.819761] [<ffffffffa057da75>] ? i915_driver_preclose+0x25/0x50 [i915] [ 134.819778] [<ffffffffa050b715>] i915_gem_context_close+0x35/0xa0 [i915] [ 134.819802] [<ffffffffa057da80>] i915_driver_preclose+0x30/0x50 [i915] [ 134.819816] [<ffffffffa03e6a7d>] drm_release+0x5d/0x5f0 [drm] [ 134.819822] [<ffffffff811aae3a>] __fput+0xea/0x240 [ 134.819827] [<ffffffff811aafde>] ____fput+0xe/0x10 [ 134.819832] [<ffffffff810701bc>] task_work_run+0xac/0xe0 [ 134.819837] [<ffffffff8104b82f>] do_exit+0x2cf/0xcf0 [ 134.819844] [<ffffffff815a5dac>] ? _raw_spin_unlock_irq+0x2c/0x60 [ 134.819849] [<ffffffff8104c2dc>] do_group_exit+0x4c/0xc0 [ 134.819855] [<ffffffff8105ef11>] get_signal_to_deliver+0x2d1/0x920 [ 134.819861] [<ffffffff81002408>] do_signal+0x48/0x620 [ 134.819867] [<ffffffff811aa0d9>] ? do_readv_writev+0x169/0x220 [ 134.819873] [<ffffffff8109e33d>] ? trace_hardirqs_on_caller+0xfd/0x1c0 [ 134.819879] [<ffffffff811c9b9d>] ? __fget_light+0x13d/0x160 [ 134.819886] [<ffffffff815a744c>] ? sysret_signal+0x5/0x47 [ 134.819892] [<ffffffff81002a45>] do_notify_resume+0x65/0x80 [ 134.819897] [<ffffffff815a76da>] int_signal+0x12/0x17 [ 134.819901] ---[ end trace dbf4da2122c3d683 ]--- At first I was going to call this a bandage to the problem. However, upon further thought, I rather like the idea of making evictions atomic, and less prone to failure anyway. The reason it can still somewhat be considered a band-aid however is GPU hangs. It would be nice if we had some way to interrupt the process when the GPU is hung. I'll leave it for a follow patch though. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem_evict.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)