linux-kernel - Re: [PATCH v9 20/26] x86/resctrl: Auto assign/unassign counters when mbm_cntr

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: <c6f4bbec-0188-4855-a57b-fa2bf1e1b731@intel.com>
Date: Fri, 22 Nov 2024 10:12:34 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: <babu.moger@....com>, <corbet@....net>, <tglx@...utronix.de>,
	<mingo@...hat.com>, <bp@...en8.de>, <dave.hansen@...ux.intel.com>
CC: <fenghua.yu@...el.com>, <x86@...nel.org>, <hpa@...or.com>,
	<thuth@...hat.com>, <paulmck@...nel.org>, <rostedt@...dmis.org>,
	<akpm@...ux-foundation.org>, <xiongwei.song@...driver.com>,
	<pawan.kumar.gupta@...ux.intel.com>, <daniel.sneddon@...ux.intel.com>,
	<perry.yuan@....com>, <sandipan.das@....com>, <kai.huang@...el.com>,
	<xiaoyao.li@...el.com>, <seanjc@...gle.com>, <jithu.joseph@...el.com>,
	<brijesh.singh@....com>, <xin3.li@...el.com>, <ebiggers@...gle.com>,
	<andrew.cooper3@...rix.com>, <mario.limonciello@....com>,
	<james.morse@....com>, <tan.shaopeng@...itsu.com>, <tony.luck@...el.com>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<peternewman@...gle.com>, <maciej.wieczor-retman@...el.com>,
	<eranian@...gle.com>, <jpoimboe@...nel.org>, <thomas.lendacky@....com>
Subject: Re: [PATCH v9 20/26] x86/resctrl: Auto assign/unassign counters when
 mbm_cntr_assign is enabled

Hi Babu,

On 11/21/24 4:26 PM, Moger, Babu wrote:
> On 11/21/2024 6:22 PM, Moger, Babu wrote:
>> On 11/18/2024 11:18 AM, Reinette Chatre wrote:
>>> On 10/29/24 4:21 PM, Babu Moger wrote:

>>>> @@ -3871,6 +3920,8 @@ static int rdtgroup_mkdir_mon(struct kernfs_node *parent_kn,
>>>>           goto out_unlock;
>>>>       }
>>>> +    rdtgroup_assign_cntrs(rdtgrp);
>>>> +
>>>>       kernfs_activate(rdtgrp->kn);
>>>>       /*
>>>> @@ -3915,6 +3966,8 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
>>>>       if (ret)
>>>>           goto out_closid_free;
>>>> +    rdtgroup_assign_cntrs(rdtgrp);
>>>> +
>>>>       kernfs_activate(rdtgrp->kn);
>>>>       ret = rdtgroup_init_alloc(rdtgrp);
>>>
>>> Please compare the above two hunks with earlier "x86/resctrl: Introduce cntr_id in mongroup for assignments".
>>> Earlier patch initializes the counters within mkdir_rdt_prepare_rmid_alloc() while the above
>>> hunk assigns the counters after mkdir_rdt_prepare_rmid_alloc() is called. Could this fragmentation be avoided
>>> with init done once within mkdir_rdt_prepare_rmid_alloc()?
>>
>> It seems more appropriate to call rdtgroup_cntr_id_init() inside mkdir_rdt_prepare(). This will initialize the cntr_id to MON_CNTR_UNSET.
>>
>> And then call rdtgroup_assign_cntrs() inside mkdir_rdt_prepare_rmid_alloc().
>>
>> what do you think?

Taking a closer look this seems most appropriate. mkdir_rdt_prepare() is where the resource groupreset
is created and all fields initialized, control and monitoring (irrespective of monitoring enabled). 
Doing the MON_CNTR_UNSET initalization in that central place seems good.
Yes, and then assigning the counters in mkdir_rdt_prepare_rmid_alloc() sounds good.

>>>> @@ -3940,6 +3993,7 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
>>>>   out_del_list:
>>>>       list_del(&rdtgrp->rdtgroup_list);
>>>>   out_rmid_free:
>>>> +    rdtgroup_unassign_cntrs(rdtgrp);
>>>>       mkdir_rdt_prepare_rmid_free(rdtgrp);
>>>>   out_closid_free:
>>>>       closid_free(closid);
>>>> @@ -4010,6 +4064,9 @@ static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>>>>       update_closid_rmid(tmpmask, NULL);
>>>>       rdtgrp->flags = RDT_DELETED;
>>>> +
>>>> +    rdtgroup_unassign_cntrs(rdtgrp);
>>>> +
>>>>       free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
>>>>       /*
>>>> @@ -4056,6 +4113,8 @@ static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>>>>       cpumask_or(tmpmask, tmpmask, &rdtgrp->cpu_mask);
>>>>       update_closid_rmid(tmpmask, NULL);
>>>> +    rdtgroup_unassign_cntrs(rdtgrp);
>>>> +
>>>>       free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
>>>>       closid_free(rdtgrp->closid);
>>>
>>> There is a potential problem here. rdtgroup_unassign_cntrs() attempts to remove counter from
>>> all domains associated with the resource group. This may fail in any of the domains that results
>>> in the counter not being marked as free in the global map and not reset the counter in the
>>> resource group ... but the resource group is removed right after calling rdtgroup_unassign_cntrs().
>>> In this scenario there is thus a counter that is considered to be in use but not assigned to any
>>> resource group.
>>>
>>>>> From what I can tell there is a difference here between default resource group and the others:
>>> on remount of resctrl the default resource group will maintain knowledge of the counter that could
>>> not be unassigned. This means that unmount/remount of resctrl does not provide a real "clean slate"
>>> when it comes to counter assignment. Is this intended?
>>>
>>
>> Yes. Agree. It is not intended.
>>
>> How about adding bitmap_zero() inside rdt_get_tree() to address this problem? Also need to reset the cntr_id in rdt_kill_sb().
> 
> I meant reset the cntr_id for the default group in rdt_kill_sb().

Doing the cntr_id reset like this matches the custom is to reset to defaults in rdt_kill_sb(). I am not sure
what you envision with the bitmap_zero() in rdt_get_tree() ... I wonder if it may not just be simpler to
call mbm_cntr_reset() from rdt_kill_sb()? This does raise the question if mbm_cntr_reset() should reset
architectural state. I do not think it does harm, the state will just be reset again when the mon dirs are
created?

Reinette