On Tue, Dec 10, 2024 at 06:00:33PM -0600, Lucas De Marchi wrote:
> 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.
You cannot allocate memory under raw_spinlock_t at all, ever. Nor free
for that matter. The allocators use spinlock internally.
> [ 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.
You very much cannot sleep here. This is hardirq context.
