| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c43171f4-48c6-b6c3-d71e-1f23367932d7@amd.com>
Date: Fri, 27 Sep 2024 12:47:21 -0500
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
Babu Moger <babu.moger@....com>, corbet@....net, fenghua.yu@...el.com,
tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com
Cc: x86@...nel.org, hpa@...or.com, paulmck@...nel.org, rdunlap@...radead.org,
tj@...nel.org, peterz@...radead.org, yanjiewtw@...il.com,
kim.phillips@....com, lukas.bulwahn@...il.com, seanjc@...gle.com,
jmattson@...gle.com, leitao@...ian.org, jpoimboe@...nel.org,
rick.p.edgecombe@...el.com, kirill.shutemov@...ux.intel.com,
jithu.joseph@...el.com, kai.huang@...el.com, kan.liang@...ux.intel.com,
daniel.sneddon@...ux.intel.com, pbonzini@...hat.com, sandipan.das@....com,
ilpo.jarvinen@...ux.intel.com, peternewman@...gle.com,
maciej.wieczor-retman@...el.com, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, eranian@...gle.com, james.morse@....com
Subject: Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify
assignment states of the groups
Hi Reinette,
On 9/19/2024 12:59 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 9/4/24 3:21 PM, Babu Moger wrote:
>> Introduce the interface to assign MBM events in mbm_cntr_assign mode.
>>
>> Events can be enabled or disabled by writing to file
>> /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>>
>> 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.
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>> v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
>> Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
>> Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
>> Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
>> Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
>> Removed ABMC reference in FS code.
>> Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
>> Not sure if we need to change the behaviour here. Processed them sequencially right now.
>> Users have the liberty to pass the flags. Restricting it might be a problem later.
>
> Could you please give an example of what problem may be encountered later? An assignment
> like "domain=_lt" seems like a contradiction to me since user space essentially asks
> for "None of the MBM events" as well as "MBM total event" and "MBM local event".
I agree it is contradiction. But user is the one who decides to do that.
I think we should allow it. Also, there is some value to it as well.
"domain=_lt" This will also reset the counters if the total and local
events are assigned earlier this action.
>
>
> ...
>
>> @@ -352,6 +352,98 @@ with the following files:
>> There are four resctrl groups. All the groups have total and local MBM events
>> assigned on domain 0 and 1.
>>
>> + Assignment state can be updated by writing to the interface.
>> +
>> + 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 each 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 MBM event.
>> + + Assign a new MBM event without impacting existing assignments.
>> + - Unassign a MBM event from currently assigned events.
>> +
>> + Examples:
>> + ::
>> +
>> + Initial group status:
>> + # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> + non_default_ctrl_mon_grp//0=tl;1=tl;
>> + non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl;
>> + //0=tl;1=tl;
>> + /child_default_mon_grp/0=tl;1=tl;
>> +
>
> Similar to previous patch, looking at this generated doc does not seem to reflect
> what is intended. Above and below are all formatted as code, the descriptions as
> well as the actual "code".
Sure. Will check again.
>
>> + To update the default group to assign only total MBM event on domain 0:
>> + # echo "//0=t" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +
>> + Assignment status after the update:
>> + # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> + non_default_ctrl_mon_grp//0=tl;1=tl;
>> + non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl;
>> + //0=t;1=tl;
>> + /child_default_mon_grp/0=tl;1=tl;
>> +
>> + To update the MON group child_default_mon_grp to remove total MBM event on domain 1:
>> + # echo "/child_default_mon_grp/1-t" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +
>> + Assignment status after the update:
>> + $ cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> + non_default_ctrl_mon_grp//0=tl;1=tl;
>> + non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl;
>> + //0=t;1=tl;
>> + /child_default_mon_grp/0=tl;1=l;
>> +
>> + To update the MON group non_default_ctrl_mon_grp/child_non_default_mon_grp to
>> + unassign both local and total MBM events on domain 1:
>> + # echo "non_default_ctrl_mon_grp/child_non_default_mon_grp/1=_" >
>> + /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +
>> + Assignment status after the update:
>> + non_default_ctrl_mon_grp//0=tl;1=tl;
>> + non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=_;
>> + //0=t;1=tl;
>> + /child_default_mon_grp/0=tl;1=l;
>> +
>> + To update the default group to add a local MBM event domain 0.
>> + # echo "//0+l" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +
>> + Assignment status after the update:
>> + # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> + non_default_ctrl_mon_grp//0=tl;1=tl;
>> + non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=_;
>> + //0=tl;1=tl;
>> + /child_default_mon_grp/0=tl;1=l;
>> +
>> + To update the non default CTRL_MON group non_default_ctrl_mon_grp to unassign all
>> + the MBM events on all the domains.
>> + # echo "non_default_ctrl_mon_grp//*=_" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +
>> + Assignment status after the update:
>> + #cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> + non_default_ctrl_mon_grp//0=_;1=_;
>> + non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=_;
>> + //0=tl;1=tl;
>> + /child_default_mon_grp/0=tl;1=l;
>> +
>> "max_threshold_occupancy":
>> Read/write file provides the largest value (in
>> bytes) at which a previously used LLC_occupancy
>
> ...
>
>> +static int rdtgroup_process_flags(struct rdt_resource *r,
>> + enum rdt_group_type rtype,
>> + char *p_grp, char *c_grp, char *tok)
>> +{
>> + int op, mon_state, assign_state, unassign_state;
>> + char *dom_str, *id_str, *op_str;
>> + struct rdt_mon_domain *d;
>> + struct rdtgroup *rdtgrp;
>> + unsigned long dom_id;
>> + int ret, found = 0;
>> +
>> + 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");
>
> "_" is not an operation.
Sure. Will remove this charactor.
>
>> + 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 */
>> + list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> + if (d->hdr.id == dom_id) {
>> + found = 1;
>> + break;
>> + }
>> + }
>> +
>> + if (!found) {
>> + rdt_last_cmd_printf("Invalid domain id %ld\n", dom_id);
>> + return -EINVAL;
>> + }
>> +
>> +check_state:
>> + mon_state = rdtgroup_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 (assign_state & ASSIGN_TOTAL) {
>> + ret = rdtgroup_assign_cntr(r, rdtgrp, d, QOS_L3_MBM_TOTAL_EVENT_ID);
>
> hmmm ... wasn't unassign going to happen first? That would potentially make counters
> available to help subsequent assign succeed.
Good point. I will change the order.
>
>> + if (ret)
>> + goto out_fail;
>> + }
>> +
>> + if (assign_state & ASSIGN_LOCAL) {
>> + ret = rdtgroup_assign_cntr(r, rdtgrp, d, QOS_L3_MBM_LOCAL_EVENT_ID);
>> + if (ret)
>> + goto out_fail;
>> + }
>> +
>> + if (unassign_state & ASSIGN_TOTAL) {
>> + ret = rdtgroup_unassign_cntr(r, rdtgrp, d, QOS_L3_MBM_TOTAL_EVENT_ID);
>> + if (ret)
>> + goto out_fail;
>> + }
>> +
>> + if (unassign_state & ASSIGN_LOCAL) {
>> + ret = rdtgroup_unassign_cntr(r, rdtgrp, d, QOS_L3_MBM_LOCAL_EVENT_ID);
>> + if (ret)
>> + goto out_fail;
>> + }
>> +
>> + goto next;
>> +
>> +out_fail:
>> +
>> + return -EINVAL;
>> +}
>> +
>
> Reinette
>
>
Thanks
--
- Babu Moger
Powered by blists - more mailing lists