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: <c4088205-a344-5ef8-15e2-9ccc5d1691ea@intel.com>
Date:   Fri, 4 Mar 2022 16:27:00 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.6.1
Subject: Re: [PATCH v3 12/21] x86/resctrl: Calculate bandwidth from the
 previous __mon_event_count() chunks
Content-Language: en-US
To:     James Morse <james.morse@arm.com>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     Fenghua Yu <fenghua.yu@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        H Peter Anvin <hpa@zytor.com>,
        Babu Moger <Babu.Moger@amd.com>,
        <shameerali.kolothum.thodi@huawei.com>,
        Jamie Iles <jamie@nuviainc.com>,
        "D Scott Phillips OS" <scott@os.amperecomputing.com>,
        <lcherian@marvell.com>, <bobo.shaobowang@huawei.com>,
        <tan.shaopeng@fujitsu.com>
References: <20220217182110.7176-1-james.morse@arm.com>
 <20220217182110.7176-13-james.morse@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20220217182110.7176-13-james.morse@arm.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?R1pYRGgrNTFvV2hTREx2NWRDZGxiM1YwWUtvUG01Vmx5MklrQmdFazZiUVR3?=
 =?utf-8?B?cjlaYWFkb3JCaUFydG0zR1dnUFB6WFZNUVRKM1VoblY5TTBaQm1PQkRDYU5E?=
 =?utf-8?B?Z0Y4Q3hFRzM2a21tYUZsTjFUeGswdmJLUjNrRVRzWHFVQ3RhZEJzODEwM3VZ?=
 =?utf-8?B?aXBYT0xZM3lFK0JTeGdYQnVxeGdtd29IdnBvdTBxNms4dEdzU1NpekZnYk9B?=
 =?utf-8?B?SE9kanFxeDQrbkIxOHI0UU1acGIrNDZQNU9ZVSt6WlYxTndQT2lrLzJpSE5q?=
 =?utf-8?B?Mi9BRHM2VVpyemFNM0szdjRBYU9TeHN1YWJuSjBHZVJmSW9rVVdPMExjZUc1?=
 =?utf-8?B?NzFWa0NHRmliTDB3dHJCa3d4YnR5TFl1Tmt2RzFleWEvMjRHVkNWc1I4aVBT?=
 =?utf-8?B?NnpmUU0reDB2czVNTjdrVTBScVFadXRmSGJTbFJqdklQcmgrT3VEZVJCVFNW?=
 =?utf-8?B?d2FMcTBtVGk4K0xWWFBsdUk3NHlxWVZxZTJMUHp5WlpyMmgveTNtS3RwYXFJ?=
 =?utf-8?B?NmJabVlZd1R5OE05VFlpMFN0U2FSempuY1BlSjVFUUwwNVFFL0ZmRmJEMllY?=
 =?utf-8?B?VDJseUU4RXJVclNqOTBwTUk2MFpQSW5qUnI5YXNRNUJNV0lHYmpaMUF3Zkh4?=
 =?utf-8?B?QkppUGhVOUxIRnRSc3NjdndlM3o1RklGMVFrSCtNQXJFLzRTblBDRkw3K3FD?=
 =?utf-8?B?Y0tIZTdYQ0pYVGtaNmRyNzZMUFRKSnZia1Bod3IvdzE3Y0kyaW1pN0c1SElC?=
 =?utf-8?B?WCtBM2ZZSE0reG95MjZaams4c0JiY0xidlU4WW1JbURKNVZvcVR5YTFCZ0hV?=
 =?utf-8?B?L0lieEdaTW1SR3ZGZXJGblJ5QmNQZDN3ZlRGV3ZFcktuRkY2N2hBT1BhL29S?=
 =?utf-8?B?cFpJMG8xVWlTeTFkOWphblozRitEZ1ByTFNBWUQzekdvenBQUU5HMzdjbzc1?=
 =?utf-8?B?QXo3YlJwek9lTlZmWU5XcU5GMEFONnpoVDZlM3hZMGM4ZldsVzY1THRqWGhL?=
 =?utf-8?B?eld4UmJvTCtyUlN5enU2VWd6ajZDVGZKTTZtdll1T293NHVoTU9lVjVZSGtE?=
 =?utf-8?B?R0lOdXF5QThKbisrZWhSenJrdlhyT01KOGdPSFYxcUxSc2doRnA5TlFkZ2Nu?=
 =?utf-8?B?U01xUXBzbUpoczQ2MDF4TXE3dWZOeDlhays2OVQzOXdPcEdLdkJSWFY2bWUw?=
 =?utf-8?B?cGlaTVppbmExRnA5N0trVGZVczlBRWZyckU4R0RrdEdJMnZoRThkTDZmMDNI?=
 =?utf-8?B?RTU3SVNNN1dZeThpblYxQkZCQy9Mc09pU0ptOUdoYzBhY0k1RFNEU3hFczB1?=
 =?utf-8?B?UW9Ua3NKRjEvanBKOGsyU0lRQ3gwbEpuZnhKOTM3TU5QdS8vMU9YK1lTZGtz?=
 =?utf-8?B?VmdyaXUvZFhiL0dvamNGMlFxRmpzNEw3ZWV4bFJDY3U4emNHeXNQYzZXY2Zy?=
 =?utf-8?B?VHgxQTJrdkZpTFpINFR1dDI1dDJJNk5nUFV6YmFWaTNaRlZQN3Nmd2JZRmtB?=
 =?utf-8?B?Mks4Mm82RXQvZmllV2YzRkhUSWVUZjF0cGRFTGR1cU5GVzkzL2M2VmpUaE5p?=
 =?utf-8?B?NkQ1ZTVxalF0TVJaK3JOVy9rVmJWY0w1d0VnN0R3N25ZNkxjNk02Y3p2OUhD?=
 =?utf-8?B?b0h2Q29xVitBQnF5UG4xQzZLVFhNK2VRRkwxSHpIZE92Q1BPZ0hRTkN1TzJR?=
 =?utf-8?B?VHFaSG9XSjFFSlVVSE1ycFFpYXlNaWtjb0FZanZOaTJUelQ0aGNiTFNndjEw?=
 =?utf-8?B?UGpRMmlSN0hwM1diaiszTHlzL0xGeU9lNUFOWFZvSUFpNmVTZDVLUXd0N0hK?=
 =?utf-8?B?aHBCWklydzJrVkpOeTFWYk9xNlBBLy9pWVlaeVd1Q3RIaUtsa08ySjR4dDJr?=
 =?utf-8?B?YzJUbFRSOTBDY1V6a09uZ2p0c2xYRitMWVFFNFpYU3k3L0FpSGRzY3JGWjdo?=
 =?utf-8?B?Y29NMHNJUms5ZzV1T2wzMFhpUzRxUDRUdUZRVTRobEdxcTBmWUVEZnYwNVRU?=
 =?utf-8?B?b1dIeTNrVVBFVEloaUpqQVBJYjZtNHBrTnJZNTgxN1J6T0dLcWZiT1pHbFQw?=
 =?utf-8?B?VnlpNDhvV3FNV0tkMmc0SW12TVpiYVVMODBPRmN2Sll4WXp4UFpOWEFGY044?=
 =?utf-8?B?NmZXdERLTXQ3OEV5S3RyYXpNNUVNR2FCZW1UWk5zc0ZLdE53b014aXQ4SEFm?=
 =?utf-8?B?eGNLbTdZak4rWVI1aEdvemxqUUgzRmU4UnBGaWVaSlh4WmlSaXVQa3NPVGNs?=
 =?utf-8?B?WFFLSnhBYjJBemtsbHlJK2xXN1BRPT0=?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f8dc2996-d719-4e9d-c395-08d9fe3edfc2
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Mar 2022 00:27:04.7177
 (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: qJHD9KZNuq5mcb3WywfGn99oI3jMswiMy56eY6ul1jngDFnhDKzhUZg4JBCulyF5++JNfuIBdTQd+M6kSSxRJDYpdZhRhmiO+J48grEUero=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR11MB3010
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.8 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 James,

On 2/17/2022 10:21 AM, James Morse wrote:
> mbm_bw_count() is only called by the mbm_handle_overflow() worker once a
> second. It reads the hardware register, calculates the bandwidth and
> updates m->prev_bw_msr which is used to hold the previous hardware register
> value.
> 
> Operating directly on hardware register values makes it difficult to make
> this code architecture independent, so that it can be moved to /fs/,
> making the mba_sc feature something resctrl supports with no additional
> support from the architecture.
> Prior to calling mbm_bw_count(), mbm_update() reads from the same hardware
> register using __mon_event_count().
> 
> Change mbm_bw_count() to use the current chunks value most recently saved by
> __mon_event_count(). This removes an extra call to __rmid_read().

> Instead of using m->prev_msr to calculate the number of chunks seen,
> use the rr->val that was updated by __mon_event_count(). This removes a extra
> calls to mbm_overflow_count() and get_corrected_mbm_count().

"removes a extra calls" -> "removes an extra call" ?

__mon_event_count() ends with "rr->val += get_corrected_mbm_count()" and
it is called twice by mbm_update(). The intention in this change is for
mbm_bw_count() to benefit from the rmid read done just before ...
but would using rr->val within mbm_bw_count() not result in it getting
data from both rmid reads due to the increment?

mbm_update() {

	rr.val = 0;

	if (is_mbm_total_enabled()) {
		...
		__mon_event_count() /* rr->val += ... */
	}
	if (is_mbm_local_enabled()) {
		...
		__mon_event_count() /* rr->val += ... */

		if (is_mba_sc(NULL))
			mbm_bw_count(rmid, &rr); /* Use rr-> val */
	}


Should rr.val perhaps be reset before each __mon_event_count() call instead of
just at the beginning of mbm_update()?

> Calculating bandwidth like this means mbm_bw_count() no longer operates
> on hardware register values directly.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> Changes since v2:
>  * Expanded commit message
> 
> Changes since v1:
>  * This patch was rewritten
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |  4 ++--
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 24 +++++++++++++++---------
>  2 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 3b9e43ba7590..c50c8911ef59 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -289,7 +289,7 @@ struct rftype {
>   * struct mbm_state - status for each MBM counter in each domain
>   * @chunks:	Total data moved (multiply by rdt_group.mon_scale to get bytes)
>   * @prev_msr:	Value of IA32_QM_CTR for this RMID last time we read it
> - * @prev_bw_msr:Value of previous IA32_QM_CTR for bandwidth counting
> + * @prev_bw_chunks: Previous chunks value read when for bandwidth calculation

"value read when for" -> "value read for"?

>   * @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
> @@ -297,7 +297,7 @@ struct rftype {
>  struct mbm_state {
>  	u64	chunks;
>  	u64	prev_msr;
> -	u64	prev_bw_msr;
> +	u64	prev_bw_chunks;
>  	u32	prev_bw;
>  	u32	delta_bw;
>  	bool	delta_comp;
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index ac1a2e8998bb..8ae375e29256 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -315,7 +315,7 @@ static u64 __mon_event_count(u32 rmid, struct rmid_read *rr)
>  
>  	if (rr->first) {
>  		memset(m, 0, sizeof(struct mbm_state));
> -		m->prev_bw_msr = m->prev_msr = tval;
> +		m->prev_msr = tval;
>  		return 0;
>  	}
>  
> @@ -329,27 +329,32 @@ static u64 __mon_event_count(u32 rmid, struct rmid_read *rr)
>  }
>  
>  /*
> + * mbm_bw_count() - Update bw count from values previously read by
> + *		    __mon_event_count().
> + * @rmid:	The rmid used to identify the cached mbm_state.
> + * @rr:		The struct rmid_read populated by __mon_event_count().
> + *
>   * Supporting function to calculate the memory bandwidth
> - * and delta bandwidth in MBps.
> + * and delta bandwidth in MBps. The chunks value previously read by
> + * __mon_event_count() is compared with the chunks value from the previous
> + * invocation. This must be called oncer per second to maintain values in MBps.

"must be called oncer" -> "must be called once"

>   */
>  static void mbm_bw_count(u32 rmid, struct rmid_read *rr)
>  {
>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(rr->r);
>  	struct mbm_state *m = &rr->d->mbm_local[rmid];
> -	u64 tval, cur_bw, chunks;
> +	u64 cur_bw, chunks, cur_chunks;
>  
> -	tval = __rmid_read(rmid, rr->evtid);
> -	if (tval & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
> -		return;
> +	cur_chunks = rr->val;
> +	chunks = cur_chunks - m->prev_bw_chunks;
> +	m->prev_bw_chunks = cur_chunks;
>  
> -	chunks = mbm_overflow_count(m->prev_bw_msr, tval, hw_res->mbm_width);
> -	cur_bw = (get_corrected_mbm_count(rmid, chunks) * hw_res->mon_scale) >> 20;
> +	cur_bw = (chunks * hw_res->mon_scale) >> 20;
>  
>  	if (m->delta_comp)
>  		m->delta_bw = abs(cur_bw - m->prev_bw);
>  	m->delta_comp = false;
>  	m->prev_bw = cur_bw;
> -	m->prev_bw_msr = tval;
>  }
>  
>  /*
> @@ -509,6 +514,7 @@ static void mbm_update(struct rdt_resource *r, struct rdt_domain *d, int rmid)
>  	rr.first = false;
>  	rr.r = r;
>  	rr.d = d;
> +	rr.val = 0;
>  
>  	/*
>  	 * This is protected from concurrent reads from user

Reinette