linux-kernel - Re: [PATCH v9 2/9] x86/resctrl: Prepare for per-ctrl_mon group mba

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <515914f2-501d-4df3-894a-2d255d18be1c@intel.com>
Date: Tue, 19 Nov 2024 17:08:42 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, Fenghua Yu <fenghua.yu@...el.com>, "Peter
 Newman" <peternewman@...gle.com>, Jonathan Corbet <corbet@....net>,
	<x86@...nel.org>
CC: James Morse <james.morse@....com>, Jamie Iles <quic_jiles@...cinc.com>,
	Babu Moger <babu.moger@....com>, Randy Dunlap <rdunlap@...radead.org>,
	"Shaopeng Tan (Fujitsu)" <tan.shaopeng@...itsu.com>,
	<linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v9 2/9] x86/resctrl: Prepare for per-ctrl_mon group
 mba_MBps control

Hi Tony,

On 11/13/24 4:17 PM, Tony Luck wrote:
> Resctrl uses local memory bandwidth event as input to the feedback
> loop when the mba_MBps mount option is used. This means that this
> mount option cannot be used on systems that only support monitoring
> of total bandwidth.
> 
> Prepare to allow users to choose the input event independently for
> each ctrl_mon group.

The lack of detail on design and implementation leaves a lot for the
reader to decipher. For example,
* the change appears to create a contract that rdtgroup.mba_mbps_event
  is only valid if mba_sc is enabled, this is "documented" in the
  structure member comment but not connected to the rest of implementation, not
  here nor later in series.
* the patch uses *three* different checks to manage new variables:
  is_mbm_local_enabled(), is_mba_sc(), and supports_mba_mbps(). Reader is
  left to decipher that all checks are built on is_mbm_local_enabled()
  and thus it is ok to use these checks before using the value that is only
  assigned when is_mbm_local_enabled().
* clearly mba_mbps_default_event cannot always have a value making reader wonder
  if enum resctrl_event_id needs a "0", takes some deciphering to get confidence
  that its assignment when is_mbm_local_enabled() fits under the contract
  that values are only value when is_mba_sc() and thus any code following contract by
  first checking for mba_sc should never encounter a 0.
* based on premise of this work reader may consider what happens if
  system does not support local MBM. more deciphering needed to get confidence
  that while mba_mbps_default_event will not be set, since is_mba_sc() still
  depends on local MBM this still fits under contract that mba_mbps_default_event
  cannot be used in this case.

Of course, it may just me that needs more help to understand what a patch is doing 
while having little insight into what it intends to do. I thought by sharing some of
the questions I felt needed to investigated may give some insight into the difficulty
a cryptic changelog creates. Review could be helped significantly if the changelog
provides insight into the design decisions. 

...

> @@ -3611,6 +3613,8 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
>  			rdt_last_cmd_puts("kernfs subdir error\n");
>  			goto out_del_list;
>  		}
> +		if (is_mba_sc(NULL))
> +			rdtgrp->mba_mbps_event = mba_mbps_default_event;
>  	}
>  
>  	goto out_unlock;
> @@ -3970,6 +3974,8 @@ static void __init rdtgroup_setup_default(void)
>  	rdtgroup_default.closid = RESCTRL_RESERVED_CLOSID;
>  	rdtgroup_default.mon.rmid = RESCTRL_RESERVED_RMID;
>  	rdtgroup_default.type = RDTCTRL_GROUP;
> +	if (supports_mba_mbps())
> +		rdtgroup_default.mba_mbps_event = mba_mbps_default_event;
>  	INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list);
>  
>  	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);

I do not see the default resource group's mba_mbps_event ever being reset. This means
that if the user mounts resctrl, changes mba_mbps_event, umount resctrl, remount
resctrl, then the default resource group will not have the default mba_mbps_event
but whatever was set on previous mount. Is this intended? No mention of this behavior in
changelog.

Reinette