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: <38b9e6df-cccd-a745-da4a-1d1a0ec86ff3@intel.com>
Date:   Thu, 11 May 2023 14:37:08 -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 3/9] x86/resctrl: Add resctrl_mbm_flush_cpu() to
 collect CPUs' MBM events
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-4-peternewman@google.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230421141723.2405942-4-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?RStuZVVNTWV0WE5CbW5qMUJXWlp6OFFhSFpiTUdQZ1p6QlpnbGI5TDMydVgz?=
 =?utf-8?B?KzhDdC9MSUJNc0xpMThUa2gzck1zQitYbXo3azZscFowWTk0eHFoTmdiNTNQ?=
 =?utf-8?B?dXlFc2hpdWxBdG9RMGt5VjNJeC82ai9uSlRKZXpKU21FMlRFMGtYQTVaeXgx?=
 =?utf-8?B?TWZQUlI4SHViM3BqcUYvWGdPUXJsSk1IWjZmWG9pei9DdU10UlB6SVZ3Vmgv?=
 =?utf-8?B?Y2FEZjJvNDRRSEc2aTE2bnN4Umg2ZEZHMWJNdEFrT1dYelo3Mmd6S2JySHJO?=
 =?utf-8?B?b1hvT09nYUVkNUx2TjV5WUk5d0R3S04vWHcxTFJUaTJKaDQ0em0zd0ZxaFAr?=
 =?utf-8?B?QVprTThwekt2NWhWT0pMeHp1YlpLYnVCVjJ6VktGUGozNmFJSVRoYlNJVWJN?=
 =?utf-8?B?NTlCWUQwbytIdmxlVnd5cG11aytIU05VWVhoYlBPOXdubkF1ZkRNUWZCMnQr?=
 =?utf-8?B?Z2wxcXg3WnJ1M242NUFHc2xwbDROVHQxaDQ5SHpRRUxwMmtZMTg1alh5cW5m?=
 =?utf-8?B?R2tUTlNoZHhWV1dmd3FDbklGeWs2WUVlSmoxRUNMemp6SEVKV0hvZ090b2xr?=
 =?utf-8?B?Sk1uKzdLVWRZK0tGMWxtSjY2dDd2RTQxbkNQQThoY3V2VGttUGZ5M3BubGUv?=
 =?utf-8?B?dVJxOXlPUEZFWVBMR2RPM09yRVVITzkzZ2JpRGpGdDk5ZE4zT0FsdDI3VjZB?=
 =?utf-8?B?cjdHb0txQkVQeFpzdG82b2o3clNjSW1NbzFxV0tTcHkyUE45MjFDalY5Ulcz?=
 =?utf-8?B?SzdNdXdPNXhITXhDUmdwV0gxeGJrd3JzL0RKM1U5b24yeXNwQ1ZjTnc5R0o2?=
 =?utf-8?B?VTgvODU0UkVVaDhEWTg2MVYzcE1HQmV4VnZjY2hOdERZRWptVm40TzFyeHJY?=
 =?utf-8?B?YXNkQTE1VnVxWUhRMWt2V3lNZkh3Qit2YVNtUGdJamJXUTAwdCtlcGJjaXFN?=
 =?utf-8?B?TjVOdkM0TUNJNlFnVUE2ZGwreGVqSFhRWmc0alVZSlJSUW1wQUtMTWl3Q0Zl?=
 =?utf-8?B?NnlMYnEzVUF4elFFaWNHMWkwV3hrUktCY3RnMUxjR0RsaFJwRjdCNnk5L3R3?=
 =?utf-8?B?M0tUTHAramFhOCtmdnRIa1o5Z0RqWnhzZ2szVFVkS2NaTjhrN0FJaFJNQVJj?=
 =?utf-8?B?TmYySmNUMGplL202VklEQWluZnRITTAxS3pkN2dja0xtbkZNZjBnazd0S1Nt?=
 =?utf-8?B?YnphU3RsK29hUG5zek16dTNGVTZTQmpNdnJsUTRiQ0NWcUNMcTB0QjlreVky?=
 =?utf-8?B?MmEyZ0RzZzEzODZoczNYcjloSjZLckczdnIwNDJ6K3ZxUEdjZkhxcmh0dkJo?=
 =?utf-8?B?eG1zVS8wYmM0VU5QOFFhL0htMXNFUGlGR2U1ZmcvRVhHdXltZjUxQjJMWUR3?=
 =?utf-8?B?WUkrb1lrekdBeW9uQ2dBZEdoQXlnaDAzOXJ5ai9aOHVDMy84NTBVVFFoTEhX?=
 =?utf-8?B?VVRZWDBPVFdiWlVkeENrcUp1YzU4UjBWbGZ3UnFkeng1aGxKS0J5OGZsMXNr?=
 =?utf-8?B?SjJFTlhoU2lwOHl6U2hyL3ZxQzhhY1MxOHdXbEx6WmwvWFdtdEdSV0ZIV1NO?=
 =?utf-8?B?WDNwWURZY1hYVlBPeTNRaE5BbEZpMlZYeDl4YnBFQkZvYzV6djZSMXZWZWgz?=
 =?utf-8?B?elMwcFJ1aWpsTFdBdGhWeEVtTGk0cUw0QXlBZ2xISFlsMzJPZTgvWFNBRjQw?=
 =?utf-8?B?U1pDc0UrZkJVcGNpVTF6VzBhOWllRXNuRG9IM1hlV2ZRcGZmZkhaWlZVaEhz?=
 =?utf-8?B?aXd0VnhaR1dWREx2Uk52cHpvNm1IMnpXQWcxUy8rUDdVUkNEbW04TjRkNDNq?=
 =?utf-8?B?dUF0dEhidzI0QVB3VS9ZSVpmZHY0RVorSHVwMFd3N3ZWWkRiaThqR2FnNHhj?=
 =?utf-8?B?S1BZTU9oVFdVYmg3N205eVhGaU5EY3lhRktmbXlLRFJtSFpGbWRXTi9xWGor?=
 =?utf-8?B?MmtFREpjU0hORHlsZzI3b01WU0lyeXBNc2QycmRzallud2o0eTFqblBROXAy?=
 =?utf-8?B?VmcwZGdlZ2tnRWppTW9wQUFxeWpLdE52bXJhOFdIRjBzT3RlRnBPWHJOL1hR?=
 =?utf-8?B?dll0SlVoMWxaekdYVm1RWU1zRmdsMGN5ajM5eGlRZWRBUjBrVjJkZzl1dWZJ?=
 =?utf-8?B?MmJiMkp1bzVJcTBRTnQrQzlvNXozNUVCejFNQzgzMUh4clQ2OVZ5QmFlbkVB?=
 =?utf-8?B?b3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c1c115f8-83bd-4de8-7b61-08db5267e0a5
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:37:10.8643
 (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: yoOQdumUnirpNnI+VdlAbkLesyBuK6AatDxXfBBNODIMckHY5Rksxja5cmTbixntqjWp7UszKYEGDGHYnnNCsbhkfpkfrbzEnLJFpfehMkI=
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,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,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:
> AMD implementations so far are only guaranteed to provide MBM event
> counts for RMIDs which are currently assigned in CPUs' PQR_ASSOC MSRs.
> Hardware can reallocate the counter resources for all other RMIDs' which
> are not currently assigned to those which are, zeroing the event counts
> of the unassigned RMIDs.
> 
> In practice, this makes it impossible to simultaneously calculate the
> memory bandwidth speed of all RMIDs on a busy system where all RMIDs are
> in use. Over a multiple-second measurement window, the RMID would need
> to remain assigned in all of the L3 cache domains where it has been
> assigned for the duration of the measurement, otherwise portions of the
> final count will be zero. In general, it is not possible to bound the
> number of RMIDs which will be assigned in an L3 domain over any interval
> of time.
> 
> To provide reliable MBM counts on such systems, introduce "soft" RMIDs:
> when enabled, each CPU is permanently assigned a hardware RMID whose
> event counts are flushed to the current soft RMID during context
> switches which result in a change in soft RMID as well as whenever
> userspace requests the current event count for a domain.
> 
> Implement resctrl_mbm_flush_cpu(), which collects a domain's current MBM
> event counts into its current software RMID. The delta for each CPU is
> determined by tracking the previous event counts in per-CPU data.  The
> software byte counts reside in the arch-independent mbm_state
> structures.

Could you elaborate why the arch-independent mbm_state was chosen? 

> 
> Co-developed-by: Stephane Eranian <eranian@google.com>
> Signed-off-by: Stephane Eranian <eranian@google.com>
> Signed-off-by: Peter Newman <peternewman@google.com>
> ---
>  arch/x86/include/asm/resctrl.h         |  2 +
>  arch/x86/kernel/cpu/resctrl/internal.h | 10 ++--
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 78 ++++++++++++++++++++++++++
>  3 files changed, 86 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> index 255a78d9d906..e7acf118d770 100644
> --- a/arch/x86/include/asm/resctrl.h
> +++ b/arch/x86/include/asm/resctrl.h
> @@ -13,6 +13,7 @@
>   * @cur_closid:	The cached Class Of Service ID
>   * @default_rmid:	The user assigned Resource Monitoring ID
>   * @default_closid:	The user assigned cached Class Of Service ID
> + * @hw_rmid:	The permanently-assigned RMID when soft RMIDs are in use
>   *
>   * The upper 32 bits of MSR_IA32_PQR_ASSOC contain closid and the
>   * lower 10 bits rmid. The update to MSR_IA32_PQR_ASSOC always
> @@ -27,6 +28,7 @@ struct resctrl_pqr_state {
>  	u32			cur_closid;
>  	u32			default_rmid;
>  	u32			default_closid;
> +	u32			hw_rmid;
>  };
>  
>  DECLARE_PER_CPU(struct resctrl_pqr_state, pqr_state);
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 02a062558c67..256eee05d447 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -298,12 +298,14 @@ struct rftype {
>   * @prev_bw:	The most recent bandwidth in MBps
>   * @delta_bw:	Difference between the current and previous bandwidth
>   * @delta_comp:	Indicates whether to compute the delta_bw
> + * @soft_rmid_bytes: Recent bandwidth count in bytes when using soft RMIDs
>   */
>  struct mbm_state {
> -	u64	prev_bw_bytes;
> -	u32	prev_bw;
> -	u32	delta_bw;
> -	bool	delta_comp;
> +	u64		prev_bw_bytes;
> +	u32		prev_bw;
> +	u32		delta_bw;
> +	bool		delta_comp;
> +	atomic64_t	soft_rmid_bytes;
>  };
>  
>  /**
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 2de8397f91cd..3671100d3cc7 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -404,6 +404,84 @@ static struct mbm_state *get_mbm_state(struct rdt_domain *d, u32 rmid,
>  	}
>  }
>  
> +struct mbm_soft_counter {
> +	u64	prev_bytes;
> +	bool	initialized;
> +};
> +
> +struct mbm_flush_state {
> +	struct mbm_soft_counter local;
> +	struct mbm_soft_counter total;
> +};
> +
> +DEFINE_PER_CPU(struct mbm_flush_state, flush_state);
> +

Why not use the existing MBM state? 

> +/*
> + * flushes the value of the cpu_rmid to the current soft rmid
> + */
> +static void __mbm_flush(int evtid, struct rdt_resource *r, struct rdt_domain *d)
> +{
> +	struct mbm_flush_state *state = this_cpu_ptr(&flush_state);
> +	u32 soft_rmid = this_cpu_ptr(&pqr_state)->cur_rmid;
> +	u32 hw_rmid = this_cpu_ptr(&pqr_state)->hw_rmid;
> +	struct mbm_soft_counter *counter;
> +	struct mbm_state *m;
> +	u64 val;
> +
> +	/* cache occupancy events are disabled in this mode */
> +	WARN_ON(!is_mbm_event(evtid));

If this is hit it would trigger a lot, perhaps WARN_ON_ONCE?

> +
> +	if (evtid == QOS_L3_MBM_LOCAL_EVENT_ID) {
> +		counter = &state->local;
> +	} else {
> +		WARN_ON(evtid != QOS_L3_MBM_TOTAL_EVENT_ID);
> +		counter = &state->total;
> +	}
> +
> +	/*
> +	 * Propagate the value read from the hw_rmid assigned to the current CPU
> +	 * into the "soft" rmid associated with the current task or CPU.
> +	 */
> +	m = get_mbm_state(d, soft_rmid, evtid);
> +	if (!m)
> +		return;
> +
> +	if (resctrl_arch_rmid_read(r, d, hw_rmid, evtid, &val))
> +		return;
> +

This all seems unsafe to run without protection. The code relies on
the rdt_domain but a CPU hotplug event could result in the domain
disappearing underneath this code. The accesses to the data structures
also appear unsafe to me. Note that resctrl_arch_rmid_read() updates
the architectural MBM state and this same state can be updated concurrently
in other code paths without appropriate locking.

> +	/* Count bandwidth after the first successful counter read. */
> +	if (counter->initialized) {
> +		/* Assume that mbm_update() will prevent double-overflows. */
> +		if (val != counter->prev_bytes)
> +			atomic64_add(val - counter->prev_bytes,
> +				     &m->soft_rmid_bytes);
> +	} else {
> +		counter->initialized = true;
> +	}
> +
> +	counter->prev_bytes = val;

I notice a lot of similarities between the above and the software controller,
see mbm_bw_count(). 

> +}
> +
> +/*
> + * Called from context switch code __resctrl_sched_in() when the current soft
> + * RMID is changing or before reporting event counts to user space.
> + */
> +void resctrl_mbm_flush_cpu(void)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +	int cpu = smp_processor_id();
> +	struct rdt_domain *d;
> +
> +	d = get_domain_from_cpu(cpu, r);
> +	if (!d)
> +		return;
> +
> +	if (is_mbm_local_enabled())
> +		__mbm_flush(QOS_L3_MBM_LOCAL_EVENT_ID, r, d);
> +	if (is_mbm_total_enabled())
> +		__mbm_flush(QOS_L3_MBM_TOTAL_EVENT_ID, r, d);
> +}

This (potentially) adds two MSR writes and two MSR reads to what could possibly
be quite slow MSRs if it was not designed to be used in context switch. Do you
perhaps have data on how long these MSR reads/writes take on these systems to get
an idea about the impact on context switch? I think this data should feature
prominently in the changelog.

> +
>  static int __mon_event_count(u32 rmid, struct rmid_read *rr)
>  {
>  	struct mbm_state *m;


Reinette