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

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

Hi Reinette,

On 7/12/24 17:10, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/3/24 2:48 PM, Babu Moger wrote:
>> The event configuration is domain specific and initialized during domain
>> initialization. It is not required to read the configuration register
>> every time user asks for it. Use the value stored in rdt_mon_hw_domain
> 
> rdt_mon_hw_domain -> rdt_hw_mon_domain
> 
>> instead. Also update the configuration value when user writes it.
> 
> Please separate the context/problem/solution clearly.

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.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> 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/rdtgroup.c | 112 +++++++++++++++----------
>>   include/linux/resctrl.h                |   4 +
>>   2 files changed, 72 insertions(+), 44 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index b2b751741dd8..91c5d45ac367 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1591,10 +1591,59 @@ static int rdtgroup_size_show(struct
>> kernfs_open_file *of,
>>   }
>>     struct mon_config_info {
>> +    struct rdt_mon_domain *d;
>>       u32 evtid;
>>       u32 mon_config;
>>   };
> 
> as seen above, mon_config is a u32
> 
>>   +#define INVALID_CONFIG_VALUE   UINT_MAX
> 
> So an invalid config value can be U32_MAX?

Sure.

> 
>> +
>> +unsigned int 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_VALUE) {
> 
> INVALID_CONFIG_INDEX?

Yes.

> 
>> +        pr_warn_once("Invalid event id %d\n", mon_info->evtid);
>> +        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;
> 
> Please add a break here.

Sure.
> 
>> +    }
>> +}
>> +
>>   #define INVALID_CONFIG_INDEX   UINT_MAX
>>     /**
>> @@ -1619,33 +1668,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;
>> +    int val;
>>         cpus_read_lock();
>>       mutex_lock(&rdtgroup_mutex);
>> @@ -1654,11 +1681,13 @@ 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);
> 
> There are too many types used interchangeably. The mon_config is a "u32",
> but the new function
> returns "unsigned int", which is then assigned to an "int". Please just
> use one type
> consistently, it is a u32 so resctrl_arch_event_config_get() can return
> u32 and "val" should
> be u32.
> 

Sure. Will do.

>> +        if (val == INVALID_CONFIG_VALUE) {
>> +            rdt_last_cmd_puts("Invalid event configuration\n");
> 
> I do not see a reason to print message to user space here. If this error
> is encountered
> then it is a kernel bug and resctrl_arch_event_config_get() would already
> have triggered
> a WARN.
> 
> Since this is a "never should happen" scenario I wonder if we can not just
> print
> the INVALID_CONFIG_VALUE to user space?

Ok. Will remove the check and the message.

> 
> 
>> +            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");
>> @@ -1689,33 +1718,27 @@ static int mbm_local_bytes_config_show(struct
>> kernfs_open_file *of,
>>       return 0;
>>   }
>>   -static void mon_event_config_write(void *info)
>> -{
>> -    struct mon_config_info *mon_info = info;
>> -    unsigned int index;
>> -
>> -    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;
>> -    }
>> -    wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>> -}
>>     static void mbm_config_write_domain(struct rdt_resource *r,
>>                       struct rdt_mon_domain *d, u32 evtid, u32 val)
>>   {
>>       struct mon_config_info mon_info = {0};
>> +    int config_val;
>>         /*
>> -     * Read the current config value first. If both are the same then
>> +     * Check the current config value first. If both are the same then
>>        * no need to write it again.
>>        */
>> -    mon_info.evtid = evtid;
>> -    mondata_config_read(d, &mon_info);
>> -    if (mon_info.mon_config == val)
>> +    config_val = resctrl_arch_event_config_get(d, evtid);
>> +    if (config_val == INVALID_CONFIG_VALUE) {
>> +        rdt_last_cmd_puts("Invalid event configuration\n");
> 
> same here about unneeded print to user space. When this is encountered it is
> a kernel bug.

Ok. Will remove the check and the message.

> 
>> +        return;
>> +    }
>> +    if (config_val == val)
>>           return;
>>   +    mon_info.d = d;
>> +    mon_info.evtid = evtid;
>>       mon_info.mon_config = val;
>>         /*
>> @@ -1724,7 +1747,8 @@ static void mbm_config_write_domain(struct
>> rdt_resource *r,
>>        * are scoped at the domain level. Writing any of these MSRs
>>        * on one CPU is observed by all the CPUs in the domain.
>>        */
>> -    smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_write,
>> +    smp_call_function_any(&d->hdr.cpu_mask,
>> +                  resctrl_arch_event_config_set,
>>                     &mon_info, 1);
>>         /*
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index 62f0f002ef41..f017258ebf85 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -352,6 +352,10 @@ void resctrl_arch_reset_rmid(struct rdt_resource
>> *r, struct rdt_mon_domain *d,
>>    */
>>   void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct
>> rdt_mon_domain *d);
>>   +void resctrl_arch_event_config_set(void *info);
>> +unsigned int resctrl_arch_event_config_get(struct rdt_mon_domain *d,
>> +                       enum resctrl_event_id eventid);
>> +
>>   extern unsigned int resctrl_rmid_realloc_threshold;
>>   extern unsigned int resctrl_rmid_realloc_limit;
>>   
> 
> Reinette
> 

-- 
Thanks
Babu Moger