* Re: [RFT] Revert "lockdep: Enable PROVE_RAW_LOCK_NESTING with PROVE_LOCKING."
[not found] ` <Z1jG53Hy0PZKdJG2@intel.com>
@ 2024-12-11 0:00 ` Lucas De Marchi
2024-12-12 9:31 ` Peter Zijlstra
0 siblings, 1 reply; 2+ messages in thread
From: Lucas De Marchi @ 2024-12-11 0:00 UTC (permalink / raw)
To: Rodrigo Vivi
Cc: Luca Coelho, intel-gfx, intel-xe, jani.saarinen, Jani Nikula,
Thomas Gleixner, Peter Zijlstra, linux-kernel
+Peter and Thomas, for question below about the use of raw_spinlock_t in perf
[ Note that the patch by itself is not proposing this revert to be merged in
any tree going to Linus - that would just shoot the messenger - it's
just a temporary stop gap to get our CI running again. Below I'm
looking for a real solution... ]
On Tue, Dec 10, 2024 at 05:55:35PM -0500, Rodrigo Vivi wrote:
>On Tue, Dec 10, 2024 at 09:00:13AM -0800, Lucas De Marchi wrote:
>> On Mon, Dec 09, 2024 at 03:53:51PM +0200, Luca Coelho wrote:
>> > This reverts commit 560af5dc839eef08a273908f390cfefefb82aa04.
>> >
>> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
>> > ---
>> >
>> > It seems that we have a few issues with this configuration in xe and
>> > in i915. Let's try to revert it to see if the problems we're seeing
>> > go away.
>> >
>> > Note, these are _real_ issues, but only if CONFIG_RT is enabled, so the actual issues need to be solved properly, but we can revert this change until then, to avoid regressions.
>>
>> +Jani Nikula, +Rodrigo
>>
>> I'm thinking about landing this in topic/core-for-CI. It seems we have
>> quite a few locks to revisit - we are taking spinlocks while holding
>> raw_spinlocks and until now there's no warning about this bug.
>
>could you point to one case? I don't see us using the raw_spinlocks...
main entrypoint is perf pmu. All these purple results:
https://intel-gfx-ci.01.org/tree/drm-tip/shards-all.html?testfilter=perf
Example:
<4> [96.732915] =============================
<4> [96.732950] [ BUG: Invalid wait context ]
<4> [96.732982] 6.13.0-rc2-CI_DRM_15816-g2223c2c738ec+ #1 Not tainted
<4> [96.733026] -----------------------------
<4> [96.733056] swapper/0/0 is trying to lock:
<4> [96.733088] ffff888129513910 (&pmu->lock){....}-{3:3}, at: i915_pmu_enable+0x48/0x3a0 [i915]
<4> [96.733485] other info that might help us debug this:
<4> [96.733536] context-{5:5}
<4> [96.733565] 1 lock held by swapper/0/0:
<4> [96.733606] #0: ffff88885f432038 (&cpuctx_lock){....}-{2:2}, at: __perf_install_in_context+0x3f/0x360
<4> [96.733710] stack backtrace:
<4> [96.733742] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.13.0-rc2-CI_DRM_15816-g2223c2c738ec+ #1
<4> [96.733841] Hardware name: Intel Corporation Meteor Lake Client Platform/MTL-P LP5x T3 RVP, BIOS MTLPFWI1.R00.3471.D91.2401310918 01/31/2024
<4> [96.733971] Call Trace:
<4> [96.734002] <TASK>
<4> [96.734029] dump_stack_lvl+0x91/0xf0
<4> [96.734078] dump_stack+0x10/0x20
<4> [96.734118] __lock_acquire+0x990/0x2820
<4> [96.734177] lock_acquire+0xc9/0x300
<4> [96.734222] ? i915_pmu_enable+0x48/0x3a0 [i915]
<4> [96.734533] _raw_spin_lock_irqsave+0x49/0x80
<4> [96.734568] ? i915_pmu_enable+0x48/0x3a0 [i915]
<4> [96.734800] i915_pmu_enable+0x48/0x3a0 [i915]
<4> [96.735031] i915_pmu_event_add+0x71/0x90 [i915]
I started converting the pmu->lock innside i915_pmu.c. I´t be great if
it was only that, but it's clear it's not sufficient. I tried to move a few locks
around to avoid having to convert uncore->lock, but ultimately couldn't avoid
it, which leads to converting a few more. So far:
raw_spin_lock_init(&guc->timestamp.lock);
raw_spin_lock_init(&pmu->lock);
raw_spin_lock_init(&i915->mmio_debug.lock);
raw_spin_lock_init(&uncore->lock);
And it's still not sufficient, because intel_ref_tracker tries to
allocate while holding one of those and I'm not confident on making that
pass GFP_ATOMIC. Maybe that allocation could be moved to init, but I ran out
of time for this and will try again later.
[ 204.706501] swapper/0/0 is trying to lock:
[ 204.710565] ffff88810005ead8 (&n->list_lock){-.-.}-{3:3}, at: get_partial_node.part.0+0x27/0x3a0
[ 204.719278] other info that might help us debug this:
[ 204.724285] context-{5:5}
[ 204.726891] 2 locks held by swapper/0/0:
[ 204.730785] #0: ffff88888cc32038 (&cpuctx_lock){....}-{2:2}, at: __perf_install_in_context+0x3f/0x360
[ 204.739995] #1: ffff88815265cf40 (&guc->timestamp.lock){....}-{2:2}, at: guc_engine_busyness+0x45/0x2c0 [i915]
[ 204.750171] stack backtrace:
[ 204.753038] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G U 6.13.0-rc2-xe+ #13
[ 204.761729] Tainted: [U]=USER
[ 204.764678] Hardware name: Intel Corporation Raptor Lake Client Platform/RPL-S ADP-S DDR5 UDIMM CRB, BIOS RPLSFWI1.R00.5045.A00.2401260733 01/26/2024
[ 204.777913] Call Trace:
[ 204.780355] <TASK>
[ 204.782450] dump_stack_lvl+0x91/0xf0
[ 204.786090] dump_stack+0x10/0x20
[ 204.789383] __lock_acquire+0x990/0x2820
[ 204.793276] ? lock_acquire+0x29c/0x300
[ 204.797088] lock_acquire+0xc9/0x300
[ 204.800642] ? get_partial_node.part.0+0x27/0x3a0
[ 204.805310] _raw_spin_lock_irqsave+0x49/0x80
[ 204.809635] ? get_partial_node.part.0+0x27/0x3a0
[ 204.814302] get_partial_node.part.0+0x27/0x3a0
[ 204.818794] ___slab_alloc+0x792/0x12f0
[ 204.822600] ? ref_tracker_alloc+0xd7/0x270
[ 204.826754] ? __lock_acquire+0x11a1/0x2820
[ 204.830906] ? ref_tracker_alloc+0xd7/0x270
[ 204.835058] __kmalloc_cache_noprof+0x277/0x480
[ 204.839554] ? __kmalloc_cache_noprof+0x277/0x480
[ 204.844221] ref_tracker_alloc+0xd7/0x270
[ 204.848206] ? ref_tracker_alloc+0xd7/0x270
[ 204.852357] guc_engine_busyness+0x122/0x2c0 [i915]
>
>>
>> It's a real problem only for PREEMPT_RT since otherwise there's
>> no difference between the 2 lock types. However fixing this may involve
>> quite a few changes: if we convert the lock to raw we may need to
>> cascade the conversions to additional locks. The ones I identified are:
>> pmu->lock, which would also need to have uncore->lock converted, which
>> would then probably cascade to quite a few others :-/. I'm not sure
>> converting uncore->lock will actually be a good thing.
>
>hmm raw_spinlocks for the lowlevel might not be a bad idea, but perhaps
>we need to convert the other way around the upper levels?
that would mean:
<4> [96.733606] #0: ffff88885f432038 (&cpuctx_lock){....}-{2:2}, at: __perf_install_in_context+0x3f/0x360
so inside the perf event infra, that has been using raw_spinlock_t
since forever. I'm surprised we got this only 10 years later :-/.
I don't think perf can sleep in that context, but Cc'ing a few people
and lkml for that question.
thanks
Lucas De Marchi
>
>>
>> I will keep digging.
>
>Ack on getting this to topic/core-for-CI so we don't block our
>CI while we investigate and fix this.
>
>Thanks,
>Rodrigo.
>
>>
>>
>> Lucas De Marchi
>>
>>
>> >
>> >
>> > lib/Kconfig.debug | 12 ++++++++++--
>> > 1 file changed, 10 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>> > index f3d723705879..de4ffe09323b 100644
>> > --- a/lib/Kconfig.debug
>> > +++ b/lib/Kconfig.debug
>> > @@ -1397,14 +1397,22 @@ config PROVE_LOCKING
>> > For more details, see Documentation/locking/lockdep-design.rst.
>> >
>> > config PROVE_RAW_LOCK_NESTING
>> > - bool
>> > + bool "Enable raw_spinlock - spinlock nesting checks"
>> > depends on PROVE_LOCKING
>> > - default y
>> > + default n
>> > help
>> > Enable the raw_spinlock vs. spinlock nesting checks which ensure
>> > that the lock nesting rules for PREEMPT_RT enabled kernels are
>> > not violated.
>> >
>> > + NOTE: There are known nesting problems. So if you enable this
>> > + option expect lockdep splats until these problems have been fully
>> > + addressed which is work in progress. This config switch allows to
>> > + identify and analyze these problems. It will be removed and the
>> > + check permanently enabled once the main issues have been fixed.
>> > +
>> > + If unsure, select N.
>> > +
>> > config LOCK_STAT
>> > bool "Lock usage statistics"
>> > depends on DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT
>> > --
>> > 2.45.2
>> >
^ permalink raw reply [flat|nested] 2+ messages in thread