Hi James,
On 2/21/25 12:07, James Morse wrote:
> Hi Babu,
>
> On 22/01/2025 20:20, Babu Moger wrote:
>> When mbm_cntr_assign mode is enabled, users can designate which of the MBM
>> events in the CTRL_MON or MON groups should have counters assigned.
>>
>> Provide an interface for assigning MBM events by writing to the file:
>> /sys/fs/resctrl/info/L3_MON/mbm_assign_control. Using this interface,
>> events can be assigned or unassigned as needed.
>>
>> Format is similar to the list format with addition of opcode for the
>> assignment operation.
>> "<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"
>>
>> Format for specific type of groups:
>>
>> * Default CTRL_MON group:
>> "//<domain_id><opcode><flags>"
>>
>> * Non-default CTRL_MON group:
>> "<CTRL_MON group>//<domain_id><opcode><flags>"
>>
>> * Child MON group of default CTRL_MON group:
>> "/<MON group>/<domain_id><opcode><flags>"
>>
>> * Child MON group of non-default CTRL_MON group:
>> "<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"
>>
>> Domain_id '*' will apply the flags on all the domains.
>>
>> Opcode can be one of the following:
>>
>> = Update the assignment to match the flags
>> + Assign a new MBM event without impacting existing assignments.
>> - Unassign a MBM event from currently assigned events.
>>
>> Assignment flags can be one of the following:
>> t MBM total event
>> l MBM local event
>> tl Both total and local MBM events
>> _ None of the MBM events. Valid only with '=' opcode. This flag cannot
>> be combined with other flags.
>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 6e29827239e0..299839bcf23f 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1050,6 +1050,244 @@ static int resctrl_mbm_assign_control_show(struct kernfs_open_file *of,
>
>> +static int resctrl_process_flags(struct rdt_resource *r,
>> + enum rdt_group_type rtype,
>> + char *p_grp, char *c_grp, char *tok)
>> +{
>> + unsigned int op, mon_state, assign_state, unassign_state;
>> + char *dom_str, *id_str, *op_str;
>> + struct rdt_mon_domain *d;
>> + unsigned long dom_id = 0;
>> + struct rdtgroup *rdtgrp;
>> + char domain[10];
>> + bool found;
>> + int ret;
>> +
>> + rdtgrp = rdtgroup_find_grp_by_name(rtype, p_grp, c_grp);
>> +
>> + if (!rdtgrp) {
>> + rdt_last_cmd_puts("Not a valid resctrl group\n");
>> + return -EINVAL;
>> + }
>> +
>> +next:
>> + if (!tok || tok[0] == '\0')
>> + return 0;
>> +
>> + /* Start processing the strings for each domain */
>> + dom_str = strim(strsep(&tok, ";"));
>> +
>> + op_str = strpbrk(dom_str, "=+-");
>> +
>> + if (op_str) {
>> + op = *op_str;
>> + } else {
>> + rdt_last_cmd_puts("Missing operation =, +, - character\n");
>> + return -EINVAL;
>> + }
>> +
>> + id_str = strsep(&dom_str, "=+-");
>> +
>> + /* Check for domain id '*' which means all domains */
>> + if (id_str && *id_str == '*') {
>> + d = NULL;
>> + goto check_state;
>> + } else if (!id_str || kstrtoul(id_str, 10, &dom_id)) {
>> + rdt_last_cmd_puts("Missing domain id\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Verify if the dom_id is valid */
>> + found = false;
>> + list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> + if (d->hdr.id == dom_id) {
>> + found = true;
>> + break;
>> + }
>> + }
>> +
>> + if (!found) {
>> + rdt_last_cmd_printf("Invalid domain id %ld\n", dom_id);
>> + return -EINVAL;
>> + }
>> +
>> +check_state:
>> + mon_state = resctrl_str_to_mon_state(dom_str);
>> +
>> + if (mon_state == ASSIGN_INVALID) {
>> + rdt_last_cmd_puts("Invalid assign flag\n");
>> + goto out_fail;
>> + }
>> +
>> + assign_state = 0;
>> + unassign_state = 0;
>> +
>> + switch (op) {
>> + case '+':
>> + if (mon_state == ASSIGN_NONE) {
>> + rdt_last_cmd_puts("Invalid assign opcode\n");
>> + goto out_fail;
>> + }
>> + assign_state = mon_state;
>> + break;
>> + case '-':
>> + if (mon_state == ASSIGN_NONE) {
>> + rdt_last_cmd_puts("Invalid assign opcode\n");
>> + goto out_fail;
>> + }
>> + unassign_state = mon_state;
>> + break;
>> + case '=':
>> + assign_state = mon_state;
>> + unassign_state = (ASSIGN_TOTAL | ASSIGN_LOCAL) & ~assign_state;
>> + break;
>> + default:
>> + break;
>> + }
>
>
>> + if (unassign_state & ASSIGN_TOTAL) {
>> + ret = resctrl_unassign_cntr_event(r, d, rdtgrp, QOS_L3_MBM_TOTAL_EVENT_ID);
>> + if (ret)
>> + goto out_fail;
>> + }
>> +
>> + if (unassign_state & ASSIGN_LOCAL) {
>> + ret = resctrl_unassign_cntr_event(r, d, rdtgrp, QOS_L3_MBM_LOCAL_EVENT_ID);
>> + if (ret)
>> + goto out_fail;
>> + }
>> +
>> + if (assign_state & ASSIGN_TOTAL) {
>> + ret = resctrl_assign_cntr_event(r, d, rdtgrp, QOS_L3_MBM_TOTAL_EVENT_ID);
>> + if (ret)
>> + goto out_fail;
>> + }
>> +
>> + if (assign_state & ASSIGN_LOCAL) {
>> + ret = resctrl_assign_cntr_event(r, d, rdtgrp, QOS_L3_MBM_LOCAL_EVENT_ID);
>> + if (ret)
>> + goto out_fail;
>> + }
>
> This sequence of if's allows the helpers to be called on platforms that doesn't support
> both local and total. Could we reject such misconfiguration here in the parsing code?
> You have these checks in rdtgroup_assign_cntrs() added in patch 17.
>
Yes. Added the check is_mbm_total_enabled() and is_mbm_local_enabled().
>
> What do you think to trying to group these four by event type, and passing the event type
> in as an argument? ... it ends up with a helper that takes a large number of arguments,
> (both assign_state and unassign_state), but there is less repetition...
Added a new helper function resctrl_process_cntr_event(). I could not
avoid the repetitions.
>
>
> Thanks,
>
> James
>
>> + goto next;
>> +
>> +out_fail:
>> + sprintf(domain, d ? "%ld" : "*", dom_id);
>> +
>> + rdt_last_cmd_printf("Assign operation '%s%c%s' failed on the group %s/%s/\n",
>> + domain, op, dom_str, p_grp, c_grp);
>> +
>> + return -EINVAL;
>> +}
>> +
>
--
Thanks
Babu Moger
