Message-ID: <ef1ce9fa-7616-4763-b31c-1b5c3a8a7360@amd.com>
Date: Fri, 22 Nov 2024 15:34:33 -0600
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, 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 Reinette,

On 11/22/2024 12:12 PM, Reinette Chatre wrote:
> 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.

ok.

> 
>>>>> @@ -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?

Yes. Calling mbm_cntr_reset() from rdt_kill_sb() seems more clean approach.

Architectural state will reset again when counter is assigned(when mon 
directories are created).

thanks
- Babu Moger

Powered by blists - more mailing lists