linux-kernel - Re: [RFC v2 7/9] x86/mm: Introduce Remote Action Request

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: <aCxLMsjkA9dBeKvD@gmail.com>
Date: Tue, 20 May 2025 11:28:18 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Rik van Riel <riel@...riel.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org, x86@...nel.org,
	kernel-team@...a.com, dave.hansen@...ux.intel.com, luto@...nel.org,
	peterz@...radead.org, tglx@...utronix.de, mingo@...hat.com,
	bp@...en8.de, hpa@...or.com, nadav.amit@...il.com,
	Yu-cheng Yu <yu-cheng.yu@...el.com>
Subject: Re: [RFC v2 7/9] x86/mm: Introduce Remote Action Request

* Rik van Riel <riel@...riel.com> wrote:

> From: Yu-cheng Yu <yu-cheng.yu@...el.com>
> 
> Remote Action Request (RAR) is a TLB flushing broadcast facility.
> To start a TLB flush, the initiator CPU creates a RAR payload and
> sends a command to the APIC.  The receiving CPUs automatically flush
> TLBs as specified in the payload without the kernel's involement.
> 
> [ riel: add pcid parameter to smp_call_rar_many so other mms can be flushed ]

Please actually review & tidy up patches that you pass through, don't 
just hack them up minimally and slap your tag and SOB on top of it.

One example, of many:

> +	 * We allow cpu's that are not yet online though, as no one else can

Here the comment has 'CPU' in lowercase, and with a grammar mistake.

> +	 * send smp call function interrupt to this cpu and as such deadlocks

Here 'CPU' is in lowercase.

> +	/* Try to fastpath.  So, what's a CPU they want?  Ignoring this one. */

Oh, here 'CPU' is uppercase again! What happened?

> +	/* No online cpus?  We're done. */

Lowercase again. Damn, I thought we settled on a way to spell this 
thing already.

> +	/* Do we have another CPU which isn't us? */

And uppercase. What a roller-coaster.

> +	/* Fastpath: do that cpu by itself. */
> +	/* Some callers race with other cpus changing the passed mask */

And lowercase.

> +	/* Send a message to all CPUs in the map */

And uppercase.

It's almost as if nobody has ever read these comments after writing 
them.

There's like a zillion small random-noise details through the entire 
series that insert unnecessary extra white noise in critical system 
code that should be a lot more carefully written, which emits a foul 
aura of carelessness. Reviewers should not be forced to point these out 
to you, in fact reviewers should not be exposed to such noise at all.

Please review the entire thing *much* more carefully before submitting 
-v3.

Thanks,

	Ingo