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: <364c2d8d-dc81-9ccc-51e8-92cd8db3c4ee@amd.com>
Date:   Tue, 10 Jan 2023 10:01:56 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH v3 2/2] x86/resctrl: Avoid redundant counter read in
 __mon_event_count()
Content-Language: en-US
To:     Peter Newman <peternewman@google.com>, reinette.chatre@intel.com,
        fenghua.yu@intel.com
Cc:     bp@alien8.de, dave.hansen@linux.intel.com, eranian@google.com,
        hpa@zytor.com, james.morse@arm.com, linux-kernel@vger.kernel.org,
        mingo@redhat.com, quic_jiles@quicinc.com, tan.shaopeng@fujitsu.com,
        tglx@linutronix.de, x86@kernel.org
References: <20221220164132.443083-1-peternewman@google.com>
 <20221220164132.443083-2-peternewman@google.com>
From:   "Moger, Babu" <babu.moger@amd.com>
In-Reply-To: <20221220164132.443083-2-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?Q29oZkN4Z1ZIdW0vYjVYMnl6SXNiWEVTSEp1VyttYmlua3FkeTRJUzZ0K2lj?=
 =?utf-8?B?RUcrbE4zOTFmZWFFREtLQXBYY3l6TjVZbENRZU5kaE00TngrYXVhSGZpRk9v?=
 =?utf-8?B?MUZ0NzFvQzN0alBGN3NDaWU5aXBZRXdHR2tOU2Z2eGcyVHZCcG9FZzAxclAr?=
 =?utf-8?B?TTcwTlpBVW9pcHNjK3ZxMmZlR0dYY0V5cS9xOGRWU21sQUd3WnZIaDErN3hQ?=
 =?utf-8?B?YTlDbnBQQ0xJa1F0RkhnVDlhanJ3Q1gxTVF5dE1McUJYYURCZHJkWXhFbkdC?=
 =?utf-8?B?cmd3SDBBVzZkUjFjbks2dXM3eTNKdWRLTkYxVFMwSVdxanppWFpFaFBhZTlk?=
 =?utf-8?B?dXB5aVFZdUFEMUNQLzdUcUZGbFRDNnN4cHF5TVA3WThXMmJuRVROcTd6Ykl1?=
 =?utf-8?B?bVJ0UlRyY2FKYTNJL0t4aktidGNNd2k0NkxHaGh0cHlndEY4dUxPZzh5aTE0?=
 =?utf-8?B?Mm5jazF6Z0JFUXlKeVRUY1Y5bWVuQmJ1QzAvUUdjcG04dy94N0dHb3hWRG1X?=
 =?utf-8?B?NHdEa2ZpYWJ2dmNLT1Q2bzNoWldTZzdCWWk0MURWczhoSmdCamVFTjhXRmdL?=
 =?utf-8?B?elJsWms5Z0NIUnU5eW9qNTh4d0k3ZkZTbzFyTmFndGdmVElJcmtyRS9nQlUy?=
 =?utf-8?B?ZE1HZkx4bGhLbTh6TDRDZ293d0piZElrYTRQYU03SWUveXo0cmo4bGlqN1lh?=
 =?utf-8?B?R09oelN1UmY4RWw5ajlzR1RKdkRZaVlLdWpHZGxzZC80dUZTU1BFci8reWxr?=
 =?utf-8?B?RVFLdTFTZlRFTjJodkpYQTVxZXA4N0NMMkcrWUhoQ2M5SlNaS2pZaWt2OTBQ?=
 =?utf-8?B?SnFQVGFtWGlzbFBKcmlJVjU4UnRKL093MTFBN0huMjR4bHYyWmk0TTRGc0NU?=
 =?utf-8?B?dnhlY3R6SGhvbW5IZmtHdjUxSXpUZEFXMTBlQjVPVlFjZVZ4ZWVpc0tmVE95?=
 =?utf-8?B?Zm00d2hIOGp0R014U1IySjFuZC95ay80anUvUjM0VWlXamsxeng4NjRhSnZB?=
 =?utf-8?B?TUVVZFRLU2VYWDJyZm1mOC9URy9SYkpLdEwvb2lWQUNwZnBCaVdSeVB4SkFQ?=
 =?utf-8?B?aGhxQXF2MHF4cmV6K2JUSjlVdmR1NGVLa0hEUzY4Q0pCZ1BvV3YrRmFXUlNN?=
 =?utf-8?B?NWV1MDZjL2dTcjZValdQM0ZMWFhUVmFjQzBoMXdTQk9zMGE3RE9JNUsvZGk0?=
 =?utf-8?B?WXlvNzQzUG9kem0vQVRUZWR0RUFGek9xNjdlMjFVUXY0RzROSVBIQ2V4RXU1?=
 =?utf-8?B?b2p3UzVib3V3Z1d2dlZDa0tsU3JxTUhnV3A4NXgxell6cXdVdHJOTUJYUjJE?=
 =?utf-8?B?enN3a0l2bmVpZEZoQTh0WWlsaHZ0SzNLM2IzZFYva2Noc3Z3cW9tVVpxREQ3?=
 =?utf-8?B?Z256M2g1MGpFSDg0RTVHTW5MU3BGZk9WTnBNQ0gzUU1ocllCY2FLYUdBMmtn?=
 =?utf-8?B?b1R1SjRYQjJqT3hHU2VRdGsvYzE3dlUxYWFtbW9naXF2VEI3eWprY2hVa0Vx?=
 =?utf-8?B?SWZaQlhXOGpBd1lBWmt1SEJVcklJSjVMWHBLcmhNYXdYN1NUTUd6ZXBXb1dQ?=
 =?utf-8?B?ZjlkaFM2UHNWbE5JZFA1SVJlOEYwMFhxK2JaRE04NE4wSzZpVWczSXd1TEov?=
 =?utf-8?B?QVlvV1B2MUtoUWpaNlNSRFZ1MWRDRFYzRDYvaGE1NUx1Z1cwVHZ1MWxiTjNp?=
 =?utf-8?B?UHgwL1ZzY0ErcXNURkV4Q2VaeFo5MW5WUXhHMzl5QjlzQ0lDM2Nla0UxemhW?=
 =?utf-8?B?T3hBUk84TUxhRjlvTzBHMSswTzJDbFE3TFJsa1h6NjI4NDRDR2tQTnVDZW1E?=
 =?utf-8?B?SlVPenAyRmQ2allXemd4YjNFSk40dWhTU1BtRkI1WTZ4ZHAvdDB1MU00QzFW?=
 =?utf-8?B?VG81N1E4TXBqMkIzaHJ4QS9XSUY1a1F2MURPcUFzMkxiTER3L2NMTFJMMkcx?=
 =?utf-8?B?T1VNRnNLSTBFYkpDZk9PakhaQVBCRnQ4a0lNck8za1V0c2YvZlkyYnpuQ2pB?=
 =?utf-8?B?Zks4b2ZoU09Lc3diMjhvYUVHSlpkWGV2b1M3QVNrdzdiY3VRQ2VTUVhvS3J6?=
 =?utf-8?B?REZ3SGNRaHRYaUh3cG4wc3RZM1ZoZWRqVkpJZ2R6R0VvQVpRNElUOWxGRmZp?=
 =?utf-8?Q?kOAwomB4CG8mG/rXj7t/ZVpaY?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 96a8a35f-bcae-4cc6-5000-08daf32401c9
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2023 16:02:00.7250
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 0mJGEbCxXMFop0c1TlVobCEPLLGgiCiXVhqwQWmDvid1PJf8cnUddj0LVkUGDbN4
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB8555
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS 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


