Re: [PATCH v6 11/22] x86/resctrl: Remove MSR reading of event configuration value

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,


On 8/16/24 16:36, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/6/24 3:00 PM, Babu Moger wrote:
>> The event configuration is domain specific and initialized during domain
>> initialization. The values is stored in rdt_hw_mon_domain.
> 
> "The values is stored in rdt_hw_mon_domain." -> "The values are stored
> in struct rdt_hw_mon_domain."

Sure.

> 
>>
>> It is not required to read the configuration register every time user asks
>> for it. Use the value stored in rdt_hw_mon_domain instead.
> 
> "rdt_hw_mon_domain" -> "struct rdt_hw_mon_domain"

Sure.
> 
>>
>> Introduce resctrl_arch_event_config_get() and
>> resctrl_arch_event_config_set() to get/set architecture domain specific
>> mbm_total_cfg/mbm_local_cfg values. Also, remove unused config value
>> definitions.
> 
> hmmm ... while the config values are not used they are now established
> ABI and any other architecture that wants to support configurable events
> will need to follow these definitions. It is thus required to keep them
> documented in the kernel in support of future changes. I
> understand that they are documented in user docs, but could we keep them
> in the kernel code also? Since they are unused they could perhaps be moved
> to comments as a compromise?

How about just keeping them as is? I will just not remove it.

