Received-SPF: pass (google.com: domain of linux-kernel+bounces-214198-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <9141ea08-5109-4c61-880b-a5136405878e@intel.com>
Date: Thu, 13 Jun 2024 18:51:47 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 17/19] x86/resctrl: Introduce the interface switch
 between ABMC and mbm_legacy
To: Babu Moger <babu.moger@amd.com>, <corbet@lwn.net>, <fenghua.yu@intel.com>,
	<tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
	<dave.hansen@linux.intel.com>
CC: <x86@kernel.org>, <hpa@zytor.com>, <paulmck@kernel.org>,
	<rdunlap@infradead.org>, <tj@kernel.org>, <peterz@infradead.org>,
	<yanjiewtw@gmail.com>, <kim.phillips@amd.com>, <lukas.bulwahn@gmail.com>,
	<seanjc@google.com>, <jmattson@google.com>, <leitao@debian.org>,
	<jpoimboe@kernel.org>, <rick.p.edgecombe@intel.com>,
	<kirill.shutemov@linux.intel.com>, <jithu.joseph@intel.com>,
	<kai.huang@intel.com>, <kan.liang@linux.intel.com>,
	<daniel.sneddon@linux.intel.com>, <pbonzini@redhat.com>,
	<sandipan.das@amd.com>, <ilpo.jarvinen@linux.intel.com>,
	<peternewman@google.com>, <maciej.wieczor-retman@intel.com>,
	<linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<eranian@google.com>, <james.morse@arm.com>
