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@intel.com 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