Hi Peter, On 5/12/2023 6:23 AM, Peter Newman wrote: > Hi Reinette, > > On Thu, May 11, 2023 at 11:36 PM Reinette Chatre > <reinette.chatre@xxxxxxxxx> wrote: >> On 4/21/2023 7:17 AM, Peter Newman wrote: >>> + raw_spin_lock_irqsave(&hw_dom->evtsel_lock, flags); >>> + >>> /* >>> * As per the SDM, when IA32_QM_EVTSEL.EvtID (bits 7:0) is configured >>> * with a valid event code for supported resource type and the bits >>> @@ -161,6 +166,9 @@ static int __rmid_read(u32 rmid, enum resctrl_event_id eventid, u64 *val) >>> wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid); >>> rdmsrl(MSR_IA32_QM_CTR, msr_val); >>> >>> + if (static_branch_likely(&rmid_read_locked)) >>> + raw_spin_unlock_irqrestore(&hw_dom->evtsel_lock, flags); >>> + >> >> If the first "if (static_branch_likely(&rmid_read_locked))" was taken then the second >> if branch _has_ to be taken. It should not be optional to release a lock if it was taken. I >> think it would be more robust if a single test of the static key decides whether the >> spinlock should be used. > > Is the concern that the branch value could change concurrently and > result in a deadlock? Possibly ... it may be that the static key cannot change value during this call but that thus requires deeper understanding of various flows for this logic to be trusted. I think this should be explicit: if a lock is taken then releasing it should not be optional at all. > I'm curious as to whether this case is performance critical enough to > justify using a static branch. It's clear that we should be using them > in the context switch path, but I'm confused about other places > they're used when there are also memory flags. Alternatively, there could be a, (for example) __rmid_read_lock() that is called from context switch and it always takes a spin lock. Reinette
