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 for Android: free password hash cracker in your pocket

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

Message-ID: <053297de-b2bb-4077-8700-997564f93fbf@intel.com>
Date: Fri, 22 Nov 2024 13:33:12 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: "Luck, Tony" <tony.luck@...el.com>
CC: Fenghua Yu <fenghua.yu@...el.com>, Peter Newman <peternewman@...gle.com>,
	Jonathan Corbet <corbet@....net>, <x86@...nel.org>, 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/21/24 9:33 AM, Luck, Tony wrote:
> On Tue, Nov 19, 2024 at 05:08:42PM -0800, Reinette Chatre wrote:
>> 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.
>>
> Hi Reinette,
> 
>> 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.
> 
> I'll add text documenting this to the commit comment here, and also a
> comment in the code that defines mba_mbps_default_event.
> 
>> * 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().
> 
> With some refactoring I've got that down to just one new additon of
> "is_mba_sc()" (protecting the assignment of rdtgrp->mba_mbps_event
> in rdtgroup_mkdir_ctrl_mon().
> 

Just to clarify, I am not stating that there should not be three different checks. Clearly
if some initialization is done before resctrl mount then the mba_sc checks may not apply.
My goal was to highlight the investigations a reader is forced to do without
guidance from the changelog.

...

>>> @@ -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.
> 
> Good catch. You are correct that a changed event value in the default
> CTRL_MON group is preserved across unmount/remount. This was not
> intentional. Moving the initialization of rdtgroup_default.mba_mbps_event
> into set_mba_sc() fixes this (with the added benefit of removing the
> supports_mba_mbps() check).

I am curious how this will turn out considering the fragmentation of the rdtgroup_default
initialization.

Reinette