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:
From: Stephane Eranian eranian@google.com
In AMD PQoS Versions 1.0 and 2.0, IA32_QM_EVTSEL MSR is shared by all processors in a QOS domain. So there's a chance it can read a different event when two processors are reading the counter concurrently. Add a spinlock to prevent this race.
This is unclear to me. As I understand it this changelog is written as though there is a race that is being fixed. I believe that rdtgroup_mutex currently protects against such races. I thus at first thought that this is a prep patch for the introduction of the new soft RMID feature, but instead this new spinlock is used independent of the soft RMID feature.
I think the spinlock is unnecessary when the soft RMID feature is disabled.
My understanding was that the race would happen a lot more when simultaneously IPI'ing all CPUs in a domain, but I had apparently overlooked that all of the counter reads were already serialized.
*/
- @lock: serializes counter reads when QM_EVTSEL MSR is shared per-domain
- Members of this structure are accessed via helpers that provide abstraction.
@@ -333,6 +334,7 @@ struct rdt_hw_domain { u32 *ctrl_val; struct arch_mbm_state *arch_mbm_total; struct arch_mbm_state *arch_mbm_local;
raw_spinlock_t evtsel_lock;};
Please note the difference between the member name in the struct ("evtsel_lock") and its description ("lock").
Will fix, thanks.
-static int __rmid_read(u32 rmid, enum resctrl_event_id eventid, u64 *val) +static int __rmid_read(struct rdt_hw_domain *hw_dom, u32 rmid,
enum resctrl_event_id eventid, u64 *val){
unsigned long flags; u64 msr_val;if (static_branch_likely(&rmid_read_locked))Why static_branch_likely() as opposed to static_branch_unlikely()?
I read the documentation for static branches and I agree that unlikely would make more sense so that the non-locked case is less impacted.
This instance apparently confused my understanding of static branches and I will need to re-visit all uses of them in this patch series.
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?
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.
-Peter