References: <cover.1716552602.git.babu.moger@amd.com>
 <0148e3317b0de3b5d1eaa58af37a2910783a69c9.1716552602.git.babu.moger@amd.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <0148e3317b0de3b5d1eaa58af37a2910783a69c9.1716552602.git.babu.moger@amd.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N1JieHVva2FDREpFc1lmcEpkcFpERkRUV01ZbGhBYllMbDlLRmt3cm0vZ3c3?=
 =?utf-8?B?NXZMcHlxckM0VmNzcTZsWVc4dmUxLzJKZW5hY3lLV2NjZFROb01OT2JzczJI?=
 =?utf-8?B?enRxL3ZocXZyeEUyajRLWlFNWjkvUVZMRUtadG5LNUFHRW1qakJkR3NieG5D?=
 =?utf-8?B?QkhxckZhaksrbnYzS1JuSUdwWlo1emhQMzl1OTdFRjdGN3BsQ1BRQlRLbGwx?=
 =?utf-8?B?ek1UOXBFZHhuVFVpMTJKdFZscVVUN1ZvMkoxWGFuNWJhOXFvNk00aGlJcUFy?=
 =?utf-8?B?UEJ1ckQ1cXUvT0FZZkI3WCtsYUc0UjVQSFhRbUhvdkQrVE05bWpzRVRjOFBm?=
 =?utf-8?B?TTVPdFNmWnE1L0tHeTJMZElpZzhLNWFXZTg1QzRxL24rN2dkWGRjNWh6MWZv?=
 =?utf-8?B?Y2Ewdk5iRkxodTI3dzBNaG9hd2EyYzAyV0N6SUNXUlJrUmVBZGUva0lhZjZT?=
 =?utf-8?B?bzAwVmNJTzhBTVhNeXdtMkxlc1d4YVVKVk9nRnU0U3BFVzRNSGlEeGV4QnJU?=
 =?utf-8?B?LzZiRm1HOXBJSTdtTVJRcTdPVndzdlpNUTJDTWdqdGtxZUV6eTFySDVHVy93?=
 =?utf-8?B?c1RobkZKZU1MVmUwS2RxRkorMzAyNUJyQmRjeStYSVByU2d0UVBIWnVYRUpp?=
 =?utf-8?B?dUJhbE9BT0g2TTRtc3l1UU5TK2VJekh4K3R5WnlOVlQrOHdsTW5nczVuZkRF?=
 =?utf-8?B?TkNaMklOTWplcnJlMzJibHFWRDRSY3lSTi8zV3NkMTVDQUswMWRPRE4xZXlU?=
 =?utf-8?B?U0ZhbUJLdTBQOWlLQnBHNk9Ha1Jpd1hoVHN0Ykl0Njc4alE1ck9GcDNZdTg5?=
 =?utf-8?B?b29qekJ1M1FWWisrYm15VlltMk0rdUFGVUxqRDMra0s5c2xRMjdZVGNPMUJZ?=
 =?utf-8?B?bTVLc3FTNGpnSmdicHhGbWMxYllUUnhGZGJaWTlhOVQvZmpwVWdRUlBVWDM1?=
 =?utf-8?B?OVNwSTVRQUpxK05PRmFXd3k0Zm16ZjE2U3ErSEt4SEIxak1CN2VubW9aTXBq?=
 =?utf-8?B?VjkrZmMzdCtnTUh0RDVYbWprdFl6aGpvNnNsOXNGZWg3QkVGZGpUcWRJMDM3?=
 =?utf-8?B?aURjWWlFayttUWpVdS9LcG9Oam5BWkJOd3VLRXY2ajRFSTZBZTlxZG5Bb3li?=
 =?utf-8?B?V29wVmR0MEk0TDdrb0lMY0NsK1IwWjZIcW1kNHFsViswSVJIbDRweHRaUWR5?=
 =?utf-8?B?RHQ3VEZ3cFZEVXlLZ01qWVhiQ05NaWVGZmgwVGRaMVpXK0tvSXUvSEtZZWVZ?=
 =?utf-8?B?UmRVTzkycTR3aE1XQ1Raa3UxV0Q0V3dMY0kzMWhZWmEzaWZBTkFYMG5zV2Jm?=
 =?utf-8?B?TjM5OFFrN1JwWVdPOWY5VVM3cUprNTFSME9HSi8ySHVRbWUwRlVzRVUyOUk1?=
 =?utf-8?B?UHV1clRHOEUrSnlBc2NCN1NGSzdHZlBqY3oxQlRnWTg3NTBTdUdLOEhvbDcw?=
 =?utf-8?B?OGd5ODF0aGdkZ3B2aDJ1eEtjaHBndEppRzV5N0xLakxOc3o3Q3JjeFRaMUMv?=
 =?utf-8?B?bnlQTVRFOXNEQkx4SjB6WkEzejB2T1BYNkJZeUpxOHhxS3AvYU5OcDFxcmg4?=
 =?utf-8?B?cDZCOGJRVUIwS0VpVytZWmlLOUY2MWlkbW44c2FFekg2ZWp3TEVVUnkvems0?=
 =?utf-8?B?ekhBMUxkNXBCVFI2cUx2STJZaWoycGsvMVFlMXRIbmQyNTlZZkJKOUNNSWoy?=
 =?utf-8?B?YnBtTW9sc1ViV25vT0M4WlJIRHZmUEsxWWpTczhTNGgvUFVvK3dHNzkzd3BU?=
 =?utf-8?B?RlVkTGFJL1NBUVRvb3Y2N3BSUkRobGxrNzV2SmFranJaMUVVWTB5KzhpSlZF?=
 =?utf-8?B?eUtWQUNSY0phQSs5VlJEVVNsa0N6ckgwRU5NdmNaZ3lQcEg2YzRJMTRITW9n?=
 =?utf-8?B?UHJPbi9HLzBIVTg5VVNYV1dNV0kyWFFiektKWlpOOXFoMWZSUmhnQ1pQWHpV?=
 =?utf-8?B?Y3dySm1iWkZKdjdaemZsMnBrT2ZuQktJaVBSWUVrRVk2cFBkbEZkRnNJVkU4?=
 =?utf-8?B?U2dmN1J1MlQ5UWl5UlZZSFBrL0pFK2dKQ0xiNVJIUGZIT2lwd0RHczNKUEdO?=
 =?utf-8?B?OTJRM0FPRTkxOGtFSTZ3YkdGOVAvL05aTElHaUd5c0FaOFdrZXRvRkZSSHRG?=
 =?utf-8?B?MGt4dGJ5a25yd3JqRkFIQlhyenZrYnVKWmlpRzNhWkhRd3JFNFdNc3hidkFi?=
 =?utf-8?B?L1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fa943014-75dd-4ecc-d2d4-08dc8c149113
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2024 01:51:54.2204
 (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: QFfnE7dV9FQtHnwgS0qV9/y1Oh4bjjkF90Pld/Pz8igwVoPjKfkHKmK5UndDEckplOVIB6ey8qK6i+fFwMgB+XNqOjGnarW+B9tcsN+R7yM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4974
X-OriginatorOrg: intel.com

Hi Babu,

On 5/24/24 5:23 AM, Babu Moger wrote:
> Introduce interface to switch between ABMC and legacy_mbm modes.

shortlog and first sentence of changelog do not match: mbm_legacy vs legacy_mbm?

> 
> By default ABMC is enabled on resctrl mount if the feature is available.
> However, user will have the option to go back to legacy_mbm if required.
> 
> $ cat /sys/fs/resctrl/info/L3_MON/mbm_assign
> [abmc]
> mbm_legacy
> 
> To enable the legacy monitoring feature:
> $ echo  "mbm_legacy" > /sys/fs/resctrl/info/L3_MON/mbm_assign

Missing information about user visible impact to counters/events
and any mitigations needed in implementation.

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v4: Minor commit text changes. Keep the default to ABMC when supported.
> 
> v3: New patch to address the review comments from upstream.
> ---
>   Documentation/arch/x86/resctrl.rst     | 10 +++++++
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 40 +++++++++++++++++++++++++-
>   2 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index ab3cde61a124..fd050d4d22cd 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -271,6 +271,16 @@ with the following files:
>   	  [abmc]
>   	  mbm_legacy
>   
> +	* To enable ABMC feature:
> +	  ::
> +
> +	    # echo  "abmc" > /sys/fs/resctrl/info/L3_MON/mbm_assign
> +
> +	* To enable the legacy monitoring feature:
> +	  ::
> +
> +	    # echo  "mbm_legacy" > /sys/fs/resctrl/info/L3_MON/mbm_assign
> +

No information about what the features are or what will happen on such a switch.

>   "max_threshold_occupancy":
>   		Read/write file provides the largest value (in
>   		bytes) at which a previously used LLC_occupancy
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index f452b6d9bb99..d77ff059269a 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -908,6 +908,43 @@ static int rdtgroup_mbm_assign_show(struct kernfs_open_file *of,
>   	return 0;
>   }
>   
> +/*
> + * rdtgroup_mode_write - Modify the resource group's mode

Comment does not match function name. Seems like "mode" is the generic term
to use instead of "assign".

> + */
> +static ssize_t rdtgroup_mbm_assign_write(struct kernfs_open_file *of,
> +					 char *buf, size_t nbytes,
> +					 loff_t off)
> +{
> +	struct rdt_resource *r = of->kn->parent->priv;
> +	int ret = 0;
> +
> +	if (!r->abmc_capable)
> +		return -EINVAL;
> +
> +	/* Valid input requires a trailing newline */
> +	if (nbytes == 0 || buf[nbytes - 1] != '\n')
> +		return -EINVAL;
> +
> +	buf[nbytes - 1] = '\0';
> +
> +	cpus_read_lock();
> +	mutex_lock(&rdtgroup_mutex);
> +
> +	rdt_last_cmd_clear();
> +
> +	if (!strcmp(buf, "mbm_legacy"))
> +		resctrl_abmc_disable(RDT_RESOURCE_L3);
> +	else if (!strcmp(buf, "abmc"))
> +		ret = resctrl_abmc_enable(RDT_RESOURCE_L3);
> +	else
> +		ret = -EINVAL;
> +
> +	mutex_unlock(&rdtgroup_mutex);
> +	cpus_read_unlock();
> +
> +	return ret ?: nbytes;
> +}
> +
>   #ifdef CONFIG_PROC_CPU_RESCTRL
>   
>   /*
> @@ -2087,9 +2124,10 @@ static struct rftype res_common_files[] = {
>   	},
>   	{
>   		.name		= "mbm_assign",
> -		.mode		= 0444,
> +		.mode		= 0644,
>   		.kf_ops		= &rdtgroup_kf_single_ops,
>   		.seq_show	= rdtgroup_mbm_assign_show,
> +		.write		= rdtgroup_mbm_assign_write,
>   	},
>   	{
>   		.name		= "cpus",


Reinette