Re: [PATCH 00/15] x86/resctrl : Support AMD QoS RMID Pinning feature

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 12/7/2023 3:07 PM, Moger, Babu wrote:
> On 12/7/2023 1:29 PM, Reinette Chatre wrote:
>> On 12/7/2023 8:12 AM, Moger, Babu wrote:
>>> On 12/6/23 12:49, Reinette Chatre wrote:
>>>> On 12/6/2023 7:40 AM, Moger, Babu wrote:
>>>>> On 12/5/23 17:17, Reinette Chatre wrote:
>>>>>> On 11/30/2023 4:57 PM, Babu Moger wrote:
>>
>>>>>>> b. Mount with ABMC support
>>>>>>>     #umount /sys/fs/resctrl/
>>>>>>>     #mount  -o abmc -t resctrl resctrl /sys/fs/resctrl/
>>>>>>>     
>>>>>> hmmm ... so this requires the user to mount resctrl, determine if the
>>>>>> feature is supported, unmount resctrl, remount resctrl with feature enabled.
>>>>>> Could you please elaborate what prevents this feature from being enabled
>>>>>> without needing to remount resctrl?
>>>>> Spec says
>>>>> "Enabling ABMC: ABMC is enabled by setting L3_QOS_EXT_CFG.ABMC_En=1 (see
>>>>> Figure 19-7). When the state of ABMC_En is changed, it must be changed to
>>>>> the updated value on all logical processors in the QOS Domain.
>>>>> Upon transitions of the ABMC_En the following actions take place:
>>>>> All ABMC assignable bandwidth counters are reset to 0.
>>>>> The L3 default mode bandwidth counters are reset to 0.
>>>>> The L3_QOS_ABMC_CFG MSR is reset to 0."
>>>>>
>>>>> So, all the monitoring group counters will be reset.
>>>>>
>>>>> It is technically possible to enable without remount. But ABMC mode
>>>>> requires few new files(in each group) which I added when mounted with "-o
>>>>> abmc". Thought it is a better option.
>>>>>
>>>>> Otherwise we need to add these files when ABMC is supported(not when
>>>>> enabled). Need to add another file in /sys/fs/resctrl/info/L3_MON to
>>>>> enable the feature on the fly.
>>>>>
>>>>> Both are acceptable options. Any thoughts?
>>>> The new resctrl files in info/ could always be present. For example,
>>>> user space may want to know how many counters are available before
>>>> enabling the feature.
>>>>
>>>> It is not yet obvious to me that this feature requires new files
>>>> in monitor groups.
>>> There are two MBM events(total and local) in each group.
>>> We should provide an interface to assign each event independently.
>>> User can assign only one event in a group. We should also provide an
>>> option assign both the events in the group. This needs to be done at
>>> resctrl group level.
>> Understood. I would like to start by considering how (if at all) existing
>> files may be used, thus my example of using mbm_total_bytes, before adding
>> more files.
>>
>>
>> ...
>>
>>>>>>>     #cat /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local_bytes
>>>>>>>     Unavailable
>>>>>> I believe that "Unavailable" already has an accepted meaning within current
>>>>>> interface and is associated with temporary failure. Even the AMD spec states "This
>>>>>> is generally a temporary condition and subsequent reads may succeed". In the
>>>>>> scenario above there is no chance that this counter would produce a value later.
>>>>>> I do not think it is ideal to overload existing interface with different meanings
>>>>>> associated with a new hardware specific feature ... something like "Disabled" seems
>>>>>> more appropriate.
>>>>> Hardware still reports it as unavailable. Also, there are some error cases
>>>>> hardware can report unavailable. We may not be able to differentiate that.
>>>> This highlights that this resctrl feature is currently latched to AMD's
>>>> ABMC. I do not think we should require that this resctrl feature is backed
>>>> by hardware that can support reads of counters that are disabled. A counter
>>>> read really only needs to be sent to hardware if it is enabled.
>>>>
>>>>>> Considering this we may even consider using these files themselves as a
>>>>>> way to enable the counters if they are disabled. For example, just
>>>>>> "echo 1 > /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes" can be used
>>>>> I am not sure about this. This is specific to domain 0. This group can
>>>>> have cpus from multiple domains. I think we should have the interface for
>>>>> all the domains(not for specific domain).
>>>> Are the ABMC registers not per CPU? This is unclear to me at this time
>>>> since changelog of patch #13 states it is per-CPU but yet the code
>>>> uses smp_call_function_any().
>>> Here are the clarifications from hardware engineer about this.
>>>
>>> # While configuring the counter, should we have to write (L3_QOS_ABMC_CFG)
>>> on all the logical processors in a domain?
>>>
>>> No.  In order to configure a specific counter, you only need to write it
>>> on a  single logical processor in a domain.  Configuring the actual ABMC
>>> counter is a side-effect of the write to this register.  And the actual
>>> ABMC counter configuration is a  global state.
>>>
>>> "Each logical processor implements a separate copy of these registers"
>>> identifies that if you write a 5 to L3_QOS_ABMC_CFG on C0T0, you will not
>>> read a 5 from the L3_QOS_ABMC_CFG register on C1T0.
>> Thank you for this information. Would reading L3_QOS_ABMC_DSC register on
>> C1T0 return the configuration written to L3_QOS_ABMC_CFG on C0T0 ?
> 
> Yes. Because the counter counter configuration is global. Reading L3_QOS_ABMC_DSC will return the configuration of the counter specified by
> 
> QOS_ABMC_CFG[CtrID].


To confirm, when you say "global" you mean within a domain?

> 
>>
>> Even so, you do confirm that the counter configuration is per domain. If I
>> understand correctly the implementation in this series assumes the counters
>> are programmed identically on all domains, but theoretically the system can support
>> domains with different counter configurations. For example, if a resource group
>> is limited to CPUs in one domain it would be unnecessary to consume the other
>> domain's counters.
> Yes. It is programmed on all the domains. Separating the domain
> configuration will require more changes. I am not planning to address
> in this series.

That may be ok. The priority is to consider how users want to interact with this
feature and create a suitable interface to support this. This version may not
separate domain configuration, but we do not want to create an the interface that
prevents such an enhancement in the future. Especially since it is already known
that hardware supports it.

Reinette