| 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: <1cd5f0a7-2478-41b8-97cc-413fa19205dd@intel.com>
Date: Wed, 17 Sep 2025 23:03:39 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....com>, <corbet@....net>, <tony.luck@...el.com>,
<Dave.Martin@....com>, <james.morse@....com>, <tglx@...utronix.de>,
<mingo@...hat.com>, <bp@...en8.de>, <dave.hansen@...ux.intel.com>
CC: <x86@...nel.org>, <hpa@...or.com>, <kas@...nel.org>,
<rick.p.edgecombe@...el.com>, <akpm@...ux-foundation.org>,
<paulmck@...nel.org>, <pmladek@...e.com>,
<pawan.kumar.gupta@...ux.intel.com>, <rostedt@...dmis.org>,
<kees@...nel.org>, <arnd@...db.de>, <fvdl@...gle.com>, <seanjc@...gle.com>,
<thomas.lendacky@....com>, <manali.shukla@....com>, <perry.yuan@....com>,
<sohil.mehta@...el.com>, <xin@...or.com>, <peterz@...radead.org>,
<mario.limonciello@....com>, <gautham.shenoy@....com>, <nikunj@....com>,
<dapeng1.mi@...ux.intel.com>, <ak@...ux.intel.com>,
<chang.seok.bae@...el.com>, <ebiggers@...gle.com>,
<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-coco@...ts.linux.dev>, <kvm@...r.kernel.org>
Subject: Re: [PATCH v9 09/10] fs/resctrl: Introduce interface to modify
io_alloc Capacity Bit Masks
Hi Babu,
On 9/2/25 3:41 PM, Babu Moger wrote:
> The io_alloc feature in resctrl enables system software to configure the
> portion of the cache allocated for I/O traffic. When supported, the
> io_alloc_cbm file in resctrl provides access to Capacity Bit Masks (CBMs)
> reserved for I/O devices.
reserved -> allocated?
The cache portions represented by CBMs are not reserved for I/O devices - these
portions are available for sharing and can still be used by CPU cache allocation.
>
> Enable users to modify io_alloc CBMs (Capacity Bit Masks) via the
Can drop "(Capacity Bit Masks)" since acronym was spelled out in first paragraph.
> io_alloc_cbm resctrl file when io_alloc is enabled.
>
> To ensure consistent cache allocation when CDP is enabled, the CBMs
This is not about "consistent cache allocation" but instead a consistent user
interface. How about "To present consistent I/O allocation information to user
space when CDP is enabled, the CBMs ..."
> written to either L3CODE or L3DATA are mirrored to the other, keeping both
> resource types synchronized.
(needs imperative)
>
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---
...
> ---
> Documentation/filesystems/resctrl.rst | 11 ++++
> fs/resctrl/ctrlmondata.c | 93 +++++++++++++++++++++++++++
> fs/resctrl/internal.h | 3 +
> fs/resctrl/rdtgroup.c | 3 +-
> 4 files changed, 109 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 15e3a4abf90e..7e3eda324de5 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -188,6 +188,17 @@ related to allocation:
> # cat /sys/fs/resctrl/info/L3/io_alloc_cbm
> 0=ffff;1=ffff
>
> + CBMs can be configured by writing to the interface.
> +
> + Example::
> +
> + # echo 1=ff > /sys/fs/resctrl/info/L3/io_alloc_cbm
> + # cat /sys/fs/resctrl/info/L3/io_alloc_cbm
> + 0=ffff;1=00ff
> + # echo 0=ff;1=f > /sys/fs/resctrl/info/L3/io_alloc_cbm
To accommodate how a shell may interpret above this should perhaps be (see schemata examples):
# echo "0=ff;1=f" > /sys/fs/resctrl/info/L3/io_alloc_cbm
> + # cat /sys/fs/resctrl/info/L3/io_alloc_cbm
> + 0=00ff;1=000f
> +
> When CDP is enabled "io_alloc_cbm" associated with the DATA and CODE
> resources may reflect the same values. For example, values read from and
> written to /sys/fs/resctrl/info/L3DATA/io_alloc_cbm may be reflected by
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index a4e861733a95..791ecb559b50 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -848,3 +848,96 @@ int resctrl_io_alloc_cbm_show(struct kernfs_open_file *of, struct seq_file *seq,
> cpus_read_unlock();
> return ret;
> }
> +
> +static int resctrl_io_alloc_parse_line(char *line, struct rdt_resource *r,
> + struct resctrl_schema *s, u32 closid)
> +{
> + enum resctrl_conf_type peer_type;
> + struct rdt_parse_data data;
> + struct rdt_ctrl_domain *d;
> + char *dom = NULL, *id;
> + unsigned long dom_id;
> +
> +next:
> + if (!line || line[0] == '\0')
> + return 0;
> +
> + dom = strsep(&line, ";");
> + id = strsep(&dom, "=");
> + if (!dom || kstrtoul(id, 10, &dom_id)) {
> + rdt_last_cmd_puts("Missing '=' or non-numeric domain\n");
> + return -EINVAL;
> + }
> +
> + dom = strim(dom);
> + list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
> + if (d->hdr.id == dom_id) {
> + data.buf = dom;
> + data.mode = RDT_MODE_SHAREABLE;
> + data.closid = closid;
> + if (parse_cbm(&data, s, d))
> + return -EINVAL;
> + /*
> + * When CDP is enabled, update the schema for both CDP_DATA
> + * and CDP_CODE.
The comment just describes what can be seen from the code. How about something like
"Keep io_alloc CLOSID's CBM of CDP_CODE and CDP_DATA in sync."?
Of note is that these comments are generic while earlier comments related to CDP are L3
specific ("L3CODE" and "L3DATA"). Having resource specific names in generic code is not ideal,
even if first implementation is only for L3. I think this was done in many places though,
even in a couple of the changelogs I created and I now realize the impact after seeing
this comment. Could you please take a look to make the name generic when it is used in
generic changelog and comments?
> + */
> + if (resctrl_arch_get_cdp_enabled(r->rid)) {
> + peer_type = resctrl_peer_type(s->conf_type);
> + memcpy(&d->staged_config[peer_type],
> + &d->staged_config[s->conf_type],
> + sizeof(d->staged_config[0]));
> + }
> + goto next;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +ssize_t resctrl_io_alloc_cbm_write(struct kernfs_open_file *of, char *buf,
> + size_t nbytes, loff_t off)
> +{
> + struct resctrl_schema *s = rdt_kn_parent_priv(of->kn);
> + struct rdt_resource *r = s->res;
> + u32 io_alloc_closid;
> + int ret = 0;
> +
> + /* Valid input requires a trailing newline */
> + if (nbytes == 0 || buf[nbytes - 1] != '\n')
> + return -EINVAL;
> +
> + buf[nbytes - 1] = '\0';
> +
> + cpus_read_lock();
> + mutex_lock(&rdtgroup_mutex);
> + rdt_last_cmd_clear();
> +
> + if (!r->cache.io_alloc_capable) {
> + rdt_last_cmd_printf("io_alloc is not supported on %s\n", s->name);
> + ret = -ENODEV;
> + goto out_unlock;
> + }
> +
> + if (!resctrl_arch_get_io_alloc_enabled(r)) {
> + rdt_last_cmd_printf("io_alloc is not enabled on %s\n", s->name);
> + ret = -ENODEV;
Compare to comment in patch #7 where the same error of io_alloc not being enabled results
in different error code (EINVAL). Please keep these consistent.
> + goto out_unlock;
> + }
> +
> + io_alloc_closid = resctrl_io_alloc_closid(r);
> +
> + rdt_staged_configs_clear();
> + ret = resctrl_io_alloc_parse_line(buf, r, s, io_alloc_closid);
> + if (ret)
> + goto out_clear_configs;
> +
> + ret = resctrl_arch_update_domains(r, io_alloc_closid);
> +
> +out_clear_configs:
> + rdt_staged_configs_clear();
> +out_unlock:
> + mutex_unlock(&rdtgroup_mutex);
> + cpus_read_unlock();
> +
> + return ret ?: nbytes;
> +}
Reinette
Powered by blists - more mailing lists