Hi Peter,
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.
Co-developed-by: Peter Newman peternewman@google.com Signed-off-by: Peter Newman peternewman@google.com Signed-off-by: Stephane Eranian eranian@google.com
arch/x86/kernel/cpu/resctrl/core.c | 41 ++++++++++++++++++++++++++ arch/x86/kernel/cpu/resctrl/internal.h | 5 ++++ arch/x86/kernel/cpu/resctrl/monitor.c | 14 +++++++-- 3 files changed, 57 insertions(+), 3 deletions(-)
...
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h index 85ceaf9a31ac..02a062558c67 100644 --- a/arch/x86/kernel/cpu/resctrl/internal.h +++ b/arch/x86/kernel/cpu/resctrl/internal.h @@ -325,6 +325,7 @@ struct arch_mbm_state {
- @ctrl_val: array of cache or mem ctrl values (indexed by CLOSID)
- @arch_mbm_total: arch private state for MBM total bandwidth
- @arch_mbm_local: arch private state for MBM local bandwidth
*/
- @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").
static inline struct rdt_hw_domain *resctrl_to_arch_dom(struct rdt_domain *r) @@ -428,6 +430,9 @@ extern struct rdt_hw_resource rdt_resources_all[]; extern struct rdtgroup rdtgroup_default; DECLARE_STATIC_KEY_FALSE(rdt_alloc_enable_key); +/* Serialization required in resctrl_arch_rmid_read(). */ +DECLARE_STATIC_KEY_FALSE(rmid_read_locked);
extern struct dentry *debugfs_resctrl; enum resctrl_res_level { diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c index 20952419be75..2de8397f91cd 100644 --- a/arch/x86/kernel/cpu/resctrl/monitor.c +++ b/arch/x86/kernel/cpu/resctrl/monitor.c @@ -146,10 +146,15 @@ static inline struct rmid_entry *__rmid_entry(u32 rmid) return entry; } -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()?
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.
if (msr_val & RMID_VAL_ERROR) return -EIO; if (msr_val & RMID_VAL_UNAVAIL) @@ -200,7 +208,7 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_domain *d, memset(am, 0, sizeof(*am)); /* Record any initial, non-zero count value. */
__rmid_read(rmid, eventid, &am->prev_msr);
__rmid_read(hw_dom, rmid, eventid, &am->prev_msr);} @@ -241,7 +249,7 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d, if (!cpumask_test_cpu(smp_processor_id(), &d->cpu_mask)) return -EINVAL;
- ret = __rmid_read(rmid, eventid, &msr_val);
- ret = __rmid_read(hw_dom, rmid, eventid, &msr_val); if (ret) return ret;
Reinette