| 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
| ||
|
Date: Thu, 31 Aug 2023 10:42:39 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: <babu.moger@....com>, <corbet@....net>, <tglx@...utronix.de>,
<mingo@...hat.com>, <bp@...en8.de>
CC: <fenghua.yu@...el.com>, <dave.hansen@...ux.intel.com>,
<x86@...nel.org>, <hpa@...or.com>, <paulmck@...nel.org>,
<akpm@...ux-foundation.org>, <quic_neeraju@...cinc.com>,
<rdunlap@...radead.org>, <damien.lemoal@...nsource.wdc.com>,
<songmuchun@...edance.com>, <peterz@...radead.org>,
<jpoimboe@...nel.org>, <pbonzini@...hat.com>,
<chang.seok.bae@...el.com>, <pawan.kumar.gupta@...ux.intel.com>,
<jmattson@...gle.com>, <daniel.sneddon@...ux.intel.com>,
<sandipan.das@....com>, <tony.luck@...el.com>,
<james.morse@....com>, <linux-doc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <bagasdotme@...il.com>,
<eranian@...gle.com>, <christophe.leroy@...roup.eu>,
<jarkko@...nel.org>, <adrian.hunter@...el.com>,
<quic_jiles@...cinc.com>, <peternewman@...gle.com>
Subject: Re: [PATCH v8 8/8] x86/resctrl: Display hardware ids of resource
groups
Hi Babu,
On 8/30/2023 4:19 PM, Moger, Babu wrote:
> On 8/29/23 15:14, Reinette Chatre wrote:
>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>
>> ...
>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index 2fae6d9e24d3..3fa32c1c2677 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -296,9 +296,15 @@ struct rdtgroup {
>>> * --> RFTYPE_BASE (Files common for both MON and CTRL groups)
>>> * Files: cpus, cpus_list, tasks
>>> *
>>> + * --> RFTYPE_DEBUG (Files to help resctrl debugging)
>>> + * File: mon_hw_id
>>> + *
>>
>> This does not look right. I think mon_hw_id should have RFTYPE_MON
>> (more below).
>
> I am not sure about this (more below).
>
>>
>>> * --> RFTYPE_CTRL (Files only for CTRL group)
>>> * Files: mode, schemata, size
>>> *
>>> + * --> RFTYPE_DEBUG (Files to help resctrl debugging)
>>> + * File: ctrl_hw_id
>>> + *
>>> */
>>> #define RFTYPE_INFO BIT(0)
>>> #define RFTYPE_BASE BIT(1)
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index 94bd69f9964c..e0c2479acf49 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -776,6 +776,38 @@ static int rdtgroup_tasks_show(struct kernfs_open_file *of,
>>> return ret;
>>> }
>>>
>>> +static int rdtgroup_closid_show(struct kernfs_open_file *of,
>>> + struct seq_file *s, void *v)
>>> +{
>>> + struct rdtgroup *rdtgrp;
>>> + int ret = 0;
>>> +
>>> + rdtgrp = rdtgroup_kn_lock_live(of->kn);
>>> + if (rdtgrp)
>>> + seq_printf(s, "%u\n", rdtgrp->closid);
>>> + else
>>> + ret = -ENOENT;
>>> + rdtgroup_kn_unlock(of->kn);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int rdtgroup_rmid_show(struct kernfs_open_file *of,
>>> + struct seq_file *s, void *v)
>>> +{
>>> + struct rdtgroup *rdtgrp;
>>> + int ret = 0;
>>> +
>>> + rdtgrp = rdtgroup_kn_lock_live(of->kn);
>>> + if (rdtgrp)
>>> + seq_printf(s, "%u\n", rdtgrp->mon.rmid);
>>> + else
>>> + ret = -ENOENT;
>>> + rdtgroup_kn_unlock(of->kn);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> #ifdef CONFIG_PROC_CPU_RESCTRL
>>>
>>> /*
>>> @@ -1837,6 +1869,13 @@ static struct rftype res_common_files[] = {
>>> .seq_show = rdtgroup_tasks_show,
>>> .fflags = RFTYPE_BASE,
>>> },
>>> + {
>>> + .name = "mon_hw_id",
>>> + .mode = 0444,
>>> + .kf_ops = &rdtgroup_kf_single_ops,
>>> + .seq_show = rdtgroup_rmid_show,
>>> + .fflags = RFTYPE_BASE | RFTYPE_DEBUG,
>>
>> I missed this earlier but I think this also needs a RFTYPE_MON.
>> Perhaps patch 3 can introduce a new RFTYPE_MON_BASE to not
>> have the flags of the two new files look too different?
>
> We have two types of files in base directory structure.
>
> if (rtype == RDTCTRL_GROUP)
> files = RFTYPE_BASE | RFTYPE_CTRL;
> else
> files = RFTYPE_BASE | RFTYPE_MON;
>
> 1. RFTYPE_BASE | RFTYPE_CTRL;
> Files for the control group only.
>
> 2. RFTYPE_BASE | RFTYPE_MON;
> Files for both control and mon groups. However, RFTYPE_MON is not used
> for any files. It is only RFTYPE_BASE.
>
> Because of the check in rdtgroup_add_files it all works fine.
> For the control group it creates files with RFTYPE_BASE | RFTYPE_CTRL and
> RFTYPE_BASE.
>
> For the mon group it creates files with RFTYPE_BASE only.
This describes current behavior because there are no resctrl
files in base that are specific to monitoring, mon_hw_id is the
first.
This does not mean that the new file mon_hw_id should just have
RFTYPE_BASE because that would result in mon_hw_id being created
for all control groups, even those that do not support monitoring
Having mon_hw_id in resctrl for a group that does not support
monitoring is not correct.
You should be able to reproduce this when booting your system
with rdt=!cmt,!mbmlocal,!mbmtotal.
>
> Adding FTYPE_MON_BASE will be a problem.
>
Yes, this change does not just involve assigning the RFTYPE_MON
to mon_hw_id. As you describe mkdir_rdt_prepare() does not take
RFTYPE_MON into account when creating the files. Could this not just
be a straightforward change to have it append RFTYPE_MON to the flags
of files needing to be created for a CTRL_MON group? This would
support new resource groups and then the default resource group
would need to be taken into account also. What am I missing?
Reinette
Powered by blists - more mailing lists