Re: [PATCH v5 12/12] Documentation/x86: Update resctrl_ui.rst for new features

[Moger, Babu]

Hi Reinette,

Already responded to this but i don't see my response in archives yet.

On 10/3/22 10:36, Reinette Chatre wrote:
> Hi Babu,
>
> On 10/3/2022 7:28 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 9/29/22 17:10, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> In subject: resctrl_ui.rst -> resctrl.rst
>>>
>>> On 9/27/2022 1:27 PM, Babu Moger wrote:
>>>> Update the documentation for the new features:
>>>> 1. Slow Memory Bandwidth allocation (SMBA).
>>>>    With this feature, the QOS  enforcement policies can be applied
>>>>    to the external slow memory connected to the host. QOS enforcement
>>>>    is accomplished by assigning a Class Of Service (COS) to a processor
>>>>    and specifying allocations or limits for that COS for each resource
>>>>    to be allocated.
>>>>
>>>> 2. Bandwidth Monitoring Event Configuration (BMEC).
>>>>    The bandwidth monitoring events mbm_total_bytes and mbm_local_bytes
>>>>    are set to count all the total and local reads/writes respectively.
>>>>    With the introduction of slow memory, the two counters are not
>>>>    enough to count all the different types are memory events. With the
>>> types are memory events -> types of memory events?
>> Ok Sure
>>>>    feature BMEC, the users have the option to configure mbm_total_bytes
>>>>    and mbm_local_bytes to count the specific type of events.
>>>>
>>>> Also add configuration instructions with examples.
>>>>
>>>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>>>> ---
>>> ...
>>>
>>>> +
>>>> +"mbm_total_config", "mbm_local_config":
>>>> +        These files contain the current event configuration for the events
>>>> +        mbm_total_bytes and mbm_local_bytes, respectively, when the
>>>> +        Bandwidth Monitoring Event Configuration (BMEC) feature is supported.
>>>> +        The event configuration settings are domain specific. Changing the
>>>> +        configuration on one CPU in a domain would affect the whole domain.
>>> This contradicts the implementation done in this series where the
>>> configuration is changed on every CPU in the domain.
>> How about this?
>>
>> The event configuration settings are domain specific and will affect all the CPUs in the domain.
> There remains a disconnect between this and the implementation that writes the
> configuration to every CPU.
>
> You could make this change to the documentation but then the
> implementation needs more than "Update MSR_IA32_EVT_CFG_BASE MSR on all
> the CPUs in cpu_mask" - that comment needs to highlight that the
> implementation does not follow the architecture and scope rules nor how
> configuration changes are made in the rest of the driver and why. Previously [1]
> you indicated that this is based on guidance from hardware team so perhaps you
> could document it as a hardware quirk related to this feature? At the minimum
> it should acknowledge the disconnect.

ok. I could document this in the code patch 9([PATCH v5 09/12]
x86/resctrl: Add sysfs interface to write mbm_total_bytes event configuration.
Something like this.

/*
+        * Update MSR_IA32_EVT_CFG_BASE MSR on all the CPUs in cpu_mask.
+        * The MSR MSR_IA32_EVT_CFG_BASE is domain specific. Writing the
+        * MSR on one CPU will affect all the CPUs in the domain.
+        * However, the hardware team recommends to update the MSR on
+        * all the CPU threads. It is not clear in the document yet.
*        * Doc will be updated in the next revision.
+        */
+       on_each_cpu_mask(cpu_mask, mon_event_config_write, &mon_info, 1);
+

Thanks
Babu