> 
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v6: Fixed inconstancy with types. Made all the types to u32 for config
>>      value.
>>      Removed few rdt_last_cmd_puts as it is not necessary.
>>      Removed unused config value definitions.
>>      Few more updates to commit message.
>>
>> v5: Introduced resctrl_arch_event_config_get and
>>      resctrl_arch_event_config_get() based on our discussion.
>>     
>> https://lore.kernel.org/lkml/68e861f9-245d-4496-a72e-46fc57d19c62@amd.com/
>>
>> v4: New patch.
>> ---
>>   arch/x86/kernel/cpu/resctrl/internal.h |  21 -----
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 104 ++++++++++++++-----------
>>   include/linux/resctrl.h                |   4 +
>>   3 files changed, 64 insertions(+), 65 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 4d8cc36a8d79..1021227d8c7e 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -32,27 +32,6 @@
>>    */
>>   #define MBM_CNTR_WIDTH_OFFSET_MAX (62 - MBM_CNTR_WIDTH_BASE)
>>   -/* Reads to Local DRAM Memory */
>> -#define READS_TO_LOCAL_MEM        BIT(0)
>> -
>> -/* Reads to Remote DRAM Memory */
>> -#define READS_TO_REMOTE_MEM        BIT(1)
>> -
>> -/* Non-Temporal Writes to Local Memory */
>> -#define NON_TEMP_WRITE_TO_LOCAL_MEM    BIT(2)
>> -
>> -/* Non-Temporal Writes to Remote Memory */
>> -#define NON_TEMP_WRITE_TO_REMOTE_MEM    BIT(3)
>> -
>> -/* Reads to Local Memory the system identifies as "Slow Memory" */
>> -#define READS_TO_LOCAL_S_MEM        BIT(4)
>> -
>> -/* Reads to Remote Memory the system identifies as "Slow Memory" */
>> -#define READS_TO_REMOTE_S_MEM        BIT(5)
>> -
>> -/* Dirty Victims to All Types of Memory */
>> -#define DIRTY_VICTIMS_TO_ALL_MEM    BIT(6)
>> -
>>   /* Max event bits supported */
>>   #define MAX_EVT_CONFIG_BITS        GENMASK(6, 0)
>>   diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 02afd3442876..0047b4eb0ff5 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1605,10 +1605,57 @@ static int rdtgroup_size_show(struct
>> kernfs_open_file *of,
>>   }
>>     struct mon_config_info {
>> +    struct rdt_mon_domain *d;
>>       u32 evtid;
>>       u32 mon_config;
>>   };
>>   +u32 resctrl_arch_event_config_get(struct rdt_mon_domain *d,
>> +                  enum resctrl_event_id eventid)
>> +{
>> +    struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
>> +
>> +    switch (eventid) {
>> +    case QOS_L3_OCCUP_EVENT_ID:
>> +        break;
>> +    case QOS_L3_MBM_TOTAL_EVENT_ID:
>> +        return hw_dom->mbm_total_cfg;
>> +    case QOS_L3_MBM_LOCAL_EVENT_ID:
>> +        return hw_dom->mbm_local_cfg;
>> +    }
>> +
>> +    /* Never expect to get here */
>> +    WARN_ON_ONCE(1);
>> +
>> +    return INVALID_CONFIG_VALUE;
>> +}
>> +
>> +void resctrl_arch_event_config_set(void *info)
>> +{
>> +    struct mon_config_info *mon_info = info;
>> +    struct rdt_hw_mon_domain *hw_dom;
>> +    unsigned int index;
>> +
>> +    index = mon_event_config_index_get(mon_info->evtid);
>> +    if (index == INVALID_CONFIG_INDEX)
>> +        return;
>> +
>> +    wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>> +
>> +    hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
>> +
>> +    switch (mon_info->evtid) {
>> +    case QOS_L3_OCCUP_EVENT_ID:
>> +        break;
>> +    case QOS_L3_MBM_TOTAL_EVENT_ID:
>> +        hw_dom->mbm_total_cfg = mon_info->mon_config;
>> +        break;
>> +    case QOS_L3_MBM_LOCAL_EVENT_ID:
>> +        hw_dom->mbm_local_cfg =  mon_info->mon_config;
>> +        break;
>> +    }
>> +}
>> +
>>   /**
>>    * mon_event_config_index_get - get the hardware index for the
>>    *                              configurable event
>> @@ -1631,33 +1678,11 @@ unsigned int mon_event_config_index_get(u32 evtid)
>>       }
>>   }
>>   -static void mon_event_config_read(void *info)
>> -{
>> -    struct mon_config_info *mon_info = info;
>> -    unsigned int index;
>> -    u64 msrval;
>> -
>> -    index = mon_event_config_index_get(mon_info->evtid);
>> -    if (index == INVALID_CONFIG_INDEX) {
>> -        pr_warn_once("Invalid event id %d\n", mon_info->evtid);
>> -        return;
>> -    }
>> -    rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
>> -
>> -    /* Report only the valid event configuration bits */
>> -    mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
>> -}
>> -
>> -static void mondata_config_read(struct rdt_mon_domain *d, struct
>> mon_config_info *mon_info)
>> -{
>> -    smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_read,
>> mon_info, 1);
>> -}
>> -
>>   static int mbm_config_show(struct seq_file *s, struct rdt_resource *r,
>> u32 evtid)
>>   {
>> -    struct mon_config_info mon_info = {0};
>>       struct rdt_mon_domain *dom;
>>       bool sep = false;
>> +    u32 val;
>>         cpus_read_lock();
>>       mutex_lock(&rdtgroup_mutex);
>> @@ -1666,11 +1691,11 @@ static int mbm_config_show(struct seq_file *s,
>> struct rdt_resource *r, u32 evtid
>>           if (sep)
>>               seq_puts(s, ";");
>>   -        memset(&mon_info, 0, sizeof(struct mon_config_info));
>> -        mon_info.evtid = evtid;
>> -        mondata_config_read(dom, &mon_info);
>> +        val = resctrl_arch_event_config_get(dom, evtid);
>> +        if (val == INVALID_CONFIG_VALUE)
> 
> Can this check and the "break" that follows be dropped? val being
> INVALID_CONFIG_VALUE would be a kernel bug and
> resctrl_arch_event_config_get()
> would already have printed the WARN. In this unlikely scenario I find it
> unexpected that mbm_config_show() will return success in this case and the
> below seq_printf() would handle the printing of INVALID_CONFIG_VALUE without
> issue anyway.

Sure. I will drop the check and break.

> 
>> +            break;
>>   -        seq_printf(s, "%d=0x%02x", dom->hdr.id, mon_info.mon_config);
>> +        seq_printf(s, "%d=0x%02x", dom->hdr.id, val);
>>           sep = true;
>>       }
>>       seq_puts(s, "\n");
> 
> Reinette
> 
> 

-- 
Thanks
Babu Moger