Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <8f09a4bb-5310-4d49-ba50-cb945f94319c@intel.com>
Date:   Tue, 12 Dec 2023 10:59:43 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 3/3] x86/resctrl: Add new "mba_MBps_event" mount option
 to documentation
Content-Language: en-US
To:     Tony Luck <tony.luck@intel.com>, Fenghua Yu <fenghua.yu@intel.com>,
        "Peter Newman" <peternewman@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        "Shuah Khan" <skhan@linuxfoundation.org>, <x86@kernel.org>
CC:     Shaopeng Tan <tan.shaopeng@fujitsu.com>,
        James Morse <james.morse@arm.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Babu Moger <babu.moger@amd.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <patches@lists.linux.dev>
References: <20231201214737.104444-1-tony.luck@intel.com>
 <20231207195613.153980-1-tony.luck@intel.com>
 <20231207195613.153980-4-tony.luck@intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20231207195613.153980-4-tony.luck@intel.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?YXV6U3ZaR2hwc0JySnhPdGxMOU5pR0dWQmFOQndKM1NIdEtpeVd1dUpGdDgy?=
 =?utf-8?B?UFlRL2lBejRFampPa3FwVWxPaGpZLzBSV0ZjRDkvQno4WERHck5PSVphV3dn?=
 =?utf-8?B?SUd0NmtrS2Njek9PYVpVcEFnVHppTlJ4YldvSHhoTGdiRWIxUjZjOVlwb01q?=
 =?utf-8?B?OFN4K2VwMEozZjVzOVZsSnJMRVI1a3g4Um9vMUJvbUpVcTN0Mzc0RzFYQmNw?=
 =?utf-8?B?NWNZZ0dxZmNvMktNL3Y0RGxnMm5seU5obUI0ZVBFdE1xd2NmdDV2YWhwMU1y?=
 =?utf-8?B?cHRmOFdIUjBXSXpnTjdESlR0b0dSWWQwZmRXWVoxRGtreGQ5T0dBL1FKM2JW?=
 =?utf-8?B?L2h1ZEF2dFhyczlhNnUwZ3E2ZVlIeWYwaURWSjZPYzBDQVc5b2xDaWlISmFx?=
 =?utf-8?B?NmppZHBSZ2t3ZFNZT1lIdzRORFYxbmxvaG1DRlRhNzh4aWtucFZWZUNZVHFs?=
 =?utf-8?B?c0tNb0JnbTQ0bUNHbzUyK1hwSDZ6b2FvaEh1d2F0ZWJRKzNHOENkM1VENXlN?=
 =?utf-8?B?R0VOVGtublN0TkhJZ3NaME1hSFBuNlpQcXAxamgwaThUelZLc2F2Z3F2TzFy?=
 =?utf-8?B?MFFrK1lxY1grWmtlRTZQdW5UMXNrdmZFZWRERmIwOVoyTnJ5OVVUQmdWaXdJ?=
 =?utf-8?B?cUVHZWYxQ0VxUTEwUUJ6VmZlWVE2ald6VnhWZkdody9ZUXR6c3lldXhQT2Rr?=
 =?utf-8?B?T3ZTMjdpejlRQnZtc2xFdDhZSDEybmkvYUZRL1lNcndCM2NVbzJXSUZLbzFo?=
 =?utf-8?B?VXVnMEhKVjZoeDVJb3dKWmRKd29qaitOMk9UcWNFMzFORGtDckdHR2dHRlZK?=
 =?utf-8?B?d3NvZlJMVHJYSTBhU3Y1TlVZZE4rcC9YWnhzclhhbmM5YmlTQUpKMU1oK1hy?=
 =?utf-8?B?em96aXlvNHc5bWpUajJtQ3J0djRHaDJ3WEEvR0E4TG5VOEhpSExTLzFNUVNr?=
 =?utf-8?B?dGg3NkIrVXd5UkVtekxPS3h2NjlsZ2ROUlIxNFNZR1ZZREwySXphNktxSVFn?=
 =?utf-8?B?RUJsUHBJS2t5azNkVFd0QTF5QXczd3dYcmZXTW54UlpZb0pOTDRMdDVRNlhw?=
 =?utf-8?B?dFAyTWgyMlV0Z2N6R2c2TjBaQ2ZSVmU5bWQ0bk4rd3RLTDQ0VHhoVElGZVh3?=
 =?utf-8?B?TU5TbVFwMVJXdnBYbmRVdlp4MWI1MXNyYlU2WlRvbDNyTDViMmloTmVodnd5?=
 =?utf-8?B?YUs4alFnT0VyN1VHK3NHallLZ01hSFJIWjdsbmhod2Y2a1RHaVVKWWlOek9Y?=
 =?utf-8?B?VUFWTHBETVAvUzdnVlVtYzF2dy9IVU5nRzlhS1doZVlBN2lJWXVUOWJLMC9r?=
 =?utf-8?B?QStUa2htcnVScHlrYWQ3SEtuMjhQbTJTTk1RQlFyWHUyYTlQOTljTG1LeEhR?=
 =?utf-8?B?Qy9kVHBaNnlLUnc0eWhXdy8zSWVSV09raldWeGtiSEM5b3VVbTRtaHF3R211?=
 =?utf-8?B?dE1YNERWNGlKU2VndlBBWFFheFVNc3FzSS9zWTBsdEJlR0hJZHM5OGJHNlFV?=
 =?utf-8?B?QkFqR0Z6MW9mV3p6MEZ0ZGh2TzR2YlRzU2xiWmpsRnJodVdQdGNacWhKeHRV?=
 =?utf-8?B?cjNVRy9aUlhmTzVodXVUTWJwek9LbXF5Ym85YWNpUC9XeC9DN2FXQTFYNG5a?=
 =?utf-8?B?ZStBZHFLaXZlZTJlc2UvK0tIQ1llRWlHWThGVENyTkgwZzVZdTdkeEpRZUs3?=
 =?utf-8?B?ZUhBQUNxT0JOWWxON0o5aW5NcHdpY1RLdmIyQUNXOHgwRTJsbXJ3czI5Tm1t?=
 =?utf-8?B?Qy9iYWRUelVVN2JaZi9MRFNrelN3L0tGZVZtSnhtbTNZLzdBZHJqaVJLbjd4?=
 =?utf-8?B?OEdrMlQvNWVlU1lGb3Fac3A2SHdwcjZtUVNtd3NOTmFwQWMwdjR3aWhTakt5?=
 =?utf-8?B?V0x4cHFTM3JOY3pPd0lwTFl5b0NWakJHZUlwQ3JVbjVyRUNoNk1wdDNPTSs3?=
 =?utf-8?B?NVljRlFyTkZ1RDVJK0c5U01VRTljZi9Yd2tNM0J3SWdEUkFtcFA4OU1YNFl4?=
 =?utf-8?B?bjFGRnBaVzdvMnp0TWh3SEE4Mko3bGljUHZYK1h4blgzbHNCelNub0FWNjJT?=
 =?utf-8?B?UUJ1L0pMVXBvM1p0bGNZWmlYaUVJUWNPNG5obGMrcUkwUitzd1RQdDJQbndt?=
 =?utf-8?B?N25SNEoweG56Sk03M2RUSVNzTzYvSUpHL1p3dWVzekxmUHAxUCt4bmZmYTVv?=
 =?utf-8?B?bXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5636f26a-e2a3-4983-07a6-08dbfb4480d5
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Dec 2023 18:59:44.2079
 (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: 50dXs/EZijw6osc0c8jxF9pKDUFZLbxYXowBwq2GuVpqXRgc3bH5k27EjSQ5ADTPgFsxS6qbFrSX9QFDaRHRLegAmeYBkOADohKZ/k6WI/k=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV2PR11MB6070
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Tue, 12 Dec 2023 11:00:02 -0800 (PST)

Hi Tony,

On 12/7/2023 11:56 AM, Tony Luck wrote:
> New mount option may be used to choose a specific memory bandwidth
> monitoring event to feed the MBA Software Controller(mba_sc) feedback
> loop.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  Documentation/arch/x86/resctrl.rst | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index a6279df64a9d..a0c521db6786 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -35,7 +35,8 @@ about the feature from resctrl's info directory.
>  
>  To use the feature mount the file system::
>  
> - # mount -t resctrl resctrl [-o cdp[,cdpl2][,mba_MBps][,debug]] /sys/fs/resctrl
> + # mount -t resctrl resctrl [-o cdp[,cdpl2][,mba_MBps] \
> + [,mba_MBps_event=[mbm_local_bytes|mbm_total_bytes]][,debug]] /sys/fs/resctrl
>  
>  mount options are:
>  
> @@ -45,7 +46,12 @@ mount options are:
>  	Enable code/data prioritization in L2 cache allocations.
>  "mba_MBps":
>  	Enable the MBA Software Controller(mba_sc) to specify MBA
> -	bandwidth in MBps
> +	bandwidth in MBps. Defaults to using MBM local bandwidth,
> +	but will use total bandwidth on systems that do not support
> +	local bandwidth monitoring.

