linux-kernel - Re: [PATCH 06/18] x86/resctrl: Allow the allocator to check if a CLOSID can allocate clean RMID

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: <ced8f472-c719-d604-5f43-9b3316ec0a95@arm.com>
Date:   Thu, 24 Nov 2022 14:21:52 +0000
From:   James Morse <james.morse@....com>
To:     "Shaopeng Tan (Fujitsu)" <tan.shaopeng@...itsu.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc:     Fenghua Yu <fenghua.yu@...el.com>,
        Reinette Chatre <reinette.chatre@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Babu Moger <Babu.Moger@....com>,
        "shameerali.kolothum.thodi@...wei.com" 
        <shameerali.kolothum.thodi@...wei.com>,
        D Scott Phillips OS <scott@...amperecomputing.com>,
        "carl@...amperecomputing.com" <carl@...amperecomputing.com>,
        "lcherian@...vell.com" <lcherian@...vell.com>,
        "bobo.shaobowang@...wei.com" <bobo.shaobowang@...wei.com>,
        Jamie Iles <quic_jiles@...cinc.com>,
        Xin Hao <xhao@...ux.alibaba.com>,
        "xingxin.hx@...nanolis.org" <xingxin.hx@...nanolis.org>,
        "baolin.wang@...ux.alibaba.com" <baolin.wang@...ux.alibaba.com>,
        "peternewman@...gle.com" <peternewman@...gle.com>
Subject: Re: [PATCH 06/18] x86/resctrl: Allow the allocator to check if a
 CLOSID can allocate clean RMID

Hello,

On 10/11/2022 10:50, Shaopeng Tan (Fujitsu) wrote:
> Hi James
> 
>> MPAM's PMG bits extend its PARTID space, meaning the same PMG value can
>> be used for different control groups.
>>
>> This means once a CLOSID is allocated, all its monitoring ids may still be dirty,
>> and held in limbo.
>>
>> Add a helper to allow the CLOSID allocator to check if a CLOSID has dirty RMID
>> values. This behaviour is enabled by a kconfig option selected by the
>> architecture, which avoids a pointless search for x86.
>>


>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c
>> b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index 59da256a77fe..99854ef4dee4 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -320,6 +320,37 @@ static struct rmid_entry *resctrl_find_free_rmid(u32

>> +bool resctrl_closid_is_dirty(u32 closid) {
>> +	struct rmid_entry *entry;
>> +	int i;
>> +
>> +	lockdep_assert_held(&rdtgroup_mutex);
>> +
>> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
>> +		return false;


>> a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index ac88610a6946..59f33adcf6f8 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -119,14 +119,18 @@ static void closid_init(void)
>>
>>  static int closid_alloc(void)
>>  {
>> -	u32 closid = ffs(closid_free_map);
>> +	u32 closid;
>>
>> -	if (closid == 0)
>> -		return -ENOSPC;
>> -	closid--;
>> -	closid_free_map &= ~(1 << closid);
>> +	for_each_set_bit(closid, &closid_free_map, closid_free_map_len) {
>> +		if
>> (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
>> +		    resctrl_closid_is_dirty(closid))


> IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) is redundant here,
> since it is also at the beginning of function resctrl_closid_is_dirty(closid).

This is true. I included it because resctrl_closid_is_dirty() is in a different
compilation unit, so the compiler can't know it does nothing if that config option isn't
enabled. This avoided a pointless call to a function that does nothing. But you're right
it would be more readable without it, and creating a control group is hardly a performance
critical path. I'll remove it.


Thanks,

James