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

From: Moger, Babu
Date: Tue Oct 04 2022 - 10:00:56 EST

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);