* [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-08-11 18:20 ` Zhanjun Dong 0 siblings, 0 replies; 37+ messages in thread From: Zhanjun Dong @ 2023-08-11 18:20 UTC (permalink / raw) To: intel-gfx, dri-devel; +Cc: Daniel Vetter This attempts to avoid circular locking dependency between flush delayed work and intel_gt_reset. When intel_gt_reset was called, task will hold a lock. To cacel delayed work here, the _sync version will also acquire a lock, which might trigger the possible cirular locking dependency warning. When intel_gt_reset called, reset_in_progress flag will be set, add code to check the flag, call async verion if reset is in progress. Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com> Cc: John Harrison <John.C.Harrison@Intel.com> Cc: Andi Shyti <andi.shyti@linux.intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index a0e3ef1c65d2..600388c849f7 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) static void guc_cancel_busyness_worker(struct intel_guc *guc) { - cancel_delayed_work_sync(&guc->timestamp.work); + /* + * When intel_gt_reset was called, task will hold a lock. + * To cacel delayed work here, the _sync version will also acquire a lock, which might + * trigger the possible cirular locking dependency warning. + * Check the reset_in_progress flag, call async verion if reset is in progress. + */ + if (guc_to_gt(guc)->uc.reset_in_progress) + cancel_delayed_work(&guc->timestamp.work); + else + cancel_delayed_work_sync(&guc->timestamp.work); } static void __reset_guc_busyness_stats(struct intel_guc *guc) -- 2.34.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-08-11 18:20 ` Zhanjun Dong 0 siblings, 0 replies; 37+ messages in thread From: Zhanjun Dong @ 2023-08-11 18:20 UTC (permalink / raw) To: intel-gfx, dri-devel; +Cc: Zhanjun Dong, Andi Shyti, John Harrison This attempts to avoid circular locking dependency between flush delayed work and intel_gt_reset. When intel_gt_reset was called, task will hold a lock. To cacel delayed work here, the _sync version will also acquire a lock, which might trigger the possible cirular locking dependency warning. When intel_gt_reset called, reset_in_progress flag will be set, add code to check the flag, call async verion if reset is in progress. Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com> Cc: John Harrison <John.C.Harrison@Intel.com> Cc: Andi Shyti <andi.shyti@linux.intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index a0e3ef1c65d2..600388c849f7 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) static void guc_cancel_busyness_worker(struct intel_guc *guc) { - cancel_delayed_work_sync(&guc->timestamp.work); + /* + * When intel_gt_reset was called, task will hold a lock. + * To cacel delayed work here, the _sync version will also acquire a lock, which might + * trigger the possible cirular locking dependency warning. + * Check the reset_in_progress flag, call async verion if reset is in progress. + */ + if (guc_to_gt(guc)->uc.reset_in_progress) + cancel_delayed_work(&guc->timestamp.work); + else + cancel_delayed_work_sync(&guc->timestamp.work); } static void __reset_guc_busyness_stats(struct intel_guc *guc) -- 2.34.1 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Avoid circular locking dependency when flush delayed work on gt reset (rev5) 2023-08-11 18:20 ` Zhanjun Dong (?) @ 2023-08-11 19:15 ` Patchwork -1 siblings, 0 replies; 37+ messages in thread From: Patchwork @ 2023-08-11 19:15 UTC (permalink / raw) To: Zhanjun Dong; +Cc: intel-gfx == Series Details == Series: drm/i915: Avoid circular locking dependency when flush delayed work on gt reset (rev5) URL : https://patchwork.freedesktop.org/series/118898/ State : warning == Summary == Error: dim checkpatch failed /home/kbuild/linux/maintainer-tools/dim: line 50: /home/kbuild/.dimrc: No such file or directory ^ permalink raw reply [flat|nested] 37+ messages in thread
* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: Avoid circular locking dependency when flush delayed work on gt reset (rev5) 2023-08-11 18:20 ` Zhanjun Dong (?) (?) @ 2023-08-11 19:15 ` Patchwork -1 siblings, 0 replies; 37+ messages in thread From: Patchwork @ 2023-08-11 19:15 UTC (permalink / raw) To: Zhanjun Dong; +Cc: intel-gfx == Series Details == Series: drm/i915: Avoid circular locking dependency when flush delayed work on gt reset (rev5) URL : https://patchwork.freedesktop.org/series/118898/ State : warning == Summary == Error: dim sparse failed /home/kbuild/linux/maintainer-tools/dim: line 50: /home/kbuild/.dimrc: No such file or directory ^ permalink raw reply [flat|nested] 37+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Avoid circular locking dependency when flush delayed work on gt reset (rev5) 2023-08-11 18:20 ` Zhanjun Dong ` (2 preceding siblings ...) (?) @ 2023-08-11 19:34 ` Patchwork -1 siblings, 0 replies; 37+ messages in thread From: Patchwork @ 2023-08-11 19:34 UTC (permalink / raw) To: Zhanjun Dong; +Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 11646 bytes --] == Series Details == Series: drm/i915: Avoid circular locking dependency when flush delayed work on gt reset (rev5) URL : https://patchwork.freedesktop.org/series/118898/ State : failure == Summary == CI Bug Log - changes from CI_DRM_13510 -> Patchwork_118898v5 ==================================================== Summary ------- **FAILURE** Serious unknown changes coming with Patchwork_118898v5 absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_118898v5, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/index.html Participating hosts (43 -> 41) ------------------------------ Missing (2): bat-dg2-11 fi-snb-2520m Possible new issues ------------------- Here are the unknown changes that may have been introduced in Patchwork_118898v5: ### IGT changes ### #### Possible regressions #### * igt@gem_tiled_pread_basic: - fi-kbl-soraka: [PASS][1] -> [INCOMPLETE][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/fi-kbl-soraka/igt@gem_tiled_pread_basic.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/fi-kbl-soraka/igt@gem_tiled_pread_basic.html Known issues ------------ Here are the changes found in Patchwork_118898v5 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_lmem_swapping@parallel-random-engines: - bat-adln-1: NOTRUN -> [SKIP][3] ([i915#4613]) +3 similar issues [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adln-1/igt@gem_lmem_swapping@parallel-random-engines.html - bat-rplp-1: NOTRUN -> [SKIP][4] ([i915#4613]) +3 similar issues [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-rplp-1/igt@gem_lmem_swapping@parallel-random-engines.html * igt@gem_lmem_swapping@random-engines: - bat-adlp-6: NOTRUN -> [SKIP][5] ([i915#4613]) +3 similar issues [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adlp-6/igt@gem_lmem_swapping@random-engines.html * igt@i915_pm_rps@basic-api: - bat-adlp-6: NOTRUN -> [SKIP][6] ([i915#6621]) [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adlp-6/igt@i915_pm_rps@basic-api.html - bat-rplp-1: NOTRUN -> [SKIP][7] ([i915#6621]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-rplp-1/igt@i915_pm_rps@basic-api.html - bat-adln-1: NOTRUN -> [SKIP][8] ([i915#6621]) [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adln-1/igt@i915_pm_rps@basic-api.html * igt@i915_selftest@live@gt_lrc: - bat-adlp-9: [PASS][9] -> [INCOMPLETE][10] ([i915#4983] / [i915#7913]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/bat-adlp-9/igt@i915_selftest@live@gt_lrc.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adlp-9/igt@i915_selftest@live@gt_lrc.html * igt@i915_selftest@live@gt_pm: - bat-adln-1: NOTRUN -> [DMESG-FAIL][11] ([i915#4258]) [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adln-1/igt@i915_selftest@live@gt_pm.html * igt@i915_selftest@live@slpc: - bat-mtlp-6: [PASS][12] -> [DMESG-WARN][13] ([i915#6367]) [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/bat-mtlp-6/igt@i915_selftest@live@slpc.html [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-mtlp-6/igt@i915_selftest@live@slpc.html * igt@kms_chamelium_hpd@common-hpd-after-suspend: - bat-rplp-1: NOTRUN -> [SKIP][14] ([i915#7828]) [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-rplp-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html - bat-adln-1: NOTRUN -> [SKIP][15] ([i915#7828]) [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adln-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html - bat-adlp-6: NOTRUN -> [SKIP][16] ([i915#7828]) [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adlp-6/igt@kms_chamelium_hpd@common-hpd-after-suspend.html - fi-apl-guc: NOTRUN -> [SKIP][17] ([fdo#109271]) [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/fi-apl-guc/igt@kms_chamelium_hpd@common-hpd-after-suspend.html - bat-mtlp-8: NOTRUN -> [SKIP][18] ([i915#7828]) [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-mtlp-8/igt@kms_chamelium_hpd@common-hpd-after-suspend.html * igt@kms_setmode@basic-clone-single-crtc: - bat-adln-1: NOTRUN -> [SKIP][19] ([i915#3555]) [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adln-1/igt@kms_setmode@basic-clone-single-crtc.html - bat-adlp-6: NOTRUN -> [SKIP][20] ([i915#3555]) [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adlp-6/igt@kms_setmode@basic-clone-single-crtc.html * igt@prime_vgem@basic-read: - bat-rplp-1: NOTRUN -> [SKIP][21] ([i915#3708]) +2 similar issues [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-rplp-1/igt@prime_vgem@basic-read.html * igt@prime_vgem@basic-write: - bat-adln-1: NOTRUN -> [SKIP][22] ([fdo#109295] / [i915#3291]) +2 similar issues [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adln-1/igt@prime_vgem@basic-write.html - bat-adlp-6: NOTRUN -> [SKIP][23] ([fdo#109295] / [i915#3291] / [i915#3708]) +2 similar issues [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adlp-6/igt@prime_vgem@basic-write.html #### Possible fixes #### * igt@i915_pm_rpm@module-reload: - fi-apl-guc: [DMESG-FAIL][24] ([i915#8585]) -> [PASS][25] [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/fi-apl-guc/igt@i915_pm_rpm@module-reload.html [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/fi-apl-guc/igt@i915_pm_rpm@module-reload.html * igt@i915_selftest@live@gt_heartbeat: - fi-kbl-soraka: [DMESG-FAIL][26] ([i915#5334] / [i915#7872]) -> [PASS][27] [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html - fi-glk-j4005: [DMESG-FAIL][28] ([i915#5334]) -> [PASS][29] [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html * igt@i915_selftest@live@gt_mocs: - bat-mtlp-8: [DMESG-FAIL][30] ([i915#7059]) -> [PASS][31] [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/bat-mtlp-8/igt@i915_selftest@live@gt_mocs.html [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-mtlp-8/igt@i915_selftest@live@gt_mocs.html * igt@i915_selftest@live@requests: - bat-mtlp-6: [DMESG-FAIL][32] ([i915#8497]) -> [PASS][33] [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/bat-mtlp-6/igt@i915_selftest@live@requests.html [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-mtlp-6/igt@i915_selftest@live@requests.html * igt@i915_selftest@live@slpc: - bat-mtlp-8: [DMESG-WARN][34] ([i915#6367]) -> [PASS][35] [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/bat-mtlp-8/igt@i915_selftest@live@slpc.html [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-mtlp-8/igt@i915_selftest@live@slpc.html * igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-a-edp-1: - bat-adlp-6: [ABORT][36] ([i915#7977] / [i915#8469] / [i915#8668]) -> [PASS][37] [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/bat-adlp-6/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-a-edp-1.html [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adlp-6/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-a-edp-1.html * igt@kms_psr@sprite_plane_onoff: - bat-adln-1: [ABORT][38] ([i915#8442] / [i915#8668]) -> [PASS][39] [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/bat-adln-1/igt@kms_psr@sprite_plane_onoff.html [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-adln-1/igt@kms_psr@sprite_plane_onoff.html #### Warnings #### * igt@i915_suspend@basic-s3-without-i915: - bat-mtlp-8: [ABORT][40] ([i915#8802]) -> [SKIP][41] ([i915#6645]) [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/bat-mtlp-8/igt@i915_suspend@basic-s3-without-i915.html [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-mtlp-8/igt@i915_suspend@basic-s3-without-i915.html * igt@kms_setmode@basic-clone-single-crtc: - bat-rplp-1: [ABORT][42] ([i915#8260] / [i915#8668]) -> [SKIP][43] ([i915#3555]) [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13510/bat-rplp-1/igt@kms_setmode@basic-clone-single-crtc.html [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/bat-rplp-1/igt@kms_setmode@basic-clone-single-crtc.html [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295 [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291 [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555 [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708 [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258 [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613 [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983 [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334 [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367 [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621 [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645 [i915#7059]: https://gitlab.freedesktop.org/drm/intel/issues/7059 [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828 [i915#7872]: https://gitlab.freedesktop.org/drm/intel/issues/7872 [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913 [i915#7977]: https://gitlab.freedesktop.org/drm/intel/issues/7977 [i915#8260]: https://gitlab.freedesktop.org/drm/intel/issues/8260 [i915#8442]: https://gitlab.freedesktop.org/drm/intel/issues/8442 [i915#8469]: https://gitlab.freedesktop.org/drm/intel/issues/8469 [i915#8497]: https://gitlab.freedesktop.org/drm/intel/issues/8497 [i915#8585]: https://gitlab.freedesktop.org/drm/intel/issues/8585 [i915#8668]: https://gitlab.freedesktop.org/drm/intel/issues/8668 [i915#8802]: https://gitlab.freedesktop.org/drm/intel/issues/8802 Build changes ------------- * Linux: CI_DRM_13510 -> Patchwork_118898v5 CI-20190529: 20190529 CI_DRM_13510: 3e03d3e65440f4d60f19ff7b0313f3faf70de813 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_7432: 1b874bbe5e3576f89f5b6abf15a56917a96ec1ad @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_118898v5: 3e03d3e65440f4d60f19ff7b0313f3faf70de813 @ git://anongit.freedesktop.org/gfx-ci/linux ### Linux commits 6e6acbea108a drm/i915: Avoid circular locking dependency when flush delayed work on gt reset == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118898v5/index.html [-- Attachment #2: Type: text/html, Size: 13986 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-11 18:20 ` Zhanjun Dong @ 2023-08-21 14:09 ` Andi Shyti -1 siblings, 0 replies; 37+ messages in thread From: Andi Shyti @ 2023-08-21 14:09 UTC (permalink / raw) To: Zhanjun Dong; +Cc: Daniel Vetter, intel-gfx, dri-devel Hi Zhanjun, On Fri, Aug 11, 2023 at 11:20:11AM -0700, Zhanjun Dong wrote: > This attempts to avoid circular locking dependency between flush delayed > work and intel_gt_reset. > When intel_gt_reset was called, task will hold a lock. > To cacel delayed work here, the _sync version will also acquire a lock, /cacel/cancel > which might trigger the possible cirular locking dependency warning. > When intel_gt_reset called, reset_in_progress flag will be set, add code > to check the flag, call async verion if reset is in progress. /verion/version/ > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com> > Cc: John Harrison <John.C.Harrison@Intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- There is no changelog here :/ Can you please add the changelog after the '---' section? The commit log has changed and... > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index a0e3ef1c65d2..600388c849f7 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > { > - cancel_delayed_work_sync(&guc->timestamp.work); > + /* > + * When intel_gt_reset was called, task will hold a lock. > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > + * trigger the possible cirular locking dependency warning. > + * Check the reset_in_progress flag, call async verion if reset is in progress. > + */ > + if (guc_to_gt(guc)->uc.reset_in_progress) > + cancel_delayed_work(&guc->timestamp.work); > + else > + cancel_delayed_work_sync(&guc->timestamp.work); ... now you are checking out of reset_in_progress. Normally the convention here is to have the *_locked() version of the function. But I'm OK with this, as well... John, any opinion? Anyway, comparing with your previous patch the decision is made out of different elements and only __reset_guc_busyness_stats() needed this change. Andi > } > > static void __reset_guc_busyness_stats(struct intel_guc *guc) > -- > 2.34.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-08-21 14:09 ` Andi Shyti 0 siblings, 0 replies; 37+ messages in thread From: Andi Shyti @ 2023-08-21 14:09 UTC (permalink / raw) To: Zhanjun Dong; +Cc: intel-gfx, John Harrison, dri-devel, Andi Shyti Hi Zhanjun, On Fri, Aug 11, 2023 at 11:20:11AM -0700, Zhanjun Dong wrote: > This attempts to avoid circular locking dependency between flush delayed > work and intel_gt_reset. > When intel_gt_reset was called, task will hold a lock. > To cacel delayed work here, the _sync version will also acquire a lock, /cacel/cancel > which might trigger the possible cirular locking dependency warning. > When intel_gt_reset called, reset_in_progress flag will be set, add code > to check the flag, call async verion if reset is in progress. /verion/version/ > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com> > Cc: John Harrison <John.C.Harrison@Intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- There is no changelog here :/ Can you please add the changelog after the '---' section? The commit log has changed and... > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index a0e3ef1c65d2..600388c849f7 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > { > - cancel_delayed_work_sync(&guc->timestamp.work); > + /* > + * When intel_gt_reset was called, task will hold a lock. > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > + * trigger the possible cirular locking dependency warning. > + * Check the reset_in_progress flag, call async verion if reset is in progress. > + */ > + if (guc_to_gt(guc)->uc.reset_in_progress) > + cancel_delayed_work(&guc->timestamp.work); > + else > + cancel_delayed_work_sync(&guc->timestamp.work); ... now you are checking out of reset_in_progress. Normally the convention here is to have the *_locked() version of the function. But I'm OK with this, as well... John, any opinion? Anyway, comparing with your previous patch the decision is made out of different elements and only __reset_guc_busyness_stats() needed this change. Andi > } > > static void __reset_guc_busyness_stats(struct intel_guc *guc) > -- > 2.34.1 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-11 18:20 ` Zhanjun Dong @ 2023-08-22 13:50 ` Daniel Vetter -1 siblings, 0 replies; 37+ messages in thread From: Daniel Vetter @ 2023-08-22 13:50 UTC (permalink / raw) To: Zhanjun Dong; +Cc: Daniel Vetter, intel-gfx, dri-devel On Fri, Aug 11, 2023 at 11:20:11AM -0700, Zhanjun Dong wrote: > This attempts to avoid circular locking dependency between flush delayed > work and intel_gt_reset. > When intel_gt_reset was called, task will hold a lock. > To cacel delayed work here, the _sync version will also acquire a lock, > which might trigger the possible cirular locking dependency warning. > When intel_gt_reset called, reset_in_progress flag will be set, add code > to check the flag, call async verion if reset is in progress. > > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com> > Cc: John Harrison <John.C.Harrison@Intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index a0e3ef1c65d2..600388c849f7 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > { > - cancel_delayed_work_sync(&guc->timestamp.work); > + /* > + * When intel_gt_reset was called, task will hold a lock. > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > + * trigger the possible cirular locking dependency warning. This is not even close to a locking bugfix. Consider this a formal nack, because the issue here is not even close to "needs more comments to explain what's going on". -Daniel > + * Check the reset_in_progress flag, call async verion if reset is in progress. > + */ > + if (guc_to_gt(guc)->uc.reset_in_progress) > + cancel_delayed_work(&guc->timestamp.work); > + else > + cancel_delayed_work_sync(&guc->timestamp.work); > } > > static void __reset_guc_busyness_stats(struct intel_guc *guc) > -- > 2.34.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-08-22 13:50 ` Daniel Vetter 0 siblings, 0 replies; 37+ messages in thread From: Daniel Vetter @ 2023-08-22 13:50 UTC (permalink / raw) To: Zhanjun Dong; +Cc: intel-gfx, John Harrison, dri-devel, Andi Shyti On Fri, Aug 11, 2023 at 11:20:11AM -0700, Zhanjun Dong wrote: > This attempts to avoid circular locking dependency between flush delayed > work and intel_gt_reset. > When intel_gt_reset was called, task will hold a lock. > To cacel delayed work here, the _sync version will also acquire a lock, > which might trigger the possible cirular locking dependency warning. > When intel_gt_reset called, reset_in_progress flag will be set, add code > to check the flag, call async verion if reset is in progress. > > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com> > Cc: John Harrison <John.C.Harrison@Intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index a0e3ef1c65d2..600388c849f7 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > { > - cancel_delayed_work_sync(&guc->timestamp.work); > + /* > + * When intel_gt_reset was called, task will hold a lock. > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > + * trigger the possible cirular locking dependency warning. This is not even close to a locking bugfix. Consider this a formal nack, because the issue here is not even close to "needs more comments to explain what's going on". -Daniel > + * Check the reset_in_progress flag, call async verion if reset is in progress. > + */ > + if (guc_to_gt(guc)->uc.reset_in_progress) > + cancel_delayed_work(&guc->timestamp.work); > + else > + cancel_delayed_work_sync(&guc->timestamp.work); > } > > static void __reset_guc_busyness_stats(struct intel_guc *guc) > -- > 2.34.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-22 13:50 ` Daniel Vetter @ 2023-08-22 14:14 ` Dong, Zhanjun -1 siblings, 0 replies; 37+ messages in thread From: Dong, Zhanjun @ 2023-08-22 14:14 UTC (permalink / raw) To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org > -----Original Message----- > From: Daniel Vetter <daniel@ffwll.ch> > Sent: August 22, 2023 9:51 AM > To: Dong, Zhanjun <zhanjun.dong@intel.com> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Harrison, > John C <john.c.harrison@intel.com>; Andi Shyti <andi.shyti@linux.intel.com>; > Daniel Vetter <daniel@ffwll.ch> > Subject: Re: [PATCH v5] drm/i915: Avoid circular locking dependency when > flush delayed work on gt reset > > On Fri, Aug 11, 2023 at 11:20:11AM -0700, Zhanjun Dong wrote: > > This attempts to avoid circular locking dependency between flush delayed > > work and intel_gt_reset. > > When intel_gt_reset was called, task will hold a lock. > > To cacel delayed work here, the _sync version will also acquire a lock, > > which might trigger the possible cirular locking dependency warning. > > When intel_gt_reset called, reset_in_progress flag will be set, add code > > to check the flag, call async verion if reset is in progress. > > > > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com> > > Cc: John Harrison <John.C.Harrison@Intel.com> > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > --- > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > index a0e3ef1c65d2..600388c849f7 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct > intel_guc *guc) > > > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > > { > > - cancel_delayed_work_sync(&guc->timestamp.work); > > + /* > > + * When intel_gt_reset was called, task will hold a lock. > > + * To cacel delayed work here, the _sync version will also acquire a lock, > which might > > + * trigger the possible cirular locking dependency warning. > > This is not even close to a locking bugfix. Consider this a formal nack, > because the issue here is not even close to "needs more comments to > explain what's going on". > -Daniel The purpose of the comment here it is to explain locking issue condition > > > + * Check the reset_in_progress flag, call async verion if reset is in > progress. The comment here explains check with the flag to avoid locking condition. The reset process is not considered to be complete in short time, other than that, do we missed anything? > > + */ > > + if (guc_to_gt(guc)->uc.reset_in_progress) > > + cancel_delayed_work(&guc->timestamp.work); > > + else > > + cancel_delayed_work_sync(&guc->timestamp.work); > > } > > > > static void __reset_guc_busyness_stats(struct intel_guc *guc) > > -- > > 2.34.1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-08-22 14:14 ` Dong, Zhanjun 0 siblings, 0 replies; 37+ messages in thread From: Dong, Zhanjun @ 2023-08-22 14:14 UTC (permalink / raw) To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org, Harrison, John C, dri-devel@lists.freedesktop.org, Andi Shyti > -----Original Message----- > From: Daniel Vetter <daniel@ffwll.ch> > Sent: August 22, 2023 9:51 AM > To: Dong, Zhanjun <zhanjun.dong@intel.com> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Harrison, > John C <john.c.harrison@intel.com>; Andi Shyti <andi.shyti@linux.intel.com>; > Daniel Vetter <daniel@ffwll.ch> > Subject: Re: [PATCH v5] drm/i915: Avoid circular locking dependency when > flush delayed work on gt reset > > On Fri, Aug 11, 2023 at 11:20:11AM -0700, Zhanjun Dong wrote: > > This attempts to avoid circular locking dependency between flush delayed > > work and intel_gt_reset. > > When intel_gt_reset was called, task will hold a lock. > > To cacel delayed work here, the _sync version will also acquire a lock, > > which might trigger the possible cirular locking dependency warning. > > When intel_gt_reset called, reset_in_progress flag will be set, add code > > to check the flag, call async verion if reset is in progress. > > > > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com> > > Cc: John Harrison <John.C.Harrison@Intel.com> > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > --- > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > index a0e3ef1c65d2..600388c849f7 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct > intel_guc *guc) > > > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > > { > > - cancel_delayed_work_sync(&guc->timestamp.work); > > + /* > > + * When intel_gt_reset was called, task will hold a lock. > > + * To cacel delayed work here, the _sync version will also acquire a lock, > which might > > + * trigger the possible cirular locking dependency warning. > > This is not even close to a locking bugfix. Consider this a formal nack, > because the issue here is not even close to "needs more comments to > explain what's going on". > -Daniel The purpose of the comment here it is to explain locking issue condition > > > + * Check the reset_in_progress flag, call async verion if reset is in > progress. The comment here explains check with the flag to avoid locking condition. The reset process is not considered to be complete in short time, other than that, do we missed anything? > > + */ > > + if (guc_to_gt(guc)->uc.reset_in_progress) > > + cancel_delayed_work(&guc->timestamp.work); > > + else > > + cancel_delayed_work_sync(&guc->timestamp.work); > > } > > > > static void __reset_guc_busyness_stats(struct intel_guc *guc) > > -- > > 2.34.1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-22 14:14 ` Dong, Zhanjun @ 2023-08-22 14:28 ` Daniel Vetter -1 siblings, 0 replies; 37+ messages in thread From: Daniel Vetter @ 2023-08-22 14:28 UTC (permalink / raw) To: Dong, Zhanjun Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Daniel Vetter On Tue, Aug 22, 2023 at 02:14:28PM +0000, Dong, Zhanjun wrote: > > > > -----Original Message----- > > From: Daniel Vetter <daniel@ffwll.ch> > > Sent: August 22, 2023 9:51 AM > > To: Dong, Zhanjun <zhanjun.dong@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Harrison, > > John C <john.c.harrison@intel.com>; Andi Shyti <andi.shyti@linux.intel.com>; > > Daniel Vetter <daniel@ffwll.ch> > > Subject: Re: [PATCH v5] drm/i915: Avoid circular locking dependency when > > flush delayed work on gt reset > > > > On Fri, Aug 11, 2023 at 11:20:11AM -0700, Zhanjun Dong wrote: > > > This attempts to avoid circular locking dependency between flush delayed > > > work and intel_gt_reset. > > > When intel_gt_reset was called, task will hold a lock. > > > To cacel delayed work here, the _sync version will also acquire a lock, > > > which might trigger the possible cirular locking dependency warning. > > > When intel_gt_reset called, reset_in_progress flag will be set, add code > > > to check the flag, call async verion if reset is in progress. > > > > > > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com> > > > Cc: John Harrison <John.C.Harrison@Intel.com> > > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > --- > > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > index a0e3ef1c65d2..600388c849f7 100644 > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct > > intel_guc *guc) > > > > > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > > > { > > > - cancel_delayed_work_sync(&guc->timestamp.work); > > > + /* > > > + * When intel_gt_reset was called, task will hold a lock. > > > + * To cacel delayed work here, the _sync version will also acquire a lock, > > which might > > > + * trigger the possible cirular locking dependency warning. > > > > This is not even close to a locking bugfix. Consider this a formal nack, > > because the issue here is not even close to "needs more comments to > > explain what's going on". > > -Daniel > > The purpose of the comment here it is to explain locking issue condition > > > > > + * Check the reset_in_progress flag, call async verion if reset is in > > progress. > > > The comment here explains check with the flag to avoid locking condition. > The reset process is not considered to be complete in short time, other than that, do we missed anything? Either the _sync is not needed at all, in case you need to explain why. Which this patch doesn't. And if the _sync isn't needed, then it's probably not needed in all/most cases? Or the _sync is needed, and in that case you just replace a potential deadlock scenario with a potential race condition. In neither case should this patch here be merged. -Daniel > > > > + */ > > > + if (guc_to_gt(guc)->uc.reset_in_progress) > > > + cancel_delayed_work(&guc->timestamp.work); > > > + else > > > + cancel_delayed_work_sync(&guc->timestamp.work); > > > } > > > > > > static void __reset_guc_busyness_stats(struct intel_guc *guc) > > > -- > > > 2.34.1 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-08-22 14:28 ` Daniel Vetter 0 siblings, 0 replies; 37+ messages in thread From: Daniel Vetter @ 2023-08-22 14:28 UTC (permalink / raw) To: Dong, Zhanjun Cc: Andi Shyti, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Harrison, John C On Tue, Aug 22, 2023 at 02:14:28PM +0000, Dong, Zhanjun wrote: > > > > -----Original Message----- > > From: Daniel Vetter <daniel@ffwll.ch> > > Sent: August 22, 2023 9:51 AM > > To: Dong, Zhanjun <zhanjun.dong@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Harrison, > > John C <john.c.harrison@intel.com>; Andi Shyti <andi.shyti@linux.intel.com>; > > Daniel Vetter <daniel@ffwll.ch> > > Subject: Re: [PATCH v5] drm/i915: Avoid circular locking dependency when > > flush delayed work on gt reset > > > > On Fri, Aug 11, 2023 at 11:20:11AM -0700, Zhanjun Dong wrote: > > > This attempts to avoid circular locking dependency between flush delayed > > > work and intel_gt_reset. > > > When intel_gt_reset was called, task will hold a lock. > > > To cacel delayed work here, the _sync version will also acquire a lock, > > > which might trigger the possible cirular locking dependency warning. > > > When intel_gt_reset called, reset_in_progress flag will be set, add code > > > to check the flag, call async verion if reset is in progress. > > > > > > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com> > > > Cc: John Harrison <John.C.Harrison@Intel.com> > > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > --- > > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > index a0e3ef1c65d2..600388c849f7 100644 > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct > > intel_guc *guc) > > > > > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > > > { > > > - cancel_delayed_work_sync(&guc->timestamp.work); > > > + /* > > > + * When intel_gt_reset was called, task will hold a lock. > > > + * To cacel delayed work here, the _sync version will also acquire a lock, > > which might > > > + * trigger the possible cirular locking dependency warning. > > > > This is not even close to a locking bugfix. Consider this a formal nack, > > because the issue here is not even close to "needs more comments to > > explain what's going on". > > -Daniel > > The purpose of the comment here it is to explain locking issue condition > > > > > + * Check the reset_in_progress flag, call async verion if reset is in > > progress. > > > The comment here explains check with the flag to avoid locking condition. > The reset process is not considered to be complete in short time, other than that, do we missed anything? Either the _sync is not needed at all, in case you need to explain why. Which this patch doesn't. And if the _sync isn't needed, then it's probably not needed in all/most cases? Or the _sync is needed, and in that case you just replace a potential deadlock scenario with a potential race condition. In neither case should this patch here be merged. -Daniel > > > > + */ > > > + if (guc_to_gt(guc)->uc.reset_in_progress) > > > + cancel_delayed_work(&guc->timestamp.work); > > > + else > > > + cancel_delayed_work_sync(&guc->timestamp.work); > > > } > > > > > > static void __reset_guc_busyness_stats(struct intel_guc *guc) > > > -- > > > 2.34.1 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-11 18:20 ` Zhanjun Dong @ 2023-08-22 18:53 ` John Harrison -1 siblings, 0 replies; 37+ messages in thread From: John Harrison @ 2023-08-22 18:53 UTC (permalink / raw) To: Zhanjun Dong, intel-gfx, dri-devel; +Cc: Daniel Vetter [-- Attachment #1: Type: text/plain, Size: 3070 bytes --] On 8/11/2023 11:20, Zhanjun Dong wrote: > This attempts to avoid circular locking dependency between flush delayed > work and intel_gt_reset. > When intel_gt_reset was called, task will hold a lock. > To cacel delayed work here, the _sync version will also acquire a lock, > which might trigger the possible cirular locking dependency warning. > When intel_gt_reset called, reset_in_progress flag will be set, add code > to check the flag, call async verion if reset is in progress. > > Signed-off-by: Zhanjun Dong<zhanjun.dong@intel.com> > Cc: John Harrison<John.C.Harrison@Intel.com> > Cc: Andi Shyti<andi.shyti@linux.intel.com> > Cc: Daniel Vetter<daniel@ffwll.ch> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index a0e3ef1c65d2..600388c849f7 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > { > - cancel_delayed_work_sync(&guc->timestamp.work); > + /* > + * When intel_gt_reset was called, task will hold a lock. > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > + * trigger the possible cirular locking dependency warning. > + * Check the reset_in_progress flag, call async verion if reset is in progress. > + */ This needs to explain in much more detail what is going on and why it is not a problem. E.g.: The busyness worker needs to be cancelled. In general that means using the synchronous cancel version to ensure that an in-progress worker will not keep executing beyond whatever is happening that needs the cancel. E.g. suspend, driver unload, etc. However, in the case of a reset, the synchronous version is not required and can trigger a false deadlock detection warning. The business worker takes the reset mutex to protect against resets interfering with it. However, it does a trylock and bails out if the reset lock is already acquired. Thus there is no actual deadlock or other concern with the worker running concurrently with a reset. So an asynchronous cancel is safe in the case of a reset rather than a driver unload or suspend type operation. On the other hand, if the cancel_sync version is used when a reset is in progress then the mutex deadlock detection sees the mutex being acquired through multiple paths and complains. So just don't bother. That keeps the detection code happy and is safe because of the trylock code described above. John. > + if (guc_to_gt(guc)->uc.reset_in_progress) > + cancel_delayed_work(&guc->timestamp.work); > + else > + cancel_delayed_work_sync(&guc->timestamp.work); > } > > static void __reset_guc_busyness_stats(struct intel_guc *guc) [-- Attachment #2: Type: text/html, Size: 3912 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-08-22 18:53 ` John Harrison 0 siblings, 0 replies; 37+ messages in thread From: John Harrison @ 2023-08-22 18:53 UTC (permalink / raw) To: Zhanjun Dong, intel-gfx, dri-devel; +Cc: Andi Shyti [-- Attachment #1: Type: text/plain, Size: 3070 bytes --] On 8/11/2023 11:20, Zhanjun Dong wrote: > This attempts to avoid circular locking dependency between flush delayed > work and intel_gt_reset. > When intel_gt_reset was called, task will hold a lock. > To cacel delayed work here, the _sync version will also acquire a lock, > which might trigger the possible cirular locking dependency warning. > When intel_gt_reset called, reset_in_progress flag will be set, add code > to check the flag, call async verion if reset is in progress. > > Signed-off-by: Zhanjun Dong<zhanjun.dong@intel.com> > Cc: John Harrison<John.C.Harrison@Intel.com> > Cc: Andi Shyti<andi.shyti@linux.intel.com> > Cc: Daniel Vetter<daniel@ffwll.ch> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index a0e3ef1c65d2..600388c849f7 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > { > - cancel_delayed_work_sync(&guc->timestamp.work); > + /* > + * When intel_gt_reset was called, task will hold a lock. > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > + * trigger the possible cirular locking dependency warning. > + * Check the reset_in_progress flag, call async verion if reset is in progress. > + */ This needs to explain in much more detail what is going on and why it is not a problem. E.g.: The busyness worker needs to be cancelled. In general that means using the synchronous cancel version to ensure that an in-progress worker will not keep executing beyond whatever is happening that needs the cancel. E.g. suspend, driver unload, etc. However, in the case of a reset, the synchronous version is not required and can trigger a false deadlock detection warning. The business worker takes the reset mutex to protect against resets interfering with it. However, it does a trylock and bails out if the reset lock is already acquired. Thus there is no actual deadlock or other concern with the worker running concurrently with a reset. So an asynchronous cancel is safe in the case of a reset rather than a driver unload or suspend type operation. On the other hand, if the cancel_sync version is used when a reset is in progress then the mutex deadlock detection sees the mutex being acquired through multiple paths and complains. So just don't bother. That keeps the detection code happy and is safe because of the trylock code described above. John. > + if (guc_to_gt(guc)->uc.reset_in_progress) > + cancel_delayed_work(&guc->timestamp.work); > + else > + cancel_delayed_work_sync(&guc->timestamp.work); > } > > static void __reset_guc_busyness_stats(struct intel_guc *guc) [-- Attachment #2: Type: text/html, Size: 3912 bytes --] ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-22 18:53 ` John Harrison @ 2023-08-23 16:00 ` Daniel Vetter -1 siblings, 0 replies; 37+ messages in thread From: Daniel Vetter @ 2023-08-23 16:00 UTC (permalink / raw) To: John Harrison; +Cc: Daniel Vetter, intel-gfx, dri-devel On Tue, Aug 22, 2023 at 11:53:24AM -0700, John Harrison wrote: > On 8/11/2023 11:20, Zhanjun Dong wrote: > > This attempts to avoid circular locking dependency between flush delayed > > work and intel_gt_reset. > > When intel_gt_reset was called, task will hold a lock. > > To cacel delayed work here, the _sync version will also acquire a lock, > > which might trigger the possible cirular locking dependency warning. > > When intel_gt_reset called, reset_in_progress flag will be set, add code > > to check the flag, call async verion if reset is in progress. > > > > Signed-off-by: Zhanjun Dong<zhanjun.dong@intel.com> > > Cc: John Harrison<John.C.Harrison@Intel.com> > > Cc: Andi Shyti<andi.shyti@linux.intel.com> > > Cc: Daniel Vetter<daniel@ffwll.ch> > > --- > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > index a0e3ef1c65d2..600388c849f7 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > > { > > - cancel_delayed_work_sync(&guc->timestamp.work); > > + /* > > + * When intel_gt_reset was called, task will hold a lock. > > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > > + * trigger the possible cirular locking dependency warning. > > + * Check the reset_in_progress flag, call async verion if reset is in progress. > > + */ > This needs to explain in much more detail what is going on and why it is not > a problem. E.g.: > > The busyness worker needs to be cancelled. In general that means > using the synchronous cancel version to ensure that an in-progress > worker will not keep executing beyond whatever is happening that > needs the cancel. E.g. suspend, driver unload, etc. However, in the > case of a reset, the synchronous version is not required and can > trigger a false deadlock detection warning. > > The business worker takes the reset mutex to protect against resets > interfering with it. However, it does a trylock and bails out if the > reset lock is already acquired. Thus there is no actual deadlock or > other concern with the worker running concurrently with a reset. So > an asynchronous cancel is safe in the case of a reset rather than a > driver unload or suspend type operation. On the other hand, if the > cancel_sync version is used when a reset is in progress then the > mutex deadlock detection sees the mutex being acquired through > multiple paths and complains. > > So just don't bother. That keeps the detection code happy and is > safe because of the trylock code described above. So why do we even need to cancel anything if it doesn't do anything while the reset is in progress? Just remove the cancel from the reset path as uneeded instead, and explain why that's ok? Because that's defacto what the cancel_work with a potential deadlock scenario for cancel_work_sync does, you either don't need it at all, or the replacement creates a bug. -Daniel > > > John. > > > > + if (guc_to_gt(guc)->uc.reset_in_progress) > > + cancel_delayed_work(&guc->timestamp.work); > > + else > > + cancel_delayed_work_sync(&guc->timestamp.work); > > } > > static void __reset_guc_busyness_stats(struct intel_guc *guc) -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-08-23 16:00 ` Daniel Vetter 0 siblings, 0 replies; 37+ messages in thread From: Daniel Vetter @ 2023-08-23 16:00 UTC (permalink / raw) To: John Harrison; +Cc: Zhanjun Dong, intel-gfx, Andi Shyti, dri-devel On Tue, Aug 22, 2023 at 11:53:24AM -0700, John Harrison wrote: > On 8/11/2023 11:20, Zhanjun Dong wrote: > > This attempts to avoid circular locking dependency between flush delayed > > work and intel_gt_reset. > > When intel_gt_reset was called, task will hold a lock. > > To cacel delayed work here, the _sync version will also acquire a lock, > > which might trigger the possible cirular locking dependency warning. > > When intel_gt_reset called, reset_in_progress flag will be set, add code > > to check the flag, call async verion if reset is in progress. > > > > Signed-off-by: Zhanjun Dong<zhanjun.dong@intel.com> > > Cc: John Harrison<John.C.Harrison@Intel.com> > > Cc: Andi Shyti<andi.shyti@linux.intel.com> > > Cc: Daniel Vetter<daniel@ffwll.ch> > > --- > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > index a0e3ef1c65d2..600388c849f7 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > > { > > - cancel_delayed_work_sync(&guc->timestamp.work); > > + /* > > + * When intel_gt_reset was called, task will hold a lock. > > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > > + * trigger the possible cirular locking dependency warning. > > + * Check the reset_in_progress flag, call async verion if reset is in progress. > > + */ > This needs to explain in much more detail what is going on and why it is not > a problem. E.g.: > > The busyness worker needs to be cancelled. In general that means > using the synchronous cancel version to ensure that an in-progress > worker will not keep executing beyond whatever is happening that > needs the cancel. E.g. suspend, driver unload, etc. However, in the > case of a reset, the synchronous version is not required and can > trigger a false deadlock detection warning. > > The business worker takes the reset mutex to protect against resets > interfering with it. However, it does a trylock and bails out if the > reset lock is already acquired. Thus there is no actual deadlock or > other concern with the worker running concurrently with a reset. So > an asynchronous cancel is safe in the case of a reset rather than a > driver unload or suspend type operation. On the other hand, if the > cancel_sync version is used when a reset is in progress then the > mutex deadlock detection sees the mutex being acquired through > multiple paths and complains. > > So just don't bother. That keeps the detection code happy and is > safe because of the trylock code described above. So why do we even need to cancel anything if it doesn't do anything while the reset is in progress? Just remove the cancel from the reset path as uneeded instead, and explain why that's ok? Because that's defacto what the cancel_work with a potential deadlock scenario for cancel_work_sync does, you either don't need it at all, or the replacement creates a bug. -Daniel > > > John. > > > > + if (guc_to_gt(guc)->uc.reset_in_progress) > > + cancel_delayed_work(&guc->timestamp.work); > > + else > > + cancel_delayed_work_sync(&guc->timestamp.work); > > } > > static void __reset_guc_busyness_stats(struct intel_guc *guc) -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-23 16:00 ` Daniel Vetter @ 2023-08-23 17:37 ` John Harrison -1 siblings, 0 replies; 37+ messages in thread From: John Harrison @ 2023-08-23 17:37 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, dri-devel On 8/23/2023 09:00, Daniel Vetter wrote: > On Tue, Aug 22, 2023 at 11:53:24AM -0700, John Harrison wrote: >> On 8/11/2023 11:20, Zhanjun Dong wrote: >>> This attempts to avoid circular locking dependency between flush delayed >>> work and intel_gt_reset. >>> When intel_gt_reset was called, task will hold a lock. >>> To cacel delayed work here, the _sync version will also acquire a lock, >>> which might trigger the possible cirular locking dependency warning. >>> When intel_gt_reset called, reset_in_progress flag will be set, add code >>> to check the flag, call async verion if reset is in progress. >>> >>> Signed-off-by: Zhanjun Dong<zhanjun.dong@intel.com> >>> Cc: John Harrison<John.C.Harrison@Intel.com> >>> Cc: Andi Shyti<andi.shyti@linux.intel.com> >>> Cc: Daniel Vetter<daniel@ffwll.ch> >>> --- >>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- >>> 1 file changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>> index a0e3ef1c65d2..600388c849f7 100644 >>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>> @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) >>> static void guc_cancel_busyness_worker(struct intel_guc *guc) >>> { >>> - cancel_delayed_work_sync(&guc->timestamp.work); >>> + /* >>> + * When intel_gt_reset was called, task will hold a lock. >>> + * To cacel delayed work here, the _sync version will also acquire a lock, which might >>> + * trigger the possible cirular locking dependency warning. >>> + * Check the reset_in_progress flag, call async verion if reset is in progress. >>> + */ >> This needs to explain in much more detail what is going on and why it is not >> a problem. E.g.: >> >> The busyness worker needs to be cancelled. In general that means >> using the synchronous cancel version to ensure that an in-progress >> worker will not keep executing beyond whatever is happening that >> needs the cancel. E.g. suspend, driver unload, etc. However, in the >> case of a reset, the synchronous version is not required and can >> trigger a false deadlock detection warning. >> >> The business worker takes the reset mutex to protect against resets >> interfering with it. However, it does a trylock and bails out if the >> reset lock is already acquired. Thus there is no actual deadlock or >> other concern with the worker running concurrently with a reset. So >> an asynchronous cancel is safe in the case of a reset rather than a >> driver unload or suspend type operation. On the other hand, if the >> cancel_sync version is used when a reset is in progress then the >> mutex deadlock detection sees the mutex being acquired through >> multiple paths and complains. >> >> So just don't bother. That keeps the detection code happy and is >> safe because of the trylock code described above. > So why do we even need to cancel anything if it doesn't do anything while > the reset is in progress? It still needs to be cancelled. The worker only aborts if it is actively executing concurrently with the reset. It might not start to execute until after the reset has completed. And there is presumably a reason why the cancel is being called, a reason not necessarily related to resets at all. Leaving the worker to run arbitrarily after the driver is expecting it to be stopped will lead to much worse things than a fake lockdep splat, e.g. a use after free pointer deref. John. > > Just remove the cancel from the reset path as uneeded instead, and explain > why that's ok? Because that's defacto what the cancel_work with a > potential deadlock scenario for cancel_work_sync does, you either don't > need it at all, or the replacement creates a bug. > -Daniel > >> >> John. >> >> >>> + if (guc_to_gt(guc)->uc.reset_in_progress) >>> + cancel_delayed_work(&guc->timestamp.work); >>> + else >>> + cancel_delayed_work_sync(&guc->timestamp.work); >>> } >>> static void __reset_guc_busyness_stats(struct intel_guc *guc) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-08-23 17:37 ` John Harrison 0 siblings, 0 replies; 37+ messages in thread From: John Harrison @ 2023-08-23 17:37 UTC (permalink / raw) To: Daniel Vetter; +Cc: Zhanjun Dong, intel-gfx, Andi Shyti, dri-devel On 8/23/2023 09:00, Daniel Vetter wrote: > On Tue, Aug 22, 2023 at 11:53:24AM -0700, John Harrison wrote: >> On 8/11/2023 11:20, Zhanjun Dong wrote: >>> This attempts to avoid circular locking dependency between flush delayed >>> work and intel_gt_reset. >>> When intel_gt_reset was called, task will hold a lock. >>> To cacel delayed work here, the _sync version will also acquire a lock, >>> which might trigger the possible cirular locking dependency warning. >>> When intel_gt_reset called, reset_in_progress flag will be set, add code >>> to check the flag, call async verion if reset is in progress. >>> >>> Signed-off-by: Zhanjun Dong<zhanjun.dong@intel.com> >>> Cc: John Harrison<John.C.Harrison@Intel.com> >>> Cc: Andi Shyti<andi.shyti@linux.intel.com> >>> Cc: Daniel Vetter<daniel@ffwll.ch> >>> --- >>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- >>> 1 file changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>> index a0e3ef1c65d2..600388c849f7 100644 >>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>> @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) >>> static void guc_cancel_busyness_worker(struct intel_guc *guc) >>> { >>> - cancel_delayed_work_sync(&guc->timestamp.work); >>> + /* >>> + * When intel_gt_reset was called, task will hold a lock. >>> + * To cacel delayed work here, the _sync version will also acquire a lock, which might >>> + * trigger the possible cirular locking dependency warning. >>> + * Check the reset_in_progress flag, call async verion if reset is in progress. >>> + */ >> This needs to explain in much more detail what is going on and why it is not >> a problem. E.g.: >> >> The busyness worker needs to be cancelled. In general that means >> using the synchronous cancel version to ensure that an in-progress >> worker will not keep executing beyond whatever is happening that >> needs the cancel. E.g. suspend, driver unload, etc. However, in the >> case of a reset, the synchronous version is not required and can >> trigger a false deadlock detection warning. >> >> The business worker takes the reset mutex to protect against resets >> interfering with it. However, it does a trylock and bails out if the >> reset lock is already acquired. Thus there is no actual deadlock or >> other concern with the worker running concurrently with a reset. So >> an asynchronous cancel is safe in the case of a reset rather than a >> driver unload or suspend type operation. On the other hand, if the >> cancel_sync version is used when a reset is in progress then the >> mutex deadlock detection sees the mutex being acquired through >> multiple paths and complains. >> >> So just don't bother. That keeps the detection code happy and is >> safe because of the trylock code described above. > So why do we even need to cancel anything if it doesn't do anything while > the reset is in progress? It still needs to be cancelled. The worker only aborts if it is actively executing concurrently with the reset. It might not start to execute until after the reset has completed. And there is presumably a reason why the cancel is being called, a reason not necessarily related to resets at all. Leaving the worker to run arbitrarily after the driver is expecting it to be stopped will lead to much worse things than a fake lockdep splat, e.g. a use after free pointer deref. John. > > Just remove the cancel from the reset path as uneeded instead, and explain > why that's ok? Because that's defacto what the cancel_work with a > potential deadlock scenario for cancel_work_sync does, you either don't > need it at all, or the replacement creates a bug. > -Daniel > >> >> John. >> >> >>> + if (guc_to_gt(guc)->uc.reset_in_progress) >>> + cancel_delayed_work(&guc->timestamp.work); >>> + else >>> + cancel_delayed_work_sync(&guc->timestamp.work); >>> } >>> static void __reset_guc_busyness_stats(struct intel_guc *guc) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-23 17:37 ` John Harrison (?) @ 2023-08-28 23:01 ` John Harrison 2023-09-06 6:50 ` Daniel Vetter -1 siblings, 1 reply; 37+ messages in thread From: John Harrison @ 2023-08-28 23:01 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, dri-devel On 8/23/2023 10:37, John Harrison wrote: > On 8/23/2023 09:00, Daniel Vetter wrote: >> On Tue, Aug 22, 2023 at 11:53:24AM -0700, John Harrison wrote: >>> On 8/11/2023 11:20, Zhanjun Dong wrote: >>>> This attempts to avoid circular locking dependency between flush >>>> delayed >>>> work and intel_gt_reset. >>>> When intel_gt_reset was called, task will hold a lock. >>>> To cacel delayed work here, the _sync version will also acquire a >>>> lock, >>>> which might trigger the possible cirular locking dependency warning. >>>> When intel_gt_reset called, reset_in_progress flag will be set, add >>>> code >>>> to check the flag, call async verion if reset is in progress. >>>> >>>> Signed-off-by: Zhanjun Dong<zhanjun.dong@intel.com> >>>> Cc: John Harrison<John.C.Harrison@Intel.com> >>>> Cc: Andi Shyti<andi.shyti@linux.intel.com> >>>> Cc: Daniel Vetter<daniel@ffwll.ch> >>>> --- >>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- >>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>> index a0e3ef1c65d2..600388c849f7 100644 >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>> @@ -1359,7 +1359,16 @@ static void >>>> guc_enable_busyness_worker(struct intel_guc *guc) >>>> static void guc_cancel_busyness_worker(struct intel_guc *guc) >>>> { >>>> - cancel_delayed_work_sync(&guc->timestamp.work); >>>> + /* >>>> + * When intel_gt_reset was called, task will hold a lock. >>>> + * To cacel delayed work here, the _sync version will also >>>> acquire a lock, which might >>>> + * trigger the possible cirular locking dependency warning. >>>> + * Check the reset_in_progress flag, call async verion if >>>> reset is in progress. >>>> + */ >>> This needs to explain in much more detail what is going on and why >>> it is not >>> a problem. E.g.: >>> >>> The busyness worker needs to be cancelled. In general that means >>> using the synchronous cancel version to ensure that an in-progress >>> worker will not keep executing beyond whatever is happening that >>> needs the cancel. E.g. suspend, driver unload, etc. However, in the >>> case of a reset, the synchronous version is not required and can >>> trigger a false deadlock detection warning. >>> >>> The business worker takes the reset mutex to protect against resets >>> interfering with it. However, it does a trylock and bails out if >>> the >>> reset lock is already acquired. Thus there is no actual deadlock or >>> other concern with the worker running concurrently with a reset. So >>> an asynchronous cancel is safe in the case of a reset rather than a >>> driver unload or suspend type operation. On the other hand, if the >>> cancel_sync version is used when a reset is in progress then the >>> mutex deadlock detection sees the mutex being acquired through >>> multiple paths and complains. >>> >>> So just don't bother. That keeps the detection code happy and is >>> safe because of the trylock code described above. >> So why do we even need to cancel anything if it doesn't do anything >> while >> the reset is in progress? > It still needs to be cancelled. The worker only aborts if it is > actively executing concurrently with the reset. It might not start to > execute until after the reset has completed. And there is presumably a > reason why the cancel is being called, a reason not necessarily > related to resets at all. Leaving the worker to run arbitrarily after > the driver is expecting it to be stopped will lead to much worse > things than a fake lockdep splat, e.g. a use after free pointer deref. > > John. @Daniel Vetter - ping? Is this explanation sufficient? Are you okay with this change now? John. > >> >> Just remove the cancel from the reset path as uneeded instead, and >> explain >> why that's ok? Because that's defacto what the cancel_work with a >> potential deadlock scenario for cancel_work_sync does, you either don't >> need it at all, or the replacement creates a bug. >> -Daniel >> >>> >>> John. >>> >>> >>>> + if (guc_to_gt(guc)->uc.reset_in_progress) >>>> + cancel_delayed_work(&guc->timestamp.work); >>>> + else >>>> + cancel_delayed_work_sync(&guc->timestamp.work); >>>> } >>>> static void __reset_guc_busyness_stats(struct intel_guc *guc) > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-28 23:01 ` [Intel-gfx] " John Harrison @ 2023-09-06 6:50 ` Daniel Vetter 0 siblings, 0 replies; 37+ messages in thread From: Daniel Vetter @ 2023-09-06 6:50 UTC (permalink / raw) To: John Harrison; +Cc: intel-gfx, dri-devel, Daniel Vetter On Mon, Aug 28, 2023 at 04:01:38PM -0700, John Harrison wrote: > On 8/23/2023 10:37, John Harrison wrote: > > On 8/23/2023 09:00, Daniel Vetter wrote: > > > On Tue, Aug 22, 2023 at 11:53:24AM -0700, John Harrison wrote: > > > > On 8/11/2023 11:20, Zhanjun Dong wrote: > > > > > This attempts to avoid circular locking dependency between > > > > > flush delayed > > > > > work and intel_gt_reset. > > > > > When intel_gt_reset was called, task will hold a lock. > > > > > To cacel delayed work here, the _sync version will also > > > > > acquire a lock, > > > > > which might trigger the possible cirular locking dependency warning. > > > > > When intel_gt_reset called, reset_in_progress flag will be > > > > > set, add code > > > > > to check the flag, call async verion if reset is in progress. > > > > > > > > > > Signed-off-by: Zhanjun Dong<zhanjun.dong@intel.com> > > > > > Cc: John Harrison<John.C.Harrison@Intel.com> > > > > > Cc: Andi Shyti<andi.shyti@linux.intel.com> > > > > > Cc: Daniel Vetter<daniel@ffwll.ch> > > > > > --- > > > > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git > > > > > a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > > index a0e3ef1c65d2..600388c849f7 100644 > > > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > > @@ -1359,7 +1359,16 @@ static void > > > > > guc_enable_busyness_worker(struct intel_guc *guc) > > > > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > > > > > { > > > > > - cancel_delayed_work_sync(&guc->timestamp.work); > > > > > + /* > > > > > + * When intel_gt_reset was called, task will hold a lock. > > > > > + * To cacel delayed work here, the _sync version will > > > > > also acquire a lock, which might > > > > > + * trigger the possible cirular locking dependency warning. > > > > > + * Check the reset_in_progress flag, call async verion > > > > > if reset is in progress. > > > > > + */ > > > > This needs to explain in much more detail what is going on and > > > > why it is not > > > > a problem. E.g.: > > > > > > > > The busyness worker needs to be cancelled. In general that means > > > > using the synchronous cancel version to ensure that an in-progress > > > > worker will not keep executing beyond whatever is happening that > > > > needs the cancel. E.g. suspend, driver unload, etc. However, in the > > > > case of a reset, the synchronous version is not required and can > > > > trigger a false deadlock detection warning. > > > > > > > > The business worker takes the reset mutex to protect against resets > > > > interfering with it. However, it does a trylock and bails > > > > out if the > > > > reset lock is already acquired. Thus there is no actual deadlock or > > > > other concern with the worker running concurrently with a reset. So > > > > an asynchronous cancel is safe in the case of a reset rather than a > > > > driver unload or suspend type operation. On the other hand, if the > > > > cancel_sync version is used when a reset is in progress then the > > > > mutex deadlock detection sees the mutex being acquired through > > > > multiple paths and complains. > > > > > > > > So just don't bother. That keeps the detection code happy and is > > > > safe because of the trylock code described above. > > > So why do we even need to cancel anything if it doesn't do anything > > > while > > > the reset is in progress? > > It still needs to be cancelled. The worker only aborts if it is actively > > executing concurrently with the reset. It might not start to execute > > until after the reset has completed. And there is presumably a reason > > why the cancel is being called, a reason not necessarily related to > > resets at all. Leaving the worker to run arbitrarily after the driver is > > expecting it to be stopped will lead to much worse things than a fake > > lockdep splat, e.g. a use after free pointer deref. > > > > John. > @Daniel Vetter - ping? Is this explanation sufficient? Are you okay with > this change now? Sorry for the late reply, I'm constantly behind on mails :-/ Ping me on irc next time around if I don't reply, that's quicker. "presumably" isn't good enough for locking design. Either you know, and can prove it all, or you shouldn't touch the code and its locking design before you've figured this out. Again, either this is a deadlock, race condition, or the cancel isn't necessary. And this argument works in full generality. All this patch does it replace the dealock with one of the other two, and that's not good enough if you don't even know which one it is. - if you need the cancel, you have a race condition - if you don't have a race condition, you don't need the cancel - currently you have the deadlock "presumably" and "maybe" aint enoug for locking design. Cheers, Daniel > > John. > > > > > > > > > Just remove the cancel from the reset path as uneeded instead, and > > > explain > > > why that's ok? Because that's defacto what the cancel_work with a > > > potential deadlock scenario for cancel_work_sync does, you either don't > > > need it at all, or the replacement creates a bug. > > > -Daniel > > > > > > > > > > > John. > > > > > > > > > > > > > + if (guc_to_gt(guc)->uc.reset_in_progress) > > > > > + cancel_delayed_work(&guc->timestamp.work); > > > > > + else > > > > > + cancel_delayed_work_sync(&guc->timestamp.work); > > > > > } > > > > > static void __reset_guc_busyness_stats(struct intel_guc *guc) > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-09-06 6:50 ` Daniel Vetter 0 siblings, 0 replies; 37+ messages in thread From: Daniel Vetter @ 2023-09-06 6:50 UTC (permalink / raw) To: John Harrison; +Cc: intel-gfx, dri-devel On Mon, Aug 28, 2023 at 04:01:38PM -0700, John Harrison wrote: > On 8/23/2023 10:37, John Harrison wrote: > > On 8/23/2023 09:00, Daniel Vetter wrote: > > > On Tue, Aug 22, 2023 at 11:53:24AM -0700, John Harrison wrote: > > > > On 8/11/2023 11:20, Zhanjun Dong wrote: > > > > > This attempts to avoid circular locking dependency between > > > > > flush delayed > > > > > work and intel_gt_reset. > > > > > When intel_gt_reset was called, task will hold a lock. > > > > > To cacel delayed work here, the _sync version will also > > > > > acquire a lock, > > > > > which might trigger the possible cirular locking dependency warning. > > > > > When intel_gt_reset called, reset_in_progress flag will be > > > > > set, add code > > > > > to check the flag, call async verion if reset is in progress. > > > > > > > > > > Signed-off-by: Zhanjun Dong<zhanjun.dong@intel.com> > > > > > Cc: John Harrison<John.C.Harrison@Intel.com> > > > > > Cc: Andi Shyti<andi.shyti@linux.intel.com> > > > > > Cc: Daniel Vetter<daniel@ffwll.ch> > > > > > --- > > > > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git > > > > > a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > > index a0e3ef1c65d2..600388c849f7 100644 > > > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > > @@ -1359,7 +1359,16 @@ static void > > > > > guc_enable_busyness_worker(struct intel_guc *guc) > > > > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > > > > > { > > > > > - cancel_delayed_work_sync(&guc->timestamp.work); > > > > > + /* > > > > > + * When intel_gt_reset was called, task will hold a lock. > > > > > + * To cacel delayed work here, the _sync version will > > > > > also acquire a lock, which might > > > > > + * trigger the possible cirular locking dependency warning. > > > > > + * Check the reset_in_progress flag, call async verion > > > > > if reset is in progress. > > > > > + */ > > > > This needs to explain in much more detail what is going on and > > > > why it is not > > > > a problem. E.g.: > > > > > > > > The busyness worker needs to be cancelled. In general that means > > > > using the synchronous cancel version to ensure that an in-progress > > > > worker will not keep executing beyond whatever is happening that > > > > needs the cancel. E.g. suspend, driver unload, etc. However, in the > > > > case of a reset, the synchronous version is not required and can > > > > trigger a false deadlock detection warning. > > > > > > > > The business worker takes the reset mutex to protect against resets > > > > interfering with it. However, it does a trylock and bails > > > > out if the > > > > reset lock is already acquired. Thus there is no actual deadlock or > > > > other concern with the worker running concurrently with a reset. So > > > > an asynchronous cancel is safe in the case of a reset rather than a > > > > driver unload or suspend type operation. On the other hand, if the > > > > cancel_sync version is used when a reset is in progress then the > > > > mutex deadlock detection sees the mutex being acquired through > > > > multiple paths and complains. > > > > > > > > So just don't bother. That keeps the detection code happy and is > > > > safe because of the trylock code described above. > > > So why do we even need to cancel anything if it doesn't do anything > > > while > > > the reset is in progress? > > It still needs to be cancelled. The worker only aborts if it is actively > > executing concurrently with the reset. It might not start to execute > > until after the reset has completed. And there is presumably a reason > > why the cancel is being called, a reason not necessarily related to > > resets at all. Leaving the worker to run arbitrarily after the driver is > > expecting it to be stopped will lead to much worse things than a fake > > lockdep splat, e.g. a use after free pointer deref. > > > > John. > @Daniel Vetter - ping? Is this explanation sufficient? Are you okay with > this change now? Sorry for the late reply, I'm constantly behind on mails :-/ Ping me on irc next time around if I don't reply, that's quicker. "presumably" isn't good enough for locking design. Either you know, and can prove it all, or you shouldn't touch the code and its locking design before you've figured this out. Again, either this is a deadlock, race condition, or the cancel isn't necessary. And this argument works in full generality. All this patch does it replace the dealock with one of the other two, and that's not good enough if you don't even know which one it is. - if you need the cancel, you have a race condition - if you don't have a race condition, you don't need the cancel - currently you have the deadlock "presumably" and "maybe" aint enoug for locking design. Cheers, Daniel > > John. > > > > > > > > > Just remove the cancel from the reset path as uneeded instead, and > > > explain > > > why that's ok? Because that's defacto what the cancel_work with a > > > potential deadlock scenario for cancel_work_sync does, you either don't > > > need it at all, or the replacement creates a bug. > > > -Daniel > > > > > > > > > > > John. > > > > > > > > > > > > > + if (guc_to_gt(guc)->uc.reset_in_progress) > > > > > + cancel_delayed_work(&guc->timestamp.work); > > > > > + else > > > > > + cancel_delayed_work_sync(&guc->timestamp.work); > > > > > } > > > > > static void __reset_guc_busyness_stats(struct intel_guc *guc) > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-09-06 6:50 ` Daniel Vetter (?) @ 2023-09-06 18:40 ` John Harrison -1 siblings, 0 replies; 37+ messages in thread From: John Harrison @ 2023-09-06 18:40 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, dri-devel On 9/5/2023 23:50, Daniel Vetter wrote: > On Mon, Aug 28, 2023 at 04:01:38PM -0700, John Harrison wrote: >> On 8/23/2023 10:37, John Harrison wrote: >>> On 8/23/2023 09:00, Daniel Vetter wrote: >>>> On Tue, Aug 22, 2023 at 11:53:24AM -0700, John Harrison wrote: >>>>> On 8/11/2023 11:20, Zhanjun Dong wrote: >>>>>> This attempts to avoid circular locking dependency between >>>>>> flush delayed >>>>>> work and intel_gt_reset. >>>>>> When intel_gt_reset was called, task will hold a lock. >>>>>> To cacel delayed work here, the _sync version will also >>>>>> acquire a lock, >>>>>> which might trigger the possible cirular locking dependency warning. >>>>>> When intel_gt_reset called, reset_in_progress flag will be >>>>>> set, add code >>>>>> to check the flag, call async verion if reset is in progress. >>>>>> >>>>>> Signed-off-by: Zhanjun Dong<zhanjun.dong@intel.com> >>>>>> Cc: John Harrison<John.C.Harrison@Intel.com> >>>>>> Cc: Andi Shyti<andi.shyti@linux.intel.com> >>>>>> Cc: Daniel Vetter<daniel@ffwll.ch> >>>>>> --- >>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- >>>>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git >>>>>> a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>>>> index a0e3ef1c65d2..600388c849f7 100644 >>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>>>> @@ -1359,7 +1359,16 @@ static void >>>>>> guc_enable_busyness_worker(struct intel_guc *guc) >>>>>> static void guc_cancel_busyness_worker(struct intel_guc *guc) >>>>>> { >>>>>> - cancel_delayed_work_sync(&guc->timestamp.work); >>>>>> + /* >>>>>> + * When intel_gt_reset was called, task will hold a lock. >>>>>> + * To cacel delayed work here, the _sync version will >>>>>> also acquire a lock, which might >>>>>> + * trigger the possible cirular locking dependency warning. >>>>>> + * Check the reset_in_progress flag, call async verion >>>>>> if reset is in progress. >>>>>> + */ >>>>> This needs to explain in much more detail what is going on and >>>>> why it is not >>>>> a problem. E.g.: >>>>> >>>>> The busyness worker needs to be cancelled. In general that means >>>>> using the synchronous cancel version to ensure that an in-progress >>>>> worker will not keep executing beyond whatever is happening that >>>>> needs the cancel. E.g. suspend, driver unload, etc. However, in the >>>>> case of a reset, the synchronous version is not required and can >>>>> trigger a false deadlock detection warning. >>>>> >>>>> The business worker takes the reset mutex to protect against resets >>>>> interfering with it. However, it does a trylock and bails >>>>> out if the >>>>> reset lock is already acquired. Thus there is no actual deadlock or >>>>> other concern with the worker running concurrently with a reset. So >>>>> an asynchronous cancel is safe in the case of a reset rather than a >>>>> driver unload or suspend type operation. On the other hand, if the >>>>> cancel_sync version is used when a reset is in progress then the >>>>> mutex deadlock detection sees the mutex being acquired through >>>>> multiple paths and complains. >>>>> >>>>> So just don't bother. That keeps the detection code happy and is >>>>> safe because of the trylock code described above. >>>> So why do we even need to cancel anything if it doesn't do anything >>>> while >>>> the reset is in progress? >>> It still needs to be cancelled. The worker only aborts if it is actively >>> executing concurrently with the reset. It might not start to execute >>> until after the reset has completed. And there is presumably a reason >>> why the cancel is being called, a reason not necessarily related to >>> resets at all. Leaving the worker to run arbitrarily after the driver is >>> expecting it to be stopped will lead to much worse things than a fake >>> lockdep splat, e.g. a use after free pointer deref. >>> >>> John. >> @Daniel Vetter - ping? Is this explanation sufficient? Are you okay with >> this change now? > Sorry for the late reply, I'm constantly behind on mails :-/ Ping me on > irc next time around if I don't reply, that's quicker. > > "presumably" isn't good enough for locking design. Either you know, and > can prove it all, or you shouldn't touch the code and its locking design > before you've figured this out. > > Again, either this is a deadlock, race condition, or the cancel isn't > necessary. And this argument works in full generality. All this patch does > it replace the dealock with one of the other two, and that's not good > enough if you don't even know which one it is. > > - if you need the cancel, you have a race condition > > - if you don't have a race condition, you don't need the cancel In the case of a reset in progress, we do not strictly need the cancel. The worker thread will take care of avoiding a deadlock by itself. But it is more efficient to do the cancel and avoid unnecessary code execution if possible. It is also more logically correct - the worker is being stopped, therefore we should cancel any pending execution of the worker. In the case of a reset not being in progress, we absolutely do need the cancel as there are multiple race conditions. > > - currently you have the deadlock No, we do not. There is no deadlock. The worker thread explicitly does a trylock and reschedules itself for later if it could not get the lock. Lockdep does not understand the back off semantics of the trylock and reports a false failure. As explained in the above code comment, if a reset is in progress then the synchronous cancel is not required because the trylock will take care of it. An asynchronous cancel is still better than no cancel because it at least tries to stop the worker from running. It's not a problem if the worker does run, but there is no advantage to running it so why not attempt to cancel it and prevent unnecessary code execution? If a reset is not in progress then we do want the synchronous cancel because the disable is potentially part of a driver unload or similar operation that requires the worker to be stopped first. Otherwise, the worker will attempt to reference pointers that might no longer exist at the time it runs. Clearly, that is not the situation if a reset is in progress. The driver cannot be unloading if it is in the middle of a reset operation. > > "presumably" and "maybe" aint enoug for locking design. Bad choice of words. I simply meant that the low level helper does not know the exact call stack it was called with. It can trivially determine if a reset is in progress or not, and that is all it really needs to know about. There are multiple other paths to this helper, none of which involve resets and some of which require a synchronous cancel. A previous version of this patch attempted to have two separate helpers - one synchronous and one asynchronous. The intention being that any call stack involving reset would call the async version and any stack requiring the sync version would call that. However, the i915 reset design is hideously complex and those changes were much more intrusive and fragile. Simply testing the reset flag at the lowest level is significantly simpler and safer. John. > > Cheers, Daniel > >> John. >> >>>> Just remove the cancel from the reset path as uneeded instead, and >>>> explain >>>> why that's ok? Because that's defacto what the cancel_work with a >>>> potential deadlock scenario for cancel_work_sync does, you either don't >>>> need it at all, or the replacement creates a bug. >>>> -Daniel >>>> >>>>> John. >>>>> >>>>> >>>>>> + if (guc_to_gt(guc)->uc.reset_in_progress) >>>>>> + cancel_delayed_work(&guc->timestamp.work); >>>>>> + else >>>>>> + cancel_delayed_work_sync(&guc->timestamp.work); >>>>>> } >>>>>> static void __reset_guc_busyness_stats(struct intel_guc *guc) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-23 17:37 ` John Harrison @ 2023-08-31 14:00 ` Andi Shyti -1 siblings, 0 replies; 37+ messages in thread From: Andi Shyti @ 2023-08-31 14:00 UTC (permalink / raw) To: John Harrison; +Cc: intel-gfx, dri-devel, Daniel Vetter Hi, > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > index a0e3ef1c65d2..600388c849f7 100644 > > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) > > > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > > > > { > > > > - cancel_delayed_work_sync(&guc->timestamp.work); > > > > + /* > > > > + * When intel_gt_reset was called, task will hold a lock. > > > > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > > > > + * trigger the possible cirular locking dependency warning. > > > > + * Check the reset_in_progress flag, call async verion if reset is in progress. > > > > + */ > > > This needs to explain in much more detail what is going on and why it is not > > > a problem. E.g.: > > > > > > The busyness worker needs to be cancelled. In general that means > > > using the synchronous cancel version to ensure that an in-progress > > > worker will not keep executing beyond whatever is happening that > > > needs the cancel. E.g. suspend, driver unload, etc. However, in the > > > case of a reset, the synchronous version is not required and can > > > trigger a false deadlock detection warning. > > > > > > The business worker takes the reset mutex to protect against resets > > > interfering with it. However, it does a trylock and bails out if the > > > reset lock is already acquired. Thus there is no actual deadlock or > > > other concern with the worker running concurrently with a reset. So > > > an asynchronous cancel is safe in the case of a reset rather than a > > > driver unload or suspend type operation. On the other hand, if the > > > cancel_sync version is used when a reset is in progress then the > > > mutex deadlock detection sees the mutex being acquired through > > > multiple paths and complains. > > > > > > So just don't bother. That keeps the detection code happy and is > > > safe because of the trylock code described above. > > So why do we even need to cancel anything if it doesn't do anything while > > the reset is in progress? > It still needs to be cancelled. The worker only aborts if it is actively > executing concurrently with the reset. It might not start to execute until > after the reset has completed. And there is presumably a reason why the > cancel is being called, a reason not necessarily related to resets at all. > Leaving the worker to run arbitrarily after the driver is expecting it to be > stopped will lead to much worse things than a fake lockdep splat, e.g. a use > after free pointer deref. I was actually thinking why not leave things as they are and just disable lockdep from CI. This doesn't look like a relevant report to me. Andi ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-08-31 14:00 ` Andi Shyti 0 siblings, 0 replies; 37+ messages in thread From: Andi Shyti @ 2023-08-31 14:00 UTC (permalink / raw) To: John Harrison; +Cc: Zhanjun Dong, intel-gfx, dri-devel, Andi Shyti Hi, > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > index a0e3ef1c65d2..600388c849f7 100644 > > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > > > > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) > > > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > > > > { > > > > - cancel_delayed_work_sync(&guc->timestamp.work); > > > > + /* > > > > + * When intel_gt_reset was called, task will hold a lock. > > > > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > > > > + * trigger the possible cirular locking dependency warning. > > > > + * Check the reset_in_progress flag, call async verion if reset is in progress. > > > > + */ > > > This needs to explain in much more detail what is going on and why it is not > > > a problem. E.g.: > > > > > > The busyness worker needs to be cancelled. In general that means > > > using the synchronous cancel version to ensure that an in-progress > > > worker will not keep executing beyond whatever is happening that > > > needs the cancel. E.g. suspend, driver unload, etc. However, in the > > > case of a reset, the synchronous version is not required and can > > > trigger a false deadlock detection warning. > > > > > > The business worker takes the reset mutex to protect against resets > > > interfering with it. However, it does a trylock and bails out if the > > > reset lock is already acquired. Thus there is no actual deadlock or > > > other concern with the worker running concurrently with a reset. So > > > an asynchronous cancel is safe in the case of a reset rather than a > > > driver unload or suspend type operation. On the other hand, if the > > > cancel_sync version is used when a reset is in progress then the > > > mutex deadlock detection sees the mutex being acquired through > > > multiple paths and complains. > > > > > > So just don't bother. That keeps the detection code happy and is > > > safe because of the trylock code described above. > > So why do we even need to cancel anything if it doesn't do anything while > > the reset is in progress? > It still needs to be cancelled. The worker only aborts if it is actively > executing concurrently with the reset. It might not start to execute until > after the reset has completed. And there is presumably a reason why the > cancel is being called, a reason not necessarily related to resets at all. > Leaving the worker to run arbitrarily after the driver is expecting it to be > stopped will lead to much worse things than a fake lockdep splat, e.g. a use > after free pointer deref. I was actually thinking why not leave things as they are and just disable lockdep from CI. This doesn't look like a relevant report to me. Andi ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-31 14:00 ` Andi Shyti @ 2023-08-31 22:27 ` John Harrison -1 siblings, 0 replies; 37+ messages in thread From: John Harrison @ 2023-08-31 22:27 UTC (permalink / raw) To: Andi Shyti; +Cc: intel-gfx, dri-devel, Daniel Vetter On 8/31/2023 07:00, Andi Shyti wrote: > Hi, > >>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>>> index a0e3ef1c65d2..600388c849f7 100644 >>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>>> @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) >>>>> static void guc_cancel_busyness_worker(struct intel_guc *guc) >>>>> { >>>>> - cancel_delayed_work_sync(&guc->timestamp.work); >>>>> + /* >>>>> + * When intel_gt_reset was called, task will hold a lock. >>>>> + * To cacel delayed work here, the _sync version will also acquire a lock, which might >>>>> + * trigger the possible cirular locking dependency warning. >>>>> + * Check the reset_in_progress flag, call async verion if reset is in progress. >>>>> + */ >>>> This needs to explain in much more detail what is going on and why it is not >>>> a problem. E.g.: >>>> >>>> The busyness worker needs to be cancelled. In general that means >>>> using the synchronous cancel version to ensure that an in-progress >>>> worker will not keep executing beyond whatever is happening that >>>> needs the cancel. E.g. suspend, driver unload, etc. However, in the >>>> case of a reset, the synchronous version is not required and can >>>> trigger a false deadlock detection warning. >>>> >>>> The business worker takes the reset mutex to protect against resets >>>> interfering with it. However, it does a trylock and bails out if the >>>> reset lock is already acquired. Thus there is no actual deadlock or >>>> other concern with the worker running concurrently with a reset. So >>>> an asynchronous cancel is safe in the case of a reset rather than a >>>> driver unload or suspend type operation. On the other hand, if the >>>> cancel_sync version is used when a reset is in progress then the >>>> mutex deadlock detection sees the mutex being acquired through >>>> multiple paths and complains. >>>> >>>> So just don't bother. That keeps the detection code happy and is >>>> safe because of the trylock code described above. >>> So why do we even need to cancel anything if it doesn't do anything while >>> the reset is in progress? >> It still needs to be cancelled. The worker only aborts if it is actively >> executing concurrently with the reset. It might not start to execute until >> after the reset has completed. And there is presumably a reason why the >> cancel is being called, a reason not necessarily related to resets at all. >> Leaving the worker to run arbitrarily after the driver is expecting it to be >> stopped will lead to much worse things than a fake lockdep splat, e.g. a use >> after free pointer deref. > I was actually thinking why not leave things as they are and just > disable lockdep from CI. This doesn't look like a relevant report > to me. > > Andi Disable lockdep? The whole of lockdep? We absolutely do not want to disable an extremely important deadlock testing infrastructure in our test framework. That would be defeating the whole point of CI. Potentially we could annotate this one particular scenario to suppress this one particular error. But it seems simpler and safer to just update the code to not hit that scenario in the first place. John. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-08-31 22:27 ` John Harrison 0 siblings, 0 replies; 37+ messages in thread From: John Harrison @ 2023-08-31 22:27 UTC (permalink / raw) To: Andi Shyti; +Cc: Zhanjun Dong, intel-gfx, dri-devel On 8/31/2023 07:00, Andi Shyti wrote: > Hi, > >>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>>> index a0e3ef1c65d2..600388c849f7 100644 >>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c >>>>> @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) >>>>> static void guc_cancel_busyness_worker(struct intel_guc *guc) >>>>> { >>>>> - cancel_delayed_work_sync(&guc->timestamp.work); >>>>> + /* >>>>> + * When intel_gt_reset was called, task will hold a lock. >>>>> + * To cacel delayed work here, the _sync version will also acquire a lock, which might >>>>> + * trigger the possible cirular locking dependency warning. >>>>> + * Check the reset_in_progress flag, call async verion if reset is in progress. >>>>> + */ >>>> This needs to explain in much more detail what is going on and why it is not >>>> a problem. E.g.: >>>> >>>> The busyness worker needs to be cancelled. In general that means >>>> using the synchronous cancel version to ensure that an in-progress >>>> worker will not keep executing beyond whatever is happening that >>>> needs the cancel. E.g. suspend, driver unload, etc. However, in the >>>> case of a reset, the synchronous version is not required and can >>>> trigger a false deadlock detection warning. >>>> >>>> The business worker takes the reset mutex to protect against resets >>>> interfering with it. However, it does a trylock and bails out if the >>>> reset lock is already acquired. Thus there is no actual deadlock or >>>> other concern with the worker running concurrently with a reset. So >>>> an asynchronous cancel is safe in the case of a reset rather than a >>>> driver unload or suspend type operation. On the other hand, if the >>>> cancel_sync version is used when a reset is in progress then the >>>> mutex deadlock detection sees the mutex being acquired through >>>> multiple paths and complains. >>>> >>>> So just don't bother. That keeps the detection code happy and is >>>> safe because of the trylock code described above. >>> So why do we even need to cancel anything if it doesn't do anything while >>> the reset is in progress? >> It still needs to be cancelled. The worker only aborts if it is actively >> executing concurrently with the reset. It might not start to execute until >> after the reset has completed. And there is presumably a reason why the >> cancel is being called, a reason not necessarily related to resets at all. >> Leaving the worker to run arbitrarily after the driver is expecting it to be >> stopped will lead to much worse things than a fake lockdep splat, e.g. a use >> after free pointer deref. > I was actually thinking why not leave things as they are and just > disable lockdep from CI. This doesn't look like a relevant report > to me. > > Andi Disable lockdep? The whole of lockdep? We absolutely do not want to disable an extremely important deadlock testing infrastructure in our test framework. That would be defeating the whole point of CI. Potentially we could annotate this one particular scenario to suppress this one particular error. But it seems simpler and safer to just update the code to not hit that scenario in the first place. John. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-31 22:27 ` John Harrison @ 2023-09-06 9:17 ` Andi Shyti -1 siblings, 0 replies; 37+ messages in thread From: Andi Shyti @ 2023-09-06 9:17 UTC (permalink / raw) To: John Harrison; +Cc: dri-devel, intel-gfx, Daniel Vetter Hi John, > > > > > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > > > > > > { > > > > > > - cancel_delayed_work_sync(&guc->timestamp.work); > > > > > > + /* > > > > > > + * When intel_gt_reset was called, task will hold a lock. > > > > > > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > > > > > > + * trigger the possible cirular locking dependency warning. > > > > > > + * Check the reset_in_progress flag, call async verion if reset is in progress. > > > > > > + */ > > > > > This needs to explain in much more detail what is going on and why it is not > > > > > a problem. E.g.: > > > > > > > > > > The busyness worker needs to be cancelled. In general that means > > > > > using the synchronous cancel version to ensure that an in-progress > > > > > worker will not keep executing beyond whatever is happening that > > > > > needs the cancel. E.g. suspend, driver unload, etc. However, in the > > > > > case of a reset, the synchronous version is not required and can > > > > > trigger a false deadlock detection warning. > > > > > > > > > > The business worker takes the reset mutex to protect against resets > > > > > interfering with it. However, it does a trylock and bails out if the > > > > > reset lock is already acquired. Thus there is no actual deadlock or > > > > > other concern with the worker running concurrently with a reset. So > > > > > an asynchronous cancel is safe in the case of a reset rather than a > > > > > driver unload or suspend type operation. On the other hand, if the > > > > > cancel_sync version is used when a reset is in progress then the > > > > > mutex deadlock detection sees the mutex being acquired through > > > > > multiple paths and complains. > > > > > > > > > > So just don't bother. That keeps the detection code happy and is > > > > > safe because of the trylock code described above. > > > > So why do we even need to cancel anything if it doesn't do anything while > > > > the reset is in progress? > > > It still needs to be cancelled. The worker only aborts if it is actively > > > executing concurrently with the reset. It might not start to execute until > > > after the reset has completed. And there is presumably a reason why the > > > cancel is being called, a reason not necessarily related to resets at all. > > > Leaving the worker to run arbitrarily after the driver is expecting it to be > > > stopped will lead to much worse things than a fake lockdep splat, e.g. a use > > > after free pointer deref. > > I was actually thinking why not leave things as they are and just > > disable lockdep from CI. This doesn't look like a relevant report > > to me. > > > > Andi > Disable lockdep? The whole of lockdep? We absolutely do not want to disable > an extremely important deadlock testing infrastructure in our test > framework. That would be defeating the whole point of CI. > > Potentially we could annotate this one particular scenario to suppress this > one particular error. But it seems simpler and safer to just update the > code to not hit that scenario in the first place. yes... lockdep is a debug tool and might provide false reports... We need to have a great willingness to start fixing and hunting debug lockdep's false positives (like this one, for instance). It's even more annoying to reduce our CI pass rates, especially when in BAT tests, with such false deadlocks. It's the developer's responsibility to test its code with debug_lockdep and fix all the potential deadlocks and ignore the false ones. I sent a patch for this[*] already. Andi [*] https://gitlab.freedesktop.org/gfx-ci/i915-infra/-/merge_requests/128 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-09-06 9:17 ` Andi Shyti 0 siblings, 0 replies; 37+ messages in thread From: Andi Shyti @ 2023-09-06 9:17 UTC (permalink / raw) To: John Harrison; +Cc: dri-devel, Zhanjun Dong, intel-gfx, Andi Shyti Hi John, > > > > > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > > > > > > { > > > > > > - cancel_delayed_work_sync(&guc->timestamp.work); > > > > > > + /* > > > > > > + * When intel_gt_reset was called, task will hold a lock. > > > > > > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > > > > > > + * trigger the possible cirular locking dependency warning. > > > > > > + * Check the reset_in_progress flag, call async verion if reset is in progress. > > > > > > + */ > > > > > This needs to explain in much more detail what is going on and why it is not > > > > > a problem. E.g.: > > > > > > > > > > The busyness worker needs to be cancelled. In general that means > > > > > using the synchronous cancel version to ensure that an in-progress > > > > > worker will not keep executing beyond whatever is happening that > > > > > needs the cancel. E.g. suspend, driver unload, etc. However, in the > > > > > case of a reset, the synchronous version is not required and can > > > > > trigger a false deadlock detection warning. > > > > > > > > > > The business worker takes the reset mutex to protect against resets > > > > > interfering with it. However, it does a trylock and bails out if the > > > > > reset lock is already acquired. Thus there is no actual deadlock or > > > > > other concern with the worker running concurrently with a reset. So > > > > > an asynchronous cancel is safe in the case of a reset rather than a > > > > > driver unload or suspend type operation. On the other hand, if the > > > > > cancel_sync version is used when a reset is in progress then the > > > > > mutex deadlock detection sees the mutex being acquired through > > > > > multiple paths and complains. > > > > > > > > > > So just don't bother. That keeps the detection code happy and is > > > > > safe because of the trylock code described above. > > > > So why do we even need to cancel anything if it doesn't do anything while > > > > the reset is in progress? > > > It still needs to be cancelled. The worker only aborts if it is actively > > > executing concurrently with the reset. It might not start to execute until > > > after the reset has completed. And there is presumably a reason why the > > > cancel is being called, a reason not necessarily related to resets at all. > > > Leaving the worker to run arbitrarily after the driver is expecting it to be > > > stopped will lead to much worse things than a fake lockdep splat, e.g. a use > > > after free pointer deref. > > I was actually thinking why not leave things as they are and just > > disable lockdep from CI. This doesn't look like a relevant report > > to me. > > > > Andi > Disable lockdep? The whole of lockdep? We absolutely do not want to disable > an extremely important deadlock testing infrastructure in our test > framework. That would be defeating the whole point of CI. > > Potentially we could annotate this one particular scenario to suppress this > one particular error. But it seems simpler and safer to just update the > code to not hit that scenario in the first place. yes... lockdep is a debug tool and might provide false reports... We need to have a great willingness to start fixing and hunting debug lockdep's false positives (like this one, for instance). It's even more annoying to reduce our CI pass rates, especially when in BAT tests, with such false deadlocks. It's the developer's responsibility to test its code with debug_lockdep and fix all the potential deadlocks and ignore the false ones. I sent a patch for this[*] already. Andi [*] https://gitlab.freedesktop.org/gfx-ci/i915-infra/-/merge_requests/128 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-09-06 9:17 ` Andi Shyti @ 2023-09-06 10:04 ` Jani Nikula -1 siblings, 0 replies; 37+ messages in thread From: Jani Nikula @ 2023-09-06 10:04 UTC (permalink / raw) To: Andi Shyti, John Harrison; +Cc: intel-gfx, Daniel Vetter, dri-devel On Wed, 06 Sep 2023, Andi Shyti <andi.shyti@linux.intel.com> wrote: >> > I was actually thinking why not leave things as they are and just >> > disable lockdep from CI. This doesn't look like a relevant report >> > to me. >> > >> > Andi >> Disable lockdep? The whole of lockdep? We absolutely do not want to disable >> an extremely important deadlock testing infrastructure in our test >> framework. That would be defeating the whole point of CI. >> >> Potentially we could annotate this one particular scenario to suppress this >> one particular error. But it seems simpler and safer to just update the >> code to not hit that scenario in the first place. > > yes... lockdep is a debug tool and might provide false reports... > We need to have a great willingness to start fixing and hunting > debug lockdep's false positives (like this one, for instance). > > It's even more annoying to reduce our CI pass rates, especially > when in BAT tests, with such false deadlocks. Make lockdep understand what you're doing, and there are no false positives. That's all there is to it. > It's the developer's responsibility to test its code with > debug_lockdep and fix all the potential deadlocks and ignore the > false ones. No. Manual validation of lockdep reports is not feasible. Lockdep is the tool to validate locking. It's the developer's responsibility to make lockdep understand the design. Besides, locking is often subtle. Stuff can change as a side effect even when you're not intentionally changing locking, e.g. during refactoring. What you're suggesting effectively means all developers should run all of igt on a bunch of different generations of machines with lockdep enabled. Realistically, not going to happen, and we have CI because of this. > I sent a patch for this[*] already. > > Andi > > [*] https://gitlab.freedesktop.org/gfx-ci/i915-infra/-/merge_requests/128 Yeah, no. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-09-06 10:04 ` Jani Nikula 0 siblings, 0 replies; 37+ messages in thread From: Jani Nikula @ 2023-09-06 10:04 UTC (permalink / raw) To: Andi Shyti, John Harrison; +Cc: intel-gfx, dri-devel On Wed, 06 Sep 2023, Andi Shyti <andi.shyti@linux.intel.com> wrote: >> > I was actually thinking why not leave things as they are and just >> > disable lockdep from CI. This doesn't look like a relevant report >> > to me. >> > >> > Andi >> Disable lockdep? The whole of lockdep? We absolutely do not want to disable >> an extremely important deadlock testing infrastructure in our test >> framework. That would be defeating the whole point of CI. >> >> Potentially we could annotate this one particular scenario to suppress this >> one particular error. But it seems simpler and safer to just update the >> code to not hit that scenario in the first place. > > yes... lockdep is a debug tool and might provide false reports... > We need to have a great willingness to start fixing and hunting > debug lockdep's false positives (like this one, for instance). > > It's even more annoying to reduce our CI pass rates, especially > when in BAT tests, with such false deadlocks. Make lockdep understand what you're doing, and there are no false positives. That's all there is to it. > It's the developer's responsibility to test its code with > debug_lockdep and fix all the potential deadlocks and ignore the > false ones. No. Manual validation of lockdep reports is not feasible. Lockdep is the tool to validate locking. It's the developer's responsibility to make lockdep understand the design. Besides, locking is often subtle. Stuff can change as a side effect even when you're not intentionally changing locking, e.g. during refactoring. What you're suggesting effectively means all developers should run all of igt on a bunch of different generations of machines with lockdep enabled. Realistically, not going to happen, and we have CI because of this. > I sent a patch for this[*] already. > > Andi > > [*] https://gitlab.freedesktop.org/gfx-ci/i915-infra/-/merge_requests/128 Yeah, no. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-09-06 10:04 ` Jani Nikula @ 2023-09-06 11:02 ` Daniel Vetter -1 siblings, 0 replies; 37+ messages in thread From: Daniel Vetter @ 2023-09-06 11:02 UTC (permalink / raw) To: Jani Nikula; +Cc: Daniel Vetter, dri-devel, intel-gfx On Wed, Sep 06, 2023 at 01:04:06PM +0300, Jani Nikula wrote: > On Wed, 06 Sep 2023, Andi Shyti <andi.shyti@linux.intel.com> wrote: > > It's the developer's responsibility to test its code with > > debug_lockdep and fix all the potential deadlocks and ignore the > > false ones. > > No. Manual validation of lockdep reports is not feasible. Lockdep is the > tool to validate locking. It's the developer's responsibility to make > lockdep understand the design. Yeah I guess I need to drop my locking design principle once more: If lockdep doesn't understand your locking design, your design is shit. You need to fix the design, not play whack-a-mole with lockdep. Or worse, pretend there's no problem and just disable lockdep outright. If you don't understand your design, and can't succinctly explain it (or demonstrate the full hierarchy with lockdep priming, testing in CI isn't good enough for anything remotely complex), then you have a _really_ big problem. Yes CI is good at catching accidental changes in locking design, but if you use it for anything more than that you're in deep trouble. Cheers, Sima -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-09-06 11:02 ` Daniel Vetter 0 siblings, 0 replies; 37+ messages in thread From: Daniel Vetter @ 2023-09-06 11:02 UTC (permalink / raw) To: Jani Nikula; +Cc: dri-devel, intel-gfx, John Harrison, Andi Shyti On Wed, Sep 06, 2023 at 01:04:06PM +0300, Jani Nikula wrote: > On Wed, 06 Sep 2023, Andi Shyti <andi.shyti@linux.intel.com> wrote: > > It's the developer's responsibility to test its code with > > debug_lockdep and fix all the potential deadlocks and ignore the > > false ones. > > No. Manual validation of lockdep reports is not feasible. Lockdep is the > tool to validate locking. It's the developer's responsibility to make > lockdep understand the design. Yeah I guess I need to drop my locking design principle once more: If lockdep doesn't understand your locking design, your design is shit. You need to fix the design, not play whack-a-mole with lockdep. Or worse, pretend there's no problem and just disable lockdep outright. If you don't understand your design, and can't succinctly explain it (or demonstrate the full hierarchy with lockdep priming, testing in CI isn't good enough for anything remotely complex), then you have a _really_ big problem. Yes CI is good at catching accidental changes in locking design, but if you use it for anything more than that you're in deep trouble. Cheers, Sima -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-09-06 9:17 ` Andi Shyti @ 2023-09-06 18:49 ` John Harrison -1 siblings, 0 replies; 37+ messages in thread From: John Harrison @ 2023-09-06 18:49 UTC (permalink / raw) To: Andi Shyti; +Cc: intel-gfx, dri-devel, Daniel Vetter On 9/6/2023 02:17, Andi Shyti wrote: > Hi John, > >>>>>>> static void guc_cancel_busyness_worker(struct intel_guc *guc) >>>>>>> { >>>>>>> - cancel_delayed_work_sync(&guc->timestamp.work); >>>>>>> + /* >>>>>>> + * When intel_gt_reset was called, task will hold a lock. >>>>>>> + * To cacel delayed work here, the _sync version will also acquire a lock, which might >>>>>>> + * trigger the possible cirular locking dependency warning. >>>>>>> + * Check the reset_in_progress flag, call async verion if reset is in progress. >>>>>>> + */ >>>>>> This needs to explain in much more detail what is going on and why it is not >>>>>> a problem. E.g.: >>>>>> >>>>>> The busyness worker needs to be cancelled. In general that means >>>>>> using the synchronous cancel version to ensure that an in-progress >>>>>> worker will not keep executing beyond whatever is happening that >>>>>> needs the cancel. E.g. suspend, driver unload, etc. However, in the >>>>>> case of a reset, the synchronous version is not required and can >>>>>> trigger a false deadlock detection warning. >>>>>> >>>>>> The business worker takes the reset mutex to protect against resets >>>>>> interfering with it. However, it does a trylock and bails out if the >>>>>> reset lock is already acquired. Thus there is no actual deadlock or >>>>>> other concern with the worker running concurrently with a reset. So >>>>>> an asynchronous cancel is safe in the case of a reset rather than a >>>>>> driver unload or suspend type operation. On the other hand, if the >>>>>> cancel_sync version is used when a reset is in progress then the >>>>>> mutex deadlock detection sees the mutex being acquired through >>>>>> multiple paths and complains. >>>>>> >>>>>> So just don't bother. That keeps the detection code happy and is >>>>>> safe because of the trylock code described above. >>>>> So why do we even need to cancel anything if it doesn't do anything while >>>>> the reset is in progress? >>>> It still needs to be cancelled. The worker only aborts if it is actively >>>> executing concurrently with the reset. It might not start to execute until >>>> after the reset has completed. And there is presumably a reason why the >>>> cancel is being called, a reason not necessarily related to resets at all. >>>> Leaving the worker to run arbitrarily after the driver is expecting it to be >>>> stopped will lead to much worse things than a fake lockdep splat, e.g. a use >>>> after free pointer deref. >>> I was actually thinking why not leave things as they are and just >>> disable lockdep from CI. This doesn't look like a relevant report >>> to me. >>> >>> Andi >> Disable lockdep? The whole of lockdep? We absolutely do not want to disable >> an extremely important deadlock testing infrastructure in our test >> framework. That would be defeating the whole point of CI. >> >> Potentially we could annotate this one particular scenario to suppress this >> one particular error. But it seems simpler and safer to just update the >> code to not hit that scenario in the first place. > yes... lockdep is a debug tool and might provide false reports... > We need to have a great willingness to start fixing and hunting > debug lockdep's false positives (like this one, for instance). That is how lockdep works. It's like a compiler warning. You have to fix them even if you think they don't matter. Because otherwise, when someone tries to turn warnings on, they drown in a sea of other people's unrelated garbage that they did not bother to fix. If lockdep is to be of any use at all then it must be run regularly as part of a CI type system and any issues it finds must be fixed up by the developer's that own the relevant code. Where fixing means either fixing genuine bugs, re-working the code to not hit a false positive or annotating the code to explain to lockdep why it is a safe operation. > > It's even more annoying to reduce our CI pass rates, especially > when in BAT tests, with such false deadlocks. Maybe. But it is even more annoying when you have a genuine locking issue that you don't notice because you have disabled lockdep and just have some random hang issue that is impossible to reproduce or debug. > > It's the developer's responsibility to test its code with > debug_lockdep and fix all the potential deadlocks and ignore the > false ones. You seem to have this backwards. Developers are not expected to run every possible test on every possible platform in every possible configuration. That is the job of CI. John. > I sent a patch for this[*] already. > > Andi > > [*] https://gitlab.freedesktop.org/gfx-ci/i915-infra/-/merge_requests/128 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-09-06 18:49 ` John Harrison 0 siblings, 0 replies; 37+ messages in thread From: John Harrison @ 2023-09-06 18:49 UTC (permalink / raw) To: Andi Shyti; +Cc: Zhanjun Dong, intel-gfx, dri-devel On 9/6/2023 02:17, Andi Shyti wrote: > Hi John, > >>>>>>> static void guc_cancel_busyness_worker(struct intel_guc *guc) >>>>>>> { >>>>>>> - cancel_delayed_work_sync(&guc->timestamp.work); >>>>>>> + /* >>>>>>> + * When intel_gt_reset was called, task will hold a lock. >>>>>>> + * To cacel delayed work here, the _sync version will also acquire a lock, which might >>>>>>> + * trigger the possible cirular locking dependency warning. >>>>>>> + * Check the reset_in_progress flag, call async verion if reset is in progress. >>>>>>> + */ >>>>>> This needs to explain in much more detail what is going on and why it is not >>>>>> a problem. E.g.: >>>>>> >>>>>> The busyness worker needs to be cancelled. In general that means >>>>>> using the synchronous cancel version to ensure that an in-progress >>>>>> worker will not keep executing beyond whatever is happening that >>>>>> needs the cancel. E.g. suspend, driver unload, etc. However, in the >>>>>> case of a reset, the synchronous version is not required and can >>>>>> trigger a false deadlock detection warning. >>>>>> >>>>>> The business worker takes the reset mutex to protect against resets >>>>>> interfering with it. However, it does a trylock and bails out if the >>>>>> reset lock is already acquired. Thus there is no actual deadlock or >>>>>> other concern with the worker running concurrently with a reset. So >>>>>> an asynchronous cancel is safe in the case of a reset rather than a >>>>>> driver unload or suspend type operation. On the other hand, if the >>>>>> cancel_sync version is used when a reset is in progress then the >>>>>> mutex deadlock detection sees the mutex being acquired through >>>>>> multiple paths and complains. >>>>>> >>>>>> So just don't bother. That keeps the detection code happy and is >>>>>> safe because of the trylock code described above. >>>>> So why do we even need to cancel anything if it doesn't do anything while >>>>> the reset is in progress? >>>> It still needs to be cancelled. The worker only aborts if it is actively >>>> executing concurrently with the reset. It might not start to execute until >>>> after the reset has completed. And there is presumably a reason why the >>>> cancel is being called, a reason not necessarily related to resets at all. >>>> Leaving the worker to run arbitrarily after the driver is expecting it to be >>>> stopped will lead to much worse things than a fake lockdep splat, e.g. a use >>>> after free pointer deref. >>> I was actually thinking why not leave things as they are and just >>> disable lockdep from CI. This doesn't look like a relevant report >>> to me. >>> >>> Andi >> Disable lockdep? The whole of lockdep? We absolutely do not want to disable >> an extremely important deadlock testing infrastructure in our test >> framework. That would be defeating the whole point of CI. >> >> Potentially we could annotate this one particular scenario to suppress this >> one particular error. But it seems simpler and safer to just update the >> code to not hit that scenario in the first place. > yes... lockdep is a debug tool and might provide false reports... > We need to have a great willingness to start fixing and hunting > debug lockdep's false positives (like this one, for instance). That is how lockdep works. It's like a compiler warning. You have to fix them even if you think they don't matter. Because otherwise, when someone tries to turn warnings on, they drown in a sea of other people's unrelated garbage that they did not bother to fix. If lockdep is to be of any use at all then it must be run regularly as part of a CI type system and any issues it finds must be fixed up by the developer's that own the relevant code. Where fixing means either fixing genuine bugs, re-working the code to not hit a false positive or annotating the code to explain to lockdep why it is a safe operation. > > It's even more annoying to reduce our CI pass rates, especially > when in BAT tests, with such false deadlocks. Maybe. But it is even more annoying when you have a genuine locking issue that you don't notice because you have disabled lockdep and just have some random hang issue that is impossible to reproduce or debug. > > It's the developer's responsibility to test its code with > debug_lockdep and fix all the potential deadlocks and ignore the > false ones. You seem to have this backwards. Developers are not expected to run every possible test on every possible platform in every possible configuration. That is the job of CI. John. > I sent a patch for this[*] already. > > Andi > > [*] https://gitlab.freedesktop.org/gfx-ci/i915-infra/-/merge_requests/128 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset 2023-08-22 18:53 ` John Harrison @ 2023-08-29 10:11 ` Andi Shyti -1 siblings, 0 replies; 37+ messages in thread From: Andi Shyti @ 2023-08-29 10:11 UTC (permalink / raw) To: John Harrison; +Cc: Daniel Vetter, intel-gfx, dri-devel Hi Zhanjun, On Tue, Aug 22, 2023 at 11:53:24AM -0700, John Harrison wrote: > On 8/11/2023 11:20, Zhanjun Dong wrote: > > This attempts to avoid circular locking dependency between flush delayed > work and intel_gt_reset. > When intel_gt_reset was called, task will hold a lock. > To cacel delayed work here, the _sync version will also acquire a lock, > which might trigger the possible cirular locking dependency warning. > When intel_gt_reset called, reset_in_progress flag will be set, add code > to check the flag, call async verion if reset is in progress. I liked the previous commit, it just needed to be wrapped (not in the dmesg copy-paste part). > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com> > Cc: John Harrison <John.C.Harrison@Intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index a0e3ef1c65d2..600388c849f7 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > { > - cancel_delayed_work_sync(&guc->timestamp.work); > + /* > + * When intel_gt_reset was called, task will hold a lock. > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > + * trigger the possible cirular locking dependency warning. > + * Check the reset_in_progress flag, call async verion if reset is in progress. > + */ Indeed the commit message is a bit misleading and it raises some alarms if explained it this way. > This needs to explain in much more detail what is going on and why it is not a > problem. E.g.: > > The busyness worker needs to be cancelled. In general that means using the > synchronous cancel version to ensure that an in-progress worker will not > keep executing beyond whatever is happening that needs the cancel. E.g. > suspend, driver unload, etc. However, in the case of a reset, the > synchronous version is not required and can trigger a false deadlock > detection warning. > > The business worker takes the reset mutex to protect against resets > interfering with it. However, it does a trylock and bails out if the reset > lock is already acquired. Thus there is no actual deadlock or other concern > with the worker running concurrently with a reset. So an asynchronous > cancel is safe in the case of a reset rather than a driver unload or > suspend type operation. On the other hand, if the cancel_sync version is > used when a reset is in progress then the mutex deadlock detection sees the > mutex being acquired through multiple paths and complains. > > So just don't bother. That keeps the detection code happy and is safe > because of the trylock code described above. Can you please update the commit message with John's suggestion? Is there any further question on this? Andi ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset @ 2023-08-29 10:11 ` Andi Shyti 0 siblings, 0 replies; 37+ messages in thread From: Andi Shyti @ 2023-08-29 10:11 UTC (permalink / raw) To: John Harrison; +Cc: Zhanjun Dong, intel-gfx, Andi Shyti, dri-devel Hi Zhanjun, On Tue, Aug 22, 2023 at 11:53:24AM -0700, John Harrison wrote: > On 8/11/2023 11:20, Zhanjun Dong wrote: > > This attempts to avoid circular locking dependency between flush delayed > work and intel_gt_reset. > When intel_gt_reset was called, task will hold a lock. > To cacel delayed work here, the _sync version will also acquire a lock, > which might trigger the possible cirular locking dependency warning. > When intel_gt_reset called, reset_in_progress flag will be set, add code > to check the flag, call async verion if reset is in progress. I liked the previous commit, it just needed to be wrapped (not in the dmesg copy-paste part). > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com> > Cc: John Harrison <John.C.Harrison@Intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index a0e3ef1c65d2..600388c849f7 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -1359,7 +1359,16 @@ static void guc_enable_busyness_worker(struct intel_guc *guc) > > static void guc_cancel_busyness_worker(struct intel_guc *guc) > { > - cancel_delayed_work_sync(&guc->timestamp.work); > + /* > + * When intel_gt_reset was called, task will hold a lock. > + * To cacel delayed work here, the _sync version will also acquire a lock, which might > + * trigger the possible cirular locking dependency warning. > + * Check the reset_in_progress flag, call async verion if reset is in progress. > + */ Indeed the commit message is a bit misleading and it raises some alarms if explained it this way. > This needs to explain in much more detail what is going on and why it is not a > problem. E.g.: > > The busyness worker needs to be cancelled. In general that means using the > synchronous cancel version to ensure that an in-progress worker will not > keep executing beyond whatever is happening that needs the cancel. E.g. > suspend, driver unload, etc. However, in the case of a reset, the > synchronous version is not required and can trigger a false deadlock > detection warning. > > The business worker takes the reset mutex to protect against resets > interfering with it. However, it does a trylock and bails out if the reset > lock is already acquired. Thus there is no actual deadlock or other concern > with the worker running concurrently with a reset. So an asynchronous > cancel is safe in the case of a reset rather than a driver unload or > suspend type operation. On the other hand, if the cancel_sync version is > used when a reset is in progress then the mutex deadlock detection sees the > mutex being acquired through multiple paths and complains. > > So just don't bother. That keeps the detection code happy and is safe > because of the trylock code described above. Can you please update the commit message with John's suggestion? Is there any further question on this? Andi ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2023-09-06 18:49 UTC | newest] Thread overview: 37+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-11 18:20 [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset Zhanjun Dong 2023-08-11 18:20 ` Zhanjun Dong 2023-08-11 19:15 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Avoid circular locking dependency when flush delayed work on gt reset (rev5) Patchwork 2023-08-11 19:15 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork 2023-08-11 19:34 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork 2023-08-21 14:09 ` [Intel-gfx] [PATCH v5] drm/i915: Avoid circular locking dependency when flush delayed work on gt reset Andi Shyti 2023-08-21 14:09 ` Andi Shyti 2023-08-22 13:50 ` [Intel-gfx] " Daniel Vetter 2023-08-22 13:50 ` Daniel Vetter 2023-08-22 14:14 ` [Intel-gfx] " Dong, Zhanjun 2023-08-22 14:14 ` Dong, Zhanjun 2023-08-22 14:28 ` [Intel-gfx] " Daniel Vetter 2023-08-22 14:28 ` Daniel Vetter 2023-08-22 18:53 ` [Intel-gfx] " John Harrison 2023-08-22 18:53 ` John Harrison 2023-08-23 16:00 ` [Intel-gfx] " Daniel Vetter 2023-08-23 16:00 ` Daniel Vetter 2023-08-23 17:37 ` [Intel-gfx] " John Harrison 2023-08-23 17:37 ` John Harrison 2023-08-28 23:01 ` [Intel-gfx] " John Harrison 2023-09-06 6:50 ` Daniel Vetter 2023-09-06 6:50 ` Daniel Vetter 2023-09-06 18:40 ` John Harrison 2023-08-31 14:00 ` Andi Shyti 2023-08-31 14:00 ` Andi Shyti 2023-08-31 22:27 ` [Intel-gfx] " John Harrison 2023-08-31 22:27 ` John Harrison 2023-09-06 9:17 ` [Intel-gfx] " Andi Shyti 2023-09-06 9:17 ` Andi Shyti 2023-09-06 10:04 ` [Intel-gfx] " Jani Nikula 2023-09-06 10:04 ` Jani Nikula 2023-09-06 11:02 ` Daniel Vetter 2023-09-06 11:02 ` Daniel Vetter 2023-09-06 18:49 ` John Harrison 2023-09-06 18:49 ` John Harrison 2023-08-29 10:11 ` [Intel-gfx] " Andi Shyti 2023-08-29 10:11 ` Andi Shyti
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.