This document is intended as user documentation. With this perspective
I find the above change to have potential for confusion. The first
(original) sentence is clearly aimed at the user, informing that when this
option is enabled bandwidth allocation can be specified in MBps. The
second sentence jumps into kernel internals mentioning bandwidth monitoring
(user may wonder - what does this have to do with allocation?) without any
transition or context.

> +"mba_MBps_event=[mbm_local_bytes|mbm_total_bytes]":
> +	Enable the MBA Software Controller(mba_sc) with a specific
> +	MBM event as input to the feedback loop.

I think it should be made obvious what the relationship between the
mba_MBps and mba_MBps_event mount options are. As it is written the
user may believe that both options are needed. The user is also
left needing to interpret cryptic information regarding kernel internals
- for example, note how "feedback loop" does not occur in this document. 

>  "debug":
>  	Make debug files accessible. Available debug files are annotated with
>  	"Available only with debug option".
> @@ -538,6 +544,12 @@ where as user can switch to the "MBA software controller" mode using
>  a mount option 'mba_MBps'. The schemata format is specified in the below
>  sections.
>  
> +By default the software feedback mechanism uses measurement of local
> +memory bandwidth to make adjustments to throttling levels. If a system
> +is running applications with poor NUMA locality users may want to use
> +the "mba_MBps_event=mbm_total_bytes" mount option which will use total
> +memory bandwidth measurements instead of local.

The paragraph is just appended to existing MBps documentation instead of
integrated with existing content. User is left to parse and connect the
different sections attempting to make sense of it all.

Reinette