Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <242db225-8ddc-968e-a754-6aaefd1b7da9@intel.com>
Date:   Thu, 11 May 2023 14:35:53 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.10.1
Subject: Re: [PATCH v1 2/9] x86/resctrl: Hold a spinlock in __rmid_read() on
 AMD
Content-Language: en-US
To:     Peter Newman <peternewman@google.com>,
        Fenghua Yu <fenghua.yu@intel.com>
CC:     Babu Moger <babu.moger@amd.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, <x86@kernel.org>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Stephane Eranian <eranian@google.com>,
        James Morse <james.morse@arm.com>,
        <linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>
References: <20230421141723.2405942-1-peternewman@google.com>
 <20230421141723.2405942-3-peternewman@google.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230421141723.2405942-3-peternewman@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MzNDY3kxQkVQdkYrNGt0dHJraFBzazhZWXV0SEdYQkZ0SjVaM3FTRW5Tb0s5?=
 =?utf-8?B?Z2QzY1MxQ2h0UkQ3VE1HS3QvTmNKRm1JZUtmWUZHM212b3FQTXlmWlZ2TzRy?=
 =?utf-8?B?bkg1UnBGclZrTEd4blhJQmFOYUZwN05YMkVkYXdGMHNCWTd3blRqbzJoeHNz?=
 =?utf-8?B?WEFKY3pTdGEwOGRLRHZWeFovbjkyblZyN1JqQjJ4aFdzRU9ZM0l1L2lScFBo?=
 =?utf-8?B?MHVtenNrb3FETk95c3lDRXhlZU5qK0JrQjJCRWd2NFVOM09BMktDTDN1bk5Y?=
 =?utf-8?B?M2N4Tm45dGptaDJJVm9ZSytwMHdGSnh4di9vRlhrb1BTeExQb29qZEt2cHBM?=
 =?utf-8?B?dzNBbHMrYWl3UThiYUFzcXMyeWRBbUNBYWZ2RENwWmdpQk9HajVlZVFrTmtE?=
 =?utf-8?B?bkJnTnpWeUkrc2h0TU1TcmVmdnhSK09pSHFkWCt1a3M0TjNNcW5pL08yM0dy?=
 =?utf-8?B?bm9MVTNzbE82anAxOE8zbGRYaDhCYUdRbVBYMWJLU21uVll1ek04ZU9FaUFl?=
 =?utf-8?B?K1I1M1AzZ2hoRU5RSlBkUnRibDZoeFNVZmVwL1hvS0JjelY3SGthdXhSaDhK?=
 =?utf-8?B?akV6L3NmOXV0c1hiQnpKb1pjSzhqMUpqV0J1WFBya29na3hLdUllejhzb1BO?=
 =?utf-8?B?V0JodGUzMzRBUzc2T05LMGlrLy81SlBwa0oza01pU3c0VzBUczFWeHZJYzZH?=
 =?utf-8?B?R3NsM21DNm5zc1BTbFRaTFRmTkhBL3lrUnBDcUpWYWlHWTNwSlUrQUdJbWx4?=
 =?utf-8?B?cFRBY2dXbTQybTdIUGordlhRQ1RnTDRBUGJjUGsvQ0xBa0k3OHIzZXJ4aFAz?=
 =?utf-8?B?eEIzQ2x4MnFyWlRqb2kvZGNBTGlDWk95cTlkV3NhSnRhR3BjZHVUZDlqSjY2?=
 =?utf-8?B?eUNjbGNaTnpXWGkwWlhJN3psVVptMFJuejk3NTdNdWVxbFpkc0RvR2dHbTZK?=
 =?utf-8?B?LzJaK0hvMWZuTWkyVXNjMVV5QTZ2bGMwR0thZjNRQXE0WnVsaDdmdmlYNS9I?=
 =?utf-8?B?MXJyOXR1dXdpNGo2dG5WeHlveVJUK2xrRWJKUFZUMDltaDVlOGl4SWszcGFl?=
 =?utf-8?B?VGVQQStnTkJXZTdpaGF6dmVtVjg5SlJrQ25YQmhBbVJTbE1KZElvYXIyS1o2?=
 =?utf-8?B?dFhhd2MyNEFabmNwMUEzVlpVWlhHdG5BeTgzeWtObVdpbUtDS0lHM3l1b0VW?=
 =?utf-8?B?T20xL0JTdFRTZ3NKaXY2RG40eVgvbnR2b25LRC9WZXRCNU90c3dSbjh3Vy90?=
 =?utf-8?B?dmhSWnk4b3lWUmMvRFo2ZUF5SitydDhwaDNvcmRzMXBSWGNsbEhUQllVNXN0?=
 =?utf-8?B?VmdGT3dBWENGZjQ5Vng1K25zVHdOWGhsbTRVV2dPbmEyQ0dCZDQzUDF2ZUxp?=
 =?utf-8?B?MmdHUTlDSFZ6TFltSnR5NENSODFzVzhaNFZQSDVGcDBVQWI0MjFvNWM1SEln?=
 =?utf-8?B?Y3BvVGhEbWthZk1ZNTF4blVUY2w3OUZ6N2dIejNmRnZabWgxbmNNSkljNnVi?=
 =?utf-8?B?TDZzRGh5eStOV0JRQmlFVFBWRnJleGtvRnNsQjZGKytPNVRlR29XNDNsMFdT?=
 =?utf-8?B?OXhGRVlVcjNHQ21TTlF1Y0g5cmQ2WG1yZFhDTC92cWdKa3BzdmIrTU82T3Er?=
 =?utf-8?B?NzlJUFRGWjBZN210ZnhEdEJodnZrTU5kY1hwNlJyWjBraW1ydFlpamdOOUZj?=
 =?utf-8?B?ak8rV29sdTBiZkdEY0hCR3lKSW5rMURqVFJuQ2tWVWQzNFZzUDQ1bnduYWdw?=
 =?utf-8?B?by9UZm44b3V1SFl5T0ptT2Nqc0QyZzcvTVBwdTFGY214eHdsUjZlbE4wY2JO?=
 =?utf-8?B?RERtSkhvQ0tyQVRoS3BMRytqeGZtMzVRMFE4WEx2NzhubkNkcElSZWQ0NGZY?=
 =?utf-8?B?MVQwb0Y1YVI5T0ZsRzlPTlkveHVYQ3VEMCtyTjF4Z1g4dE1rZ1lLNS83MmpZ?=
 =?utf-8?B?N1dSK3Fnc2NNQzFaOFRnMXQwS3BrdEtpdFA4RXd2bm5QMlM0MG8xdlMyTXRs?=
 =?utf-8?B?bW5ydjk4aGpxVmswZHcvVVk2WXNxT1Z3Qy9nNzBRZUE1dkJHaGVnSEhHdFlM?=
 =?utf-8?B?U1laMkxNcmR0NTZVZ3FFdVJRQmt2V2VEZ3NZMzNnL1BsaVZ6SElQMDJVKy91?=
 =?utf-8?B?ZXBBaEs3QUpYNUhxRnBmZDB6dVdOZ3FSN011enR0YkR4UHJucWN1cVFkeHcy?=
 =?utf-8?B?Q0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 09575de0-5c99-4076-b701-08db5267b3db
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 May 2023 21:35:55.7777
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: oOEKb8kdOaCt3Bpte2tBaDeyBlXKQLoIiOzTGc52mTZL3r4Txkto58Ti0WJGnYm+hEYQu4Kforo+ojLIcwH2tOzrgANR4g+q5fybfOO3Wxs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB5964
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-6.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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