linux-kernel - RE: [PATCH v3 10/26] fs/resctrl: Improve handling for events that can be read from any CPU

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <DS7PR11MB607763D8B912A60A3574D2BAFCBA2@DS7PR11MB6077.namprd11.prod.outlook.com>
Date: Wed, 23 Apr 2025 00:51:37 +0000
From: "Luck, Tony" <tony.luck@...el.com>
To: "Chatre, Reinette" <reinette.chatre@...el.com>
CC: Fenghua Yu <fenghuay@...dia.com>, "Wieczor-Retman, Maciej"
	<maciej.wieczor-retman@...el.com>, Peter Newman <peternewman@...gle.com>,
	James Morse <james.morse@....com>, Babu Moger <babu.moger@....com>, "Drew
 Fustini" <dfustini@...libre.com>, Dave Martin <Dave.Martin@....com>,
	"Keshavamurthy, Anil S" <anil.s.keshavamurthy@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"patches@...ts.linux.dev" <patches@...ts.linux.dev>
Subject: RE: [PATCH v3 10/26] fs/resctrl: Improve handling for events that can
 be read from any CPU

> >> cpumask_any() is just cpumask_first() so it will pick the first CPU in the
> >> online mask that may not be the current CPU.
> >>
> >> fwiw ... there are some optimizations planned in this area that I have not yet studied:
> >> https://lore.kernel.org/lkml/20250407153856.133093-1-yury.norov@gmail.com/
> >
> > I remember Peter complaining[1] about extra context switches when
> > cpumask_any_housekeeping() was introduced, but it seems that the
> > discussion died with no fix applied.
>
> The initial complaint was indeed that reading individual events is slower.
>
> The issue is that the intended use case read from many files at frequent
> intervals and thus becomes vulnerable to any changes in this area that
> really is already a slow path (reading from a file ... taking a mutex ...).
>
> Instead of working on shaving cycles off this path the discussion transitioned
> to resctrl providing better support for the underlying use case. I
> understood that this is being experimented with [2] and last I heard it
> looks promising.
>
> >
> > The blocking problem is that ARM may not be able to read a counter
> > on a tick_nohz CPU because it may need to sleep.
> >
> > Do we need more options for events:
> >
> > 1) Must be read on a CPU in the right domain        // Legacy
> > 2) Can be read from any CPU                 // My addtion
> > 3) Must be read on a "housekeeping" CPU             // James' code in upstream
> > 4) Cannot be read on a tick_nohz CPU                // Could be combined with 1 or 2?
>
> I do not see needing additional complexity here. I think it will be simpler
> to just replace use of cpumask_any_housekeeping() in mon_event_read() with
> open code that supports the particular usage. As I understand it is prohibited
> for all CPUs to be in tick_nohz_full_mask so it looks to me as though the
> existing "if (tick_nohz_full_cpu(cpu))" should never be true (since no CPU is being excluded).
> Also, since mon_event_read() has no need to exclude CPUs, just a cpumask_andnot()
> should suffice to determine what remains of given mask after accounting for all the
> NO_HZ CPUs if tick_nohz_full_enabled().

Maybe there isn’t much complexity to make this "read one counter" better on systems
where reading from any CPU is possible. Taking your advice from the earlier review
the filesystem code can set a flag in the mon_evt structure. struct mon_data and
struct rmid_read can change from holding the event id to holding a pointer to the
mon_evt (as the source of truth).

Then mon_event_read() can just have a simple direct call to mon_event_count()
just before the call to cpumask_any_housekeeping() like this:

		if (evt->any_cpu) {
			mon_event_count(rr);
			goto done;
		}

The "goto done" jumps to the resctrl_arch_mon_ctx_free(r, evt->evtid, rr->arch_mon_ctx);
at the end of mon_event_read()

Folks can still pursue the bulk read of many counters (though I expect you might want
one file per domain, rather than a single file to report everything).

>
> Reinette
>
> >
> >> Reinette
> >
> > [1] https://lore.kernel.org/all/20241031142553.3963058-2-peternewman@google.com/
> >>
>
> [2] https://lore.kernel.org/lkml/CALPaoCgpnVORZfbKVLXDFUZvv8jhpShHPzB3cwdLTZQH1o9ULw@mail.gmail.com/

-Tony