Hi James,
On 8/16/24 11:28, James Morse wrote:
> Hi Babu,
>
> On 06/08/2024 23:00, Babu Moger wrote:
>> Provide the interface to list the monitor states of all the resctrl
>> groups in ABMC mode.
>>
>> Example:
>> $cat /sys/fs/resctrl/info/L3_MON/mbm_control
>>
>> List follows the following format:
>>
>> "<CTRL_MON group>/<MON group>/<domain_id>=<flags>"
>>
>> Format for specific type of groups:
>>
>> - Default CTRL_MON group:
>> "//<domain_id>=<flags>"
>>
>> - Non-default CTRL_MON group:
>> "<CTRL_MON group>//<domain_id>=<flags>"
>>
>> - Child MON group of default CTRL_MON group:
>> "/<MON group>/<domain_id>=<flags>"
>>
>> - Child MON group of non-default CTRL_MON group:
>> "<CTRL_MON group>/<MON group>/<domain_id>=<flags>"
>>
>> Flags can be one of the following:
>> t MBM total event is enabled
>> l MBM local event is enabled
>> tl Both total and local MBM events are enabled
>> _ None of the MBM events are enabled
>
>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index d15fd1bde5f4..d7aadca5e4ab 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -965,6 +965,75 @@ static int rdtgroup_num_mbm_cntrs_show(struct kernfs_open_file *of,
>> return 0;
>> }
>>
>> +static char *rdtgroup_mon_state_to_str(struct rdtgroup *rdtgrp,
>> + struct rdt_mon_domain *d, char *str)
>> +{
>> + char *tmp = str;
>> + int index;
>> +
>> + /*
>> + * Query the monitor state for the domain.
>> + * Index 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID
>> + * Index 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
>> + */
>> + index = mon_event_config_index_get(QOS_L3_MBM_TOTAL_EVENT_ID);
>> + if (rdtgrp->mon.cntr_id[index] != MON_CNTR_UNSET &&
>> + test_bit(rdtgrp->mon.cntr_id[index], d->mbm_cntr_map))
>> + *tmp++ = 't';
>> +
>> + index = mon_event_config_index_get(QOS_L3_MBM_LOCAL_EVENT_ID);
>> + if (rdtgrp->mon.cntr_id[index] != MON_CNTR_UNSET &&
>> + test_bit(rdtgrp->mon.cntr_id[index], d->mbm_cntr_map))
>> + *tmp++ = 'l';
>> +
>> + if (tmp == str)
>> + *tmp++ = '_';
>> +
>> + *tmp = '\0';
>> + return str;
>> +}
>> +
>> +static int rdtgroup_mbm_control_show(struct kernfs_open_file *of,
>> + struct seq_file *s, void *v)
>> +{
>> + struct rdt_resource *r = of->kn->parent->priv;
>
>> + struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>
> This is filesystem code, once it moves to /fs/ you can't grab an architecture specific
> struct like this. (suggestion below).
Yes. Correct. I don't need this. I will remove this.
>
>
>> + struct rdt_mon_domain *dom;
>> + struct rdtgroup *rdtg;
>> + char str[10];
>
> Shouldn't new commands that might fail start with rdt_last_cmd_clear()?
Yes. Correct.
>
>
>> + if (!hw_res->mbm_cntr_assign_enabled) {
>
> I think this should be wrapped up as:
> | resctrl_arch_mbm_cntr_assign_test(r)
>
> as this flag is private to the architecture.
I can use resctrl_arch_get_abmc_enabled()
>
>
>> + rdt_last_cmd_puts("ABMC feature is not enabled\n");
>
> lockdep barks that you need to hold rdtgroup_mutex when calling rdt_last_cmd_puts() -
> otherwise this can run in parallel with another syscall.
Ok. Sure. Will fix it.
>
>> + return -EINVAL;
>> + }
>> +
>> + mutex_lock(&rdtgroup_mutex);
>> +
>> + list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list) {
>> + struct rdtgroup *crg;
>> +
>> + seq_printf(s, "%s//", rdtg->kn->name);
>> +
>> + list_for_each_entry(dom, &r->mon_domains, hdr.list)
>> + seq_printf(s, "%d=%s;", dom->hdr.id,
>> + rdtgroup_mon_state_to_str(rdtg, dom, str));
>> + seq_putc(s, '\n');
>> +
>> + list_for_each_entry(crg, &rdtg->mon.crdtgrp_list,
>> + mon.crdtgrp_list) {
>> + seq_printf(s, "%s/%s/", rdtg->kn->name, crg->kn->name);
>> +
>> + list_for_each_entry(dom, &r->mon_domains, hdr.list)
>> + seq_printf(s, "%d=%s;", dom->hdr.id,
>> + rdtgroup_mon_state_to_str(crg, dom, str));
>> + seq_putc(s, '\n');
>> + }
>> + }
>> +
>> + mutex_unlock(&rdtgroup_mutex);
>> + return 0;
>> +}
>
>
> Thanks,
>
> James
>
>
--
Thanks
Babu Moger