On 12/20/22 10:41, Peter Newman wrote:
> __mon_event_count() does the per-RMID, per-domain work for
> user-initiated event count reads and the initialization of new monitor
> groups.
>
> In the initialization case, after resctrl_arch_reset_rmid() calls
> __rmid_read() to record an initial count for a new monitor group, it
> immediately calls resctrl_arch_rmid_read(). This re-read of the hardware
> counter is unnecessary and the following computations are ignored by the
> caller during initialization.
>
> Following return from resctrl_arch_reset_rmid(), just clear the
> mbm_state and return. This involves moving the mbm_state lookup into the
> rr->first case, as it's not needed for regular event count reads: the
> QOS_L3_OCCUP_EVENT_ID case was redundant with the accumulating logic at
> the end of the function.
>
> Signed-off-by: Peter Newman <peternewman@google.com>
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> v3:
>  - changelog clarifications suggested by Reinette
> v2:
>  (patch introduced)
>
> v2: https://lore.kernel.org/lkml/20221214160856.2164207-2-peternewman@google.com/
> ---
>  arch/x86/kernel/cpu/resctrl/monitor.c | 43 ++++++++++++---------------
>  1 file changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 77538abeb72a..e708df478077 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -366,41 +366,36 @@ void free_rmid(u32 rmid)
>  		list_add_tail(&entry->list, &rmid_free_lru);
>  }
>  
> +static struct mbm_state *get_mbm_state(struct rdt_domain *d, u32 rmid,
> +				       enum resctrl_event_id evtid)
> +{
> +	switch (evtid) {
> +	case QOS_L3_MBM_TOTAL_EVENT_ID:
> +		return &d->mbm_total[rmid];
> +	case QOS_L3_MBM_LOCAL_EVENT_ID:
> +		return &d->mbm_local[rmid];
> +	default:
> +		return NULL;
> +	}
> +}
> +
>  static int __mon_event_count(u32 rmid, struct rmid_read *rr)
>  {
>  	struct mbm_state *m;
>  	u64 tval = 0;
>  
> -	if (rr->first)
> +	if (rr->first) {
>  		resctrl_arch_reset_rmid(rr->r, rr->d, rmid, rr->evtid);
> +		m = get_mbm_state(rr->d, rmid, rr->evtid);
> +		if (m)
> +			memset(m, 0, sizeof(struct mbm_state));
> +		return 0;
> +	}
>  
>  	rr->err = resctrl_arch_rmid_read(rr->r, rr->d, rmid, rr->evtid, &tval);
>  	if (rr->err)
>  		return rr->err;
>  
> -	switch (rr->evtid) {
> -	case QOS_L3_OCCUP_EVENT_ID:
> -		rr->val += tval;
> -		return 0;
> -	case QOS_L3_MBM_TOTAL_EVENT_ID:
> -		m = &rr->d->mbm_total[rmid];
> -		break;
> -	case QOS_L3_MBM_LOCAL_EVENT_ID:
> -		m = &rr->d->mbm_local[rmid];
> -		break;
> -	default:
> -		/*
> -		 * Code would never reach here because an invalid
> -		 * event id would fail in resctrl_arch_rmid_read().
> -		 */
> -		return -EINVAL;
> -	}
> -
> -	if (rr->first) {
> -		memset(m, 0, sizeof(struct mbm_state));
> -		return 0;
> -	}
> -
>  	rr->val += tval;
>  
>  	return 0;

Tested the patches on AMD systems. Looks good.
Tested-by: Babu Moger <babu.moger@amd.com>

Thanks
Babu Moger