linux-kernel - Re: [PATCH] x86/resctrl: Remove unnecessary references to cacheinfo in the resctrl subsystem.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <60f5d0cc-afcf-4ca3-ba55-dc4298d95477@intel.com>
Date: Wed, 28 May 2025 10:38:20 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: qinyuntan <qinyuntan@...ux.alibaba.com>, Tony Luck <tony.luck@...el.com>
CC: "H . Peter Anvin" <hpa@...or.com>, <linux-kernel@...r.kernel.org>,
	<x86@...nel.org>
Subject: Re: [PATCH] x86/resctrl: Remove unnecessary references to cacheinfo
 in the resctrl subsystem.

Hi Qinyun Tan,

On 5/27/25 11:37 PM, qinyuntan wrote:
> On 5/28/25 12:49 PM, Reinette Chatre wrote:

...

>> One issue I can think of here is when there is a usage where the user does
>> task isolation on the numa node boundary. Let's consider the SNC-3 example
>> again with node3, node4, and node5 on the second socket, "L3 cache ID 1".
>> If all CPUs on node3 are in tick_nohz_full_mask while none of the node4 and
>> node5 CPUs are in tick_nohz_full_mask then one of node3's CPUs will get
>> an unnecessary IPI.
>>
> You are right, how about this? First, obtain any cpu in hdr.cpu_mask, and then use the cacheinfo shared_cpu_map of this cpu:
> 
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index 9337787461d2d..d43f438465ad0 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -596,7 +596,8 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>         struct rdtgroup *rdtgrp;
>         struct rdt_resource *r;
>         struct mon_data *md;
> -       int domid, ret = 0;
> +       struct cacheinfo *ci;
> +       int domid, cpu, ret = 0;
> 
>         rdtgrp = rdtgroup_kn_lock_live(of->kn);
>         if (!rdtgrp) {
> @@ -625,8 +626,12 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>                 list_for_each_entry(d, &r->mon_domains, hdr.list) {
>                         if (d->ci_id == domid) {
>                                 rr.ci_id = d->ci_id;
> +                               cpu = cpumask_any(&d->hdr.cpu_mask)
> +                               ci = get_cpu_cacheinfo_level(cpu, RESCTRL_L3_CACHE);
> +                               if (!ci)
> +                                       continue;
>                                 mon_event_read(&rr, r, NULL, rdtgrp,
> -                                              &d->hdr.cpu_mask, evtid, false);
> +                                              &ci->shared_cpu_map, evtid, false);
>                                 goto checkresult;
>                         }
>                 }
> 

This looks good to me. Much better than what I was thinking about.

Apart from the items already mentioned I would like to add a couple of style comments:
- Please order variable declarations at beginning of function in reverse fir tree order.
  For example, above snippet would look like:

        struct rdtgroup *rdtgrp;
        int domid, cpu, ret = 0;
        struct rdt_resource *r;
        struct cacheinfo *ci;
        struct mon_data *md;

- Please align struct member names in tabular format (re. the struct rmid_read changes).
- Please ensure struct descriptions are aligned with the text surrounding it. (re.
  the struct rmid_read and struct rdt_mon_domain changes).

Thank you very much.

Reinette