Message-ID: <6edffe1b-e9a9-4995-8172-353efc189666@amd.com>
Date: Thu, 2 May 2024 15:14:33 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Peter Newman <peternewman@...gle.com>
Cc: corbet@....net, fenghua.yu@...el.com, reinette.chatre@...el.com,
 tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
 paulmck@...nel.org, rdunlap@...radead.org, tj@...nel.org,
 peterz@...radead.org, yanjiewtw@...il.com, kim.phillips@....com,
 lukas.bulwahn@...il.com, seanjc@...gle.com, jmattson@...gle.com,
 leitao@...ian.org, jpoimboe@...nel.org, rick.p.edgecombe@...el.com,
 kirill.shutemov@...ux.intel.com, jithu.joseph@...el.com,
 kai.huang@...el.com, kan.liang@...ux.intel.com,
 daniel.sneddon@...ux.intel.com, pbonzini@...hat.com, sandipan.das@....com,
 ilpo.jarvinen@...ux.intel.com, maciej.wieczor-retman@...el.com,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, eranian@...gle.com,
 james.morse@....com
Subject: Re: [RFC PATCH v3 00/17] x86/resctrl : Support AMD Assignable
 Bandwidth Monitoring Counters (ABMC)

Hi Peter,

On 5/2/24 12:50, Peter Newman wrote:
> Hi Babu,
> 
> On Thu, May 2, 2024 at 9:25 AM Moger, Babu <babu.moger@....com> wrote:
>> On 5/1/24 12:48, Peter Newman wrote:
>>> The FS layer is informed by the arch layer (through rdt_resource
>>> fields) how many assignable monitors are available and whether a
>>> monitor is assigned to an entire group or a single event in a group.
>>> Also, the FS layer can assume that monitors are indexed contiguously,
>>> allowing it to host the data structures managing FS-level view of
>>> monitor usage.
>>>
>>> I used the following resctrl_arch-interfaces to propagate assignments
>>> to the implementation:
>>>
>>> void resctrl_arch_assign_monitor(struct rdt_domain *d, u32 mon_id, u32
>>> closid, u32 rmid, int evtid);
>>
>> Sure. I can add these in next version.
>>
>> Few comments..
>>
>> AMD does not need closid for assignment. I assume ARM requires closid.
> 
> Correct, MPAM needs a CLOSID+RMID (PARTID+PMG) to identify a
> monitoring group. The CLOSID parameter is ignored on x86.
> 
>>
>> What is mon_id here?
> 
> On ABMC, the value is programmed into L3_QOS_ABMC_CFG.CtrID

ok.

> 
> 
>>
>>> void resctrl_arch_unassign_monitor(struct rdt_domain *d, u32 mon_id);
>>
>> We need rmid and evtid for unassign interface here.
> 
> From my reading of the ABMC specification, it does not look necessary
> to program BwSrc or BwType when changing L3_QOS_ABMC_CFG.CtrEn to 0
> for a particular CtrID. This interface only disables a counter, so it
> should not need to know about how it was previously used when assign
> is able to reassign, as assign will always reset the arch_mbm data.

Yes. That is correct. We may not need to set BwSrc or BwType for unassign.

But, we need evtid to update the monitor state of the rdtgroup.
> 
> I do not see any harm in the arch_mbm data being stale while the
> counter is unassigned, because the data is not accessed when reading
> the hardware counter fails. In general, resctrl_arch_rmid_read()
> cannot return any information if the hardware counter is not readable
> at the time it is called.

Ok. Le me check about keeping the stale arch_mbm data after unassign.
It may be okay.


> 
>>
>>
>>>
>>> I chose to allow reassigning an assigned monitor without calling
>>> unassign first. This is important when monitors are unassigned and
>>> assigned in a single write to mbm_assign_control, as it allows all
>>> updates to be performed in a single round of parallel IPIs to the
>>> domains.
>>
>> Yes. It is not required to call unassign before assign. Hardware(AMD)
>> supports it.
>>
>> But, we only have 32 counters. We need to know which counter we are going
>> to use for assignment. If all the counters already assigned, then we can't
>> figure out the counter id without calling unassigm first. Using the random
>> counter will overwrite the already assigned counter.
> 
> I made the caller of resctrl_arch_assign_monitor() responsible for
> selecting which monitor to assign. As long as the user orders the
> unassign operations before the assign operations in a write to
> mbm_assign_control, the FS code will be able to find an available
> monitor ID.

How does assign_resctrl_arch_assign_monitor() selects the monitor id (or
counter id) if all of them are assigned already.

In this series the monitor ids are allocated using assign_cntrs_alloc.
rdtgroup_assign_abmc()  calls assign_cntrs_alloc() to get monitor id. It
reports error if it cannot get free monitor id.

Expectation is the user to unassign an event from another group(or the
same group) before calling assign.

Are you expecting something else here?

> 
> 
>>> I chose to make this a mount option to simplify the management of the
>>> monitor tracking data structures. They are simply allocated at mount
>>> time and deallocated and unmount.
>>
>> Initially I added it as an mount option.
>> Based on our earlier discussion, we decided to use the assign feature by
>> default if hardware supports it. Users don't have to worry about the details.
>>>
>>> I called the option "mon_assign": The mount option parser calls
>>> resctrl_arch_mon_assign_enable() to determine whether the
>>> implementation supports assignment in some form. If it returns an
>>> error, the mount fails. When successful, the assignable monitor count
>>> is made non-zero in the appropriate rdt_resource, triggering the
>>> behavior change in the FS layer.
>>>
>>> I'm still not sure if it's a good idea to enable monitor assignment by
>>> default. This would be a major disruption in the MBM usage model
>>> triggered by moving software between AMD CPU models. I thought the
>>
>> Why will it be a disruption? Why do you think mount option will solve the
>> problem? As always, there will be option to go back to legacy mode. right?
>>
>>> safest option was to disallow creating more monitoring groups than
>>> monitors unless the option is selected. Given that nobody else
>>
>> Current code allows to create more groups, but it will report "Monitor
>> assignment failed" when it runs out of monitors.
> 
> Ok that should be fine then.
> 
> However, I don't think it's necessary to support dynamically changing
> the usage model of monitoring groups without remounting. I believe it
> makes it more difficult for the FS code to generically manage monitor
> assignment.

Are you suggesting to enable ABMC by default when available?

Then provide the mount option switch back to legacy mode?
I am fine with that if we all agree on that.
-- 
Thanks
Babu Moger

Powered by blists - more mailing lists