| 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: <9d59d38b-af1a-46d8-81c4-b426d47d4ed6@amd.com>
Date: Wed, 17 Apr 2024 14:39:07 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Peter Newman <peternewman@...gle.com>
Cc: corbet@....net, fenghua.yu@...el.com, reinette.chatre@...el.com,
tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, 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, maciej.wieczor-retman@...el.com,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, eranian@...gle.com,
james.morse@....com
Subject: Re: [RFC PATCH v3 17/17] x86/resctrl: Introduce interface to modify
assignment states of the groups
Hi Peter,
On 4/17/24 12:45, Peter Newman wrote:
> Hi Babu,
>
> On Thu, Mar 28, 2024 at 6:10 PM Babu Moger <babu.moger@....com> wrote:
>>
>> Introduce rdtgroup_mbm_assign_control_write to assign mbm events.
>> Assignment state can be updated by writing to this interface.
>> Assignment states are applied on all the domains. Assignment on one
>> domain applied on all the domains. User can pass one valid domain and
>> assignment will be updated on all the available domains.
>
> It sounds like you said the same thing 3 times in a row.
Sure. Will change it. With the introduction of domain specific assignment,
I can change it to something like this below.
------------------
"Introduce rdtgroup_mbm_assign_control_write to assign mbm events.
By default, the assignment is applied on all the domains when a new group
is created if the hardware counter is available at the time. This
interface provides the option to modify the assignment specific to each
domain."
------------------
>
>
>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>> index 2d96565501ab..64ec70637c66 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -328,6 +328,77 @@ with the following files:
>> None of events are assigned on this mon group. This is a child
>> monitor group of the non default control mon group.
>>
>> + Assignment state can be updated by writing to this interface.
>> +
>> + NOTE: Assignment on one domain applied on all the domains. User can
>> + pass one valid domain and assignment will be updated on all the
>> + available domains.
>
> How would different assignments to different domains work? If the
> allocations are global, then the allocated monitor ID is available to
> all domains whether they use it or not.
That is correct.
[A] Hardware counters(max 2 per group) are allocated at the group level.
So, those counters are available to all the domains on that group. I will
maintain a bitmap at the domain level. The bitmap will be set on the
domains where assignment is applied and IPIs are sent. IPIs will not be
sent to other domains.
>
>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 9fd37b6c3b24..7f8b1386287a 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1011,6 +1035,215 @@ static int rdtgroup_mbm_assign_control_show(struct kernfs_open_file *of,
>> return 0;
>> }
>>
>> +static struct rdtgroup *resctrl_get_rdtgroup(enum rdt_group_type rtype, char *p_grp, char *c_grp)
>> +{
>> + struct rdtgroup *rdtg, *crg;
>> +
>> + if (rtype == RDTCTRL_GROUP && *p_grp == '\0') {
>> + return &rdtgroup_default;
>> + } else if (rtype == RDTCTRL_GROUP) {
>> + list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list)
>> + if (!strcmp(p_grp, rdtg->kn->name))
>> + return rdtg;
>> + } else if (rtype == RDTMON_GROUP) {
>> + list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list) {
>> + if (!strcmp(p_grp, rdtg->kn->name)) {
>> + list_for_each_entry(crg, &rdtg->mon.crdtgrp_list,
>> + mon.crdtgrp_list) {
>> + if (!strcmp(c_grp, crg->kn->name))
>> + return crg;
>> + }
>> + }
>> + }
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static int resctrl_process_flags(enum rdt_group_type rtype, char *p_grp, char *c_grp, char *tok)
>> +{
>> + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> + int op, mon_state, assign_state, unassign_state;
>> + char *dom_str, *id_str, *op_str;
>> + struct rdtgroup *rdt_grp;
>> + struct rdt_domain *d;
>> + unsigned long dom_id;
>> + int ret, found = 0;
>> +
>> + rdt_grp = resctrl_get_rdtgroup(rtype, p_grp, c_grp);
>> +
>> + if (!rdt_grp) {
>> + 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, "=+-_");
>> +
>> + 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->domains, list) {
>> + if (d->id == dom_id) {
>> + found = 1;
>> + break;
>> + }
>> + }
>> + if (!found) {
>> + rdt_last_cmd_printf("Invalid domain id %ld\n", dom_id);
>> + return -EINVAL;
>> + }
>> +
>> + if (op != '_')
>> + mon_state = str_to_mon_state(dom_str);
>> +
>> + assign_state = 0;
>> + unassign_state = 0;
>> +
>> + switch (op) {
>> + case '+':
>> + assign_state = mon_state;
>> + break;
>> + case '-':
>> + unassign_state = mon_state;
>> + break;
>> + case '=':
>> + assign_state = mon_state;
>> + unassign_state = (ASSIGN_TOTAL | ASSIGN_LOCAL) & ~assign_state;
>> + break;
>> + case '_':
>> + unassign_state = ASSIGN_TOTAL | ASSIGN_LOCAL;
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + if (assign_state & ASSIGN_TOTAL)
>> + ret = rdtgroup_assign_abmc(rdt_grp, QOS_L3_MBM_TOTAL_EVENT_ID,
>> + ASSIGN_TOTAL);
>
> Related to my comments yesterday[1], it seems redundant for an
> interface to need two names for the same event.
Yea. I will remove one of this parameter.
>
>
>> + if (ret)
>> + goto out_fail;
>> +
>> + if (assign_state & ASSIGN_LOCAL)
>> + ret = rdtgroup_assign_abmc(rdt_grp, QOS_L3_MBM_LOCAL_EVENT_ID,
>> + ASSIGN_LOCAL);
>> +
>> + if (ret)
>> + goto out_fail;
>> +
>> + if (unassign_state & ASSIGN_TOTAL)
>> + ret = rdtgroup_unassign_abmc(rdt_grp, QOS_L3_MBM_TOTAL_EVENT_ID,
>> + ASSIGN_TOTAL);
>> + if (ret)
>> + goto out_fail;
>> +
>> + if (unassign_state & ASSIGN_LOCAL)
>> + ret = rdtgroup_unassign_abmc(rdt_grp, QOS_L3_MBM_LOCAL_EVENT_ID,
>> + ASSIGN_LOCAL);
>> + if (ret)
>> + goto out_fail;
>> +
>> + goto next;
>
> I saw that each call to rdtgroup_assign_abmc() allocates a counter.
> Does that mean assigning to multiple domains (in the same or multiple
> commands) allocates a new counter (or pair of counters) in every
> domain?
No. Counters allocation is at group level which is global. Will maintain a
bitmap at the domain to determine if the counter is assigned or unassigned
at the specific domain. Please see the comment above [A].
>
> Thanks!
> -Peter
>
> [1] https://lore.kernel.org/lkml/CALPaoCj_yb_muT78jFQ5gL0wkifohSAVwxMDTm2FX_2YVpANdw@mail.gmail.com/
--
Thanks
Babu Moger
Powered by blists - more mailing lists