Re: [PATCH v8 10/13] x86/resctrl: Add sysfs interface to write mbm_total_bytes_config

From: "Moger, Babu" <babu.moger@amd.com>
To: Peter Newman <peternewman@google.com>
Cc: akpm@linux-foundation.org, bagasdotme@gmail.com, bp@alien8.de,
	chang.seok.bae@intel.com, corbet@lwn.net,
	damien.lemoal@opensource.wdc.com, daniel.sneddon@linux.intel.com,
	dave.hansen@linux.intel.com, eranian@google.com,
	fenghua.yu@intel.com, hpa@zytor.com, james.morse@arm.com,
	jmattson@google.com, jpoimboe@kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	mingo@redhat.com, paulmck@kernel.org,
	pawan.kumar.gupta@linux.intel.com, pbonzini@redhat.com,
	peterz@infradead.org, quic_neeraju@quicinc.com,
	rdunlap@infradead.org, reinette.chatre@intel.com,
	sandipan.das@amd.com, songmuchun@bytedance.com,
	tglx@linutronix.de, tony.luck@intel.com, x86@kernel.org
Subject: Re: [PATCH v8 10/13] x86/resctrl: Add sysfs interface to write mbm_total_bytes_config
Date: Mon, 7 Nov 2022 12:50:35 -0600	[thread overview]
Message-ID: <1cc4ffae-3d36-b369-7cc9-b7130701b944@amd.com> (raw)
In-Reply-To: <20221107102134.255757-1-peternewman@google.com>

Hi Peter,

On 11/7/22 04:21, Peter Newman wrote:
> Hi Babu,
>
> On Fri, Nov 04, 2022 at 03:01:09PM -0500, Babu Moger wrote:
>> +	/*
>> +	 * When an Event Configuration is changed, the bandwidth counters
>> +	 * for all RMIDs and Events will be cleared by the hardware. The
>> +	 * hardware also sets MSR_IA32_QM_CTR.Unavailable (bit 62) for
>> +	 * every RMID on the next read to any event for every RMID.
>> +	 * Subsequent reads will have MSR_IA32_QM_CTR.Unavailable (bit 62)
>> +	 * cleared while it is tracked by the hardware. Clear the
>> +	 * mbm_local and mbm_total counts for all the RMIDs.
>> +	 */
>> +	memset(d->mbm_local, 0, sizeof(struct mbm_state) * r->num_rmid);
>> +	memset(d->mbm_total, 0, sizeof(struct mbm_state) * r->num_rmid);
> Looking around, I can't find a reader for mbm_total anymore. It looks
> like the last place it was used went away in James's recent change:
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220902154829.30399-19-james.morse%40arm.com&amp;data=05%7C01%7Cbabu.moger%40amd.com%7C84a9d0f934894a3031a608dac0a9db33%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638034133003350939%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=i3isjCzxnBp4b2VblC7ZpH3hShUEe7unKiKfngG1kzE%3D&amp;reserved=0
>
> Are we supposed to be clearing arch_mbm_total now?

Yes. You are right. We should be using resctrl_arch_reset_rmid to reset
the rmids here.

This patch should work. Will fix it in next revision after the other comments.

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c             │
index 6b222f8e58ae..28d9d99a639e
100644                                                                  │
---
a/arch/x86/kernel/cpu/resctrl/rdtgroup.c                                                            
│
+++
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c                                                            
│
@@ -1517,7 +1517,7 @@ static int mbm_config_write(struct rdt_resource *r,
struct rdt_domain *d,          │
                            u32 evtid, u32
val)                                                          │
 {                                                                                                      
│
        struct mon_config_info mon_info =
{0};                                                           │
-       int ret =
0;                                                                                    
│
+       int ret = 0,
i;                                                                                 
│
                                                                                                        
│
       
rdt_last_cmd_clear();                                                                           
│
                                                                                                        
│
@@ -1557,8 +1557,10 @@ static int mbm_config_write(struct rdt_resource *r,
struct rdt_domain *d,         │
         * cleared while it is tracked by the hardware. Clear
the                                        │
         * mbm_local and mbm_total counts for all the
RMIDs.                                             │
        
*/                                                                                             
│
-       memset(d->mbm_local, 0, sizeof(struct mbm_state) *
r->num_rmid);                                 │
-       memset(d->mbm_total, 0, sizeof(struct mbm_state) *
r->num_rmid);                                 │
+       for (i = 0; i < r->num_rmid; i++)
{                                                              │
+               resctrl_arch_reset_rmid(r, d, i,
QOS_L3_MBM_TOTAL_EVENT_ID);                             │
+               resctrl_arch_reset_rmid(r, d, i,
QOS_L3_MBM_LOCAL_EVENT_ID);                             │
+      
}                                                                                               
│
                                                                                                        
│
 write_exit:                                                                                            
│
        return ret;                                   


Thanks

Babu

