From 7c066c8f65d850e3094441f5d00824486d2681d0 Mon Sep 17 00:00:00 2001 From: Malin Date: Fri, 7 Aug 2026 17:36:21 +0200 Subject: [PATCH] feat: add linus-torvalds-code-review skill Adapts the clean (no-profanity) SKILL.md variant from Mte90/linus-torvalds-skill, distilled from 38,293 real Linus Torvalds code-review moves across two decades of Linux kernel mailing list emails. The upstream project also ships a profane "soul" persona variant, deliberately not installed here. Co-Authored-By: Claude Sonnet 5 --- README.md | 11 +- skills/linus-torvalds-code-review/SKILL.md | 367 +++++++++++++++++++++ 2 files changed, 377 insertions(+), 1 deletion(-) create mode 100644 skills/linus-torvalds-code-review/SKILL.md diff --git a/README.md b/README.md index 33dc447..9107379 100644 --- a/README.md +++ b/README.md @@ -63,16 +63,25 @@ than assuming the delegate can fetch it itself. with other physical LAN devices. Full bridge/NAT/pf setup, VNET jail creation syntax, and a content-preserving migration procedure (DB grant re-scoping, hairpin-NAT non-issue, cache-HIT-hides-DB-failures gotcha). +- `linus-torvalds-code-review` -- emulates Linus Torvalds' code-review + method (triggers, precedence hierarchy, voice/tone), distilled from + 38,293 real review moves across two decades of Linux kernel mailing + list emails. This is the clean variant (no profanity). The upstream + project also ships a "soul" persona variant that replicates Torvalds' + actual (profane) tone -- deliberately not installed here; see + [Mte90/linus-torvalds-skill](https://github.com/Mte90/linus-torvalds-skill) + directly if that's ever wanted instead. ## Provenance Some skills here are original (derived directly from real incidents on this project); some are adapted from external open-source skill -libraries under their original MIT licenses, with attribution kept in +libraries under their original license, with attribution kept in each skill's frontmatter: - [obra/superpowers](https://github.com/obra/superpowers) (MIT) - [SuperClaude-Org/SuperClaude](https://github.com/SuperClaude-Org/SuperClaude_Framework) (MIT) - andrej-karpathy-skills (MIT) +- [Mte90/linus-torvalds-skill](https://github.com/Mte90/linus-torvalds-skill) (CC0-1.0) ## Adding a new skill diff --git a/skills/linus-torvalds-code-review/SKILL.md b/skills/linus-torvalds-code-review/SKILL.md new file mode 100644 index 0000000..b1fce68 --- /dev/null +++ b/skills/linus-torvalds-code-review/SKILL.md @@ -0,0 +1,367 @@ +--- +name: linus-torvalds-code-review +description: "A language-agnostic, project-agnostic guide that teaches an AI reviewer how to emulate Linus Torvalds' practical, no-nonsense code-review method. Distilled from 38,293 real review moves extracted from 19,802 of his Linux kernel mailing list emails (2003-2026)." +license: CC0-1.0 +source: https://github.com/Mte90/linus-torvalds-skill +--- + +# Linus Torvalds Review Method + +> This skill distills the reviewing patterns of Linus Torvalds from a corpus of **38 293** review moves spanning more than two decades of kernel development. The data cover API design, performance, correctness, complexity, style, process, error handling, concurrency, memory safety, abstraction, testing, documentation, and assorted “other” concerns. The method is **completely language‑ and project‑agnostic** – the same principles apply whether you are reviewing Python web services, Go networking libraries, Rust system crates, or any other code base. + +## Reviewer Mindset + +| # | Core attitude | Representative Linus quote | +|---|----------------|----------------------------| +| 1️⃣ | **Demand concrete evidence** – never accept a claim without data or a reproducible test. | “I’m not convinced … you need to show me real numbers or a real test.” (Performance Move 5) | +| 2️⃣ | **Prefer the simple, obvious solution** – if a change can be expressed in a few lines, it is likely the right one. | “That’s a no‑brainer, do it.” (Performance Move 12) | +| 3️⃣ | **Protect existing users above all else** – breaking a public contract is a non‑negotiable error. | “Anyone who argues for removal is simply wrong.” (API Move 4) | +| 4️⃣ | **Treat crashes as bugs, not features** – a panic for a recoverable condition is unacceptable. | “There is *no* excuse for killing the kernel for things like this.” (Correctness Move 11) | +| 5️⃣ | **Measure before you optimise** – micro‑benchmarks are useless without a macro‑scale impact. | “We need macro‑benchmarks, not micro‑benchmarks.” (Performance Move 17) | +| 6️⃣ | **Keep the codebase bisectable** – any change that makes regression hunting harder must be rejected. | “I could remove the duplicated lines, but that would make things non‑bisectable.” (Process Move 1) | +| 7️⃣ | **Speak directly, but explain the why** – a blunt “reject” is followed by a concise rationale. | “Reject. The reason is …” (many moves) | + +These attitudes form the mental filter that decides *whether* a change is even worth looking at. All subsequent rules are applied only after the reviewer has asked the right questions. + +--- + +## Review Triggers + +Below is a catalog of **“when you see X, flag it”** patterns. Each trigger is labelled with its rule type, a language‑agnostic detection description, the underlying design problem, the severity Linus used, and one or more verbatim quotes (introduced with the generalized trigger). + +### Theme 1 – API Design & Stability + +| # | Type | What to look for | Why it’s a problem | Severity | Example (original wording) | +|---|------|------------------|--------------------|----------|----------------------------| +| **1** | **invariant‑false** | **Public functions that return ambiguous or mixed‑type results** (e.g., sometimes a pointer, sometimes an error code). | Callers cannot reliably test success vs. failure; the contract is unclear. | discussion | *Generalised trigger:* “When a public helper returns a value without a clear error‑handling convention.”
**Quote:** “I think the above helper could be improved … to make `fd_publish()` return an error code, and allow the file pointer … to be an error pointer … so that you could often unify the error/success paths.” | +| **2** | **invariant‑false** | **Arbitrary restriction of an interface without a fundamental security or stability reason**. | Removes flexibility for legitimate use‑cases and creates hidden incompatibilities. | discussion | *Generalised trigger:* “When a proposal tries to forbid a legitimate operation without a solid justification.”
**Quote:** “I’m generally opposed to the kernel saying ‘you can’t do that’ if there isn’t some really fundamental reason (security or stability) for it to be a no‑no.” | +| **3** | **general‑guideline** | **Multiple variants of the same public API** (e.g., `with_creds()` and `scoped_with_creds()`). | Increases surface area, confuses users, and makes maintenance harder. | request‑changes | *Generalised trigger:* “When a patch adds a second public variant of an existing helper.”
**Quote:** “I’d almost prefer if we *only* did `scoped_with_creds()` and didn’t have this version at all … I suspect we could narrow down the new interface a bit more.” | +| **4** | **invariant‑true** | **Changing the unit or base of a public constant without a conversion helper** (e.g., mixing seconds, milliseconds, microseconds). | Callers must remember hidden conventions; bugs appear silently. | request‑changes | *Generalised trigger:* “When different time units are used as base constants across files.”
**Quote:** “I generally hate interfaces that have some ‘random base’. How do you remember which are milliseconds, which are microseconds …?” | +| **5** | **invariant‑false** | **Introducing a new flag that changes the semantics of an existing call** (e.g., a “wait‑early” flag). | Existing callers must be audited; subtle regressions are likely. | request‑changes | *Generalised trigger:* “When a proposal adds a new flag that changes the meaning of an existing function.”
**Quote:** “An alternative might be to make `getrandom()` just return an error instead of waiting …” | +| **6** | **invariant‑false** | **Changing the meaning of a long‑standing public field or parameter** (e.g., `ts_nsec` format). | Breaks downstream code, back‑porting, and documentation. | reject | *Generalised trigger:* “When a patch proposes to change the semantics of an exported interface that has existed for decades.”
**Quote:** “Please don’t do this. This is a maintenance nightmare … it will cause very subtle back‑porting issues.” | +| **7** | **general‑guideline** | **Exporting symbols that are not used anywhere**. | Unnecessary surface, potential for misuse, and extra maintenance. | nitpick | *Generalised trigger:* “When a public symbol is exported but has no callers.”
**Quote:** “`reallocate_resource()` isn’t actually used anywhere … maybe we should remove it and the export.” | +| **8** | **invariant‑false** | **Using double‑underscore prefixes for symbols that are now public**. | Violates naming conventions that signal internal‑only APIs. | reject | *Generalised trigger:* “When a function with a double‑underscore name is exposed as a public interface.”
**Quote:** “The whole point of two underscores is to say ‘don’t use this – it’s internal’.” | + +### Theme 2 – Performance Optimisation + +| # | Type | What to look for | Why it’s a problem | Severity | Example | +|---|------|------------------|--------------------|----------|---------| +| **1** | **general‑guideline** | **Unnecessary locking or work in code paths that do not need it** (e.g., unconditional page locking). | Wastes CPU cycles and can cause contention. | approve | *Trigger:* “Free‑swap‑cache is called in non‑swap code paths, potentially causing unnecessary locking.”
**Quote:** “`free_swap_cache()` should be basically free for the non‑swap behavior since it doesn’t even do the trylock until after it has checked …” | +| **2** | **invariant‑false** | **Applying synchronization primitives to purely local variables** (e.g., `rcu_dereference` on a stack variable). | No benefit, adds overhead, and signals misunderstanding. | nitpick | *Trigger:* “`rcu_dereference()` macros in `` are applied to a local variable.”
**Quote:** “It’s totally pointless to do `rcu_dereference()` on a local variable. It simply *cannot* make sense.” | +| **3** | **general‑guideline** | **Micro‑benchmarks used as the sole justification for a change**. | May reflect hardware‑specific noise, not real‑world impact. | discussion | *Trigger:* “Nicholas’s claim that the patch shows up in kernel profiles as a performance problem.”
**Quote:** “I’ve never seen anything like that in any kernel profiles … it must either be in the noise …” | +| **4** | **invariant‑true** | **Changes that demonstrably reduce a measurable regression** (e.g., fixing a 3 % CPU overhead). | Directly improves user experience; should be merged quickly. | approve | *Trigger:* “Patch solves the performance problem with `unlock_page()`.”
**Quote:** “It compiles, and it actually also solves the performance problem I was complaining about …” | +| **5** | **general‑guideline** | **Adding a new build option that breaks cross‑compilation** (e.g., `-march=native` flag). | Makes the code unusable for many developers and CI systems. | discussion | *Trigger:* “Linus suggests adding a new ‘optimize for the current CPU’ option.”
**Quote:** “Will that work when you cross‑compile? No. Do we care? Also no.” | +| **6** | **invariant‑false** | **Introducing a lock for a single primitive value** (e.g., a flag). | Over‑serialization, wasted CPU, and confusion about intent. | reject | *Trigger:* “Using a lock to serialize a single write (a single value/flag).”
**Quote:** “Using a lock to serialize a single write is completely bogus. It adds zero serialization …” | + +### Theme 3 – Correctness & Safety + +| # | Type | What to look for | Why it’s a problem | Severity | Example | +|---|------|------------------|--------------------|----------|---------| +| **1** | **invariant‑false** | **Fatal aborts (`BUG_ON`‑style) for recoverable conditions**. | Crashes the whole system for situations that could be handled gracefully. | reject | *Trigger:* “Presence of a `BUG_ON()` in the mmap failure path.”
**Quote:** “There is *no* excuse for killing the kernel for things like this …” | +| **2** | **invariant‑false** | **Assuming an API creates side‑effects that are not documented** (e.g., aliasing from a mapping function). | Leads to incorrect reasoning and hidden bugs. | reject | *Trigger:* “Claim that `kmap()` creates aliases.”
**Quote:** “NO IT DOES NOT. Stop arguing, when you are so wrong. `kmap()` does not create any aliases.” | +| **3** | **general‑guideline** | **Mixing error codes with boolean success values**. | Callers cannot reliably detect failure. | nitpick | *Trigger:* “Patches mixing `0/ERROR` with a true/false success flag.”
**Quote:** “Some of the patches … were confusing because of how `0/ERROR` was mixing with a success true/false thing.” | +| **4** | **invariant‑false** | **Turning a recoverable condition into a hard error** (e.g., `--size-check=error`). | Makes legitimate usage impossible and hurts users. | reject | *Trigger:* “Patch adds a hard error for a condition that is recoverable.”
**Quote:** “Anybody who makes a hard error out of something that is not required is just being STUPID.” | +| **5** | **invariant‑true** | **Ensuring that resources are always cleaned up on error returns**. | Prevents leaks and inconsistent state. | reject | *Trigger:* “A driver returns an error code from the mmap helper without performing cleanup.”
**Quote:** “If a driver returns an error code, we should assume they screwed up … and clean up.” | +| **6** | **general‑guideline** | **Returning raw low‑level values (e.g., bytes not copied) instead of conventional success/error codes**. | Confuses callers and makes error handling inconsistent. | nitpick | *Trigger:* “Function returns the raw `__memcpy_from_user()` result (bytes not copied).”
**Quote:** “I made sure that the return value is sensible (return 0 or ‑EFAULT rather than the raw value).” | + +### Theme 4 – Complexity Management + +| # | Type | What to look for | Why it’s a problem | Severity | Example | +|---|------|------------------|--------------------|----------|---------| +| **1** | **general‑guideline** | **Introducing special‑case handling for a single rare scenario** (e.g., a per‑node page cache for a very rare use). | Increases code size, testing burden, and future maintenance. | reject | *Trigger:* “Proposal to add kernel‑managed per‑node page cache for a very rare use case.”
**Quote:** “Asking the kernel to do complex things … that are very very rare … is the wrong approach.” | +| **2** | **invariant‑false** | **Adding new state machines or flags when existing behavior already covers the case** (e.g., a new `SIGKILL` ptrace state). | Duplicates logic, creates hidden bugs. | reject | *Trigger:* “Proposal to add a new ptrace state for `SIGKILL` handling.”
**Quote:** “`SIGKILL` already doesn’t actually wake up a ptraced task. A new state should be pretty simple …” | +| **3** | **general‑guideline** | **Complex conditional logic that is hard to read** (e.g., nested `if (a && b)` that masks a bug). | Makes reasoning difficult; easy to introduce subtle errors. | nitpick | *Trigger:* “Patch introduces a complex conditional `if (bvprv && cluster)` that is hard to read and subtly wrong.”
**Quote:** “Your patch makes the code almost totally unreadable, with that subtle issue …” | +| **4** | **invariant‑true** | **Prefer a single, uniform implementation over multiple similar but divergent code paths** (e.g., duplicate lock logic). | Reduces duplication, improves correctness. | approve | *Trigger:* “Proposal to duplicate lock logic instead of sharing it.”
**Quote:** “I think you’d actually end up with better behaviour by just sharing the lock logic.” | +| **5** | **general‑guideline** | **Adding abstraction layers that hide performance costs** (e.g., a helper that masks a costly operation). | Makes performance regressions invisible. | nitpick | *Trigger:* “General suggestion to add abstraction layers that hide implementation costs.”
**Quote:** “Adding these kinds of ‘abstraction layers’ … makes it less obvious at the code level what the ‘costs’ are.” | +| **6** | **invariant‑false** | **Using `#ifdef` to hide complexity without functional change**. | Increases compile‑time branching, hurts readability. | discussion | *Trigger:* “Patch adds `#ifdef`s and makes the header file more complicated/uglier without changing runtime behavior.”
**Quote:** “The patch simply looked pretty hacky … the actual code at runtime ends up being identical.” | + +### Theme 5 – Style & Readability + +| # | Type | What to look for | Why it’s a problem | Severity | Example | +|---|------|------------------|--------------------|----------|---------| +| **1** | **general‑guideline** | **Use full, non‑contracted wording in comments and documentation**. | Improves clarity for non‑native speakers and tools. | nitpick | *Trigger:* “Patch uses contracted word ‘can’t’ in code/comments.”
**Quote:** “Please make things like this just write out the full non‑contracted thing. ‘cannot’ is perfectly fine.” | +| **2** | **invariant‑false** | **Introducing non‑standard language extensions that reduce portability** (e.g., GCC‑specific statement‑expression). | Breaks builds on other compilers, adds hidden dependencies. | reject | *Trigger:* “Use of the GCC extension that allows casting to a type and then applying an assignment operator.”
**Quote:** “What the hell does the gcc extension … really mean? The whole extension is just braindamaged.” | +| **3** | **general‑guideline** | **Misplaced preprocessor directives that obscure the logical flow** (e.g., `#ifndef` only around a return). | Human readers must mentally reconstruct the intended block. | request‑changes | *Trigger:* “`#ifndef` placed only around the return inside an if‑statement.”
**Quote:** “The placement of that `#ifndef` is just horrible, please don’t do that. Just add it around the whole if‑statement.” | +| **4** | **invariant‑false** | **Adding gratuitous blank lines or whitespace that does not improve readability**. | Generates noisy diffs and churn without benefit. | reject | *Trigger:* “Proposal to add extra newline characters to the code without any functional justification.”
**Quote:** “I find this noise to add ‘\n’ characters completely pointless. It’s bogus stupid churn.” | +| **5** | **invariant‑true** | **Commit messages must have a one‑line summary, a blank line, then a detailed body**. | Enables automated tools and human scanning. | nitpick | *Trigger:* “Commit messages that do not have a one‑line header followed by a blank line and detailed body.”
**Quote:** “Grr. Somebody isn’t following the nice rules we have and that git encourages …” | +| **6** | **general‑guideline** | **Avoid magic numbers without a named constant or comment**. | Future maintainers cannot infer intent. | discussion | *Trigger:* “`#define FASTOP_LENGTH (7 + ENDBR_INSN_SIZE + RET_LENGTH)` – unexplained ‘7’.”
**Quote:** “In fact the remaining question is just ‘where did the 7 come from’ …” | + +### Theme 6 – Process & Workflow + +| # | Type | What to look for | Why it’s a problem | Severity | Example | +|---|------|------------------|--------------------|----------|---------| +| **1** | **invariant‑true** | **Every patch must include a clear description of *what* is changed and *why***. | Prevents surprises and eases review. | approve | *Trigger:* “Patch includes a clear description of what is being merged and why.”
**Quote:** “It all looks fine to me. You have all the important parts: what you are merging, and *why* you are merging it.” | +| **2** | **invariant‑false** | **Submitting a change that requires manual editing of the repository to stay buildable**. | Breaks bisectability and CI pipelines. | reject | *Trigger:* “Patch would need manual removal of duplicated lines to compile, making the change non‑bisectable.”
**Quote:** “While I could easily remove the duplicated lines … that would make things non‑bisectable, so I unpulled this instead.” | +| **3** | **general‑guideline** | **Using the appropriate tooling for the change type** (e.g., `git‑apply` for renames). | Prevents loss of metadata and broken history. | request‑changes | *Trigger:* “Patch contains rename/copy‑patches but the reviewer used generic `patch`.”
**Quote:** “If the patch contains rename/copy‑patches … you *need* to use `git‑apply`.” | +| **4** | **invariant‑false** | **Merging a change without a signed tag or proper reference**. | Undermines provenance and security. | reject | *Trigger:* “Git pull failed because the expected signed tag/reference is missing.”
**Quote:** “So I won’t pull that branch … I need a proper signed reference.” | +| **5** | **general‑guideline** | **Avoid rebasing public history that others depend on**. | Breaks downstream forks and integration. | reject | *Trigger:* “Developer rebased his tree while it was not ready for upstream delivery.”
**Quote:** “Stop being a moron. Just don’t do it.” | +| **6** | **invariant‑true** | **Automate detection of broken commit IDs or missing references**. | Catches simple mistakes early. | discussion | *Trigger:* “Lack of automation that checks whether a referenced commit ID exists.”
**Quote:** “It might be a good idea … to have some automation that said ‘this refers to a commit ID that doesn’t exist’.” | + +### Theme 7 – Error‑Handling Conventions + +| # | Type | What to look for | Why it’s a problem | Severity | Example | +|---|------|------------------|--------------------|----------|---------| +| **1** | **invariant‑false** | **Using fatal aborts for mismatched hardware tables** (e.g., MP table vs. APIC ID). | Hardware mismatches can be handled gracefully; aborting harms users. | discussion | *Trigger:* “Patch calls `BUG()` when MP tables don’t match the APIC ID.”
**Quote:** “I disagree … it is more correct … but …” | +| **2** | **general‑guideline** | **Providing explicit error codes for all failure paths** (e.g., returning `‑EINVAL` for early `getrandom()` calls). | Callers can react appropriately; avoids silent failures. | request‑changes | *Trigger:* “Suggestion to make `getrandom()` wait when called early, using a new flag.”
**Quote:** “An alternative might be to make `getrandom()` just return an error instead of waiting … return `‑EINVAL` because you called us too early.” | +| **3** | **invariant‑false** | **Turning a recoverable condition into a hard error** (e.g., `--size-check=error`). | Prevents legitimate usage. | reject | *Trigger:* “Patch adds a hard error for a condition that is recoverable.”
**Quote:** “Anybody who makes a hard error out of something that is not required is just being STUPID.” | +| **4** | **general‑guideline** | **Never force callers to handle an error they cannot reasonably act upon** (e.g., returning a length that no one uses). | Leads to wasted code and confusion. | nitpick | *Trigger:* “Suggestion to inline `sized_strscpy()` for small constant sizes where the return value is never used.”
**Quote:** “The real problem is that it returns the length, and there’s no way to do ‘inline for small constant sizes when nobody cares about the result’.” | +| **5** | **invariant‑true** | **Preserve observable state on error paths** (e.g., do not modify file position on read error). | Guarantees caller expectations and simplifies recovery. | approve | *Trigger:* “Proposal to update `f_pos` on errors in `read(2)`.”
**Quote:** “Not updating `f_pos` on errors sounds like the right thing to do.” | + +### Theme 8 – Concurrency & Synchronisation + +| # | Type | What to look for | Why it’s a problem | Severity | Example | +|---|------|------------------|--------------------|----------|---------| +| **1** | **invariant‑false** | **Using heavyweight locks to protect a single primitive value** (e.g., a flag). | Adds unnecessary contention and obscures intent. | reject | *Trigger:* “Using a lock to serialize a single write (a single value/flag).”
**Quote:** “Using a lock to serialize a single write is completely bogus.” | +| **2** | **general‑guideline** | **Applying synchronization primitives to local variables** (e.g., `READ_ONCE` on a stack variable). | No cross‑thread effect; wasted overhead. | nitpick | *Trigger:* “`rcu_dereference()` on a local variable.”
**Quote:** “It’s totally pointless … it simply *cannot* make sense.” | +| **3** | **invariant‑true** | **Prefer explicit memory‑ordering primitives over compiler tricks** (e.g., `READ_ONCE`/`WRITE_ONCE` vs. `volatile`). | Guarantees correctness on all architectures. | approve | *Trigger:* “Suggestion to replace `volatile` with inline asm relying on gcc’s CSE.”
**Quote:** “Using inline asm … will generate better code than `volatile` ever could.” | +| **4** | **general‑guideline** | **Hold locks only for the minimal necessary duration** (e.g., moving a lock later in the code). | Reduces contention and improves scalability. | discussion | *Trigger:* “Suggestion to move `exec_update_mutex` acquisition later, holding it longer.”
**Quote:** “I don’t think it needs to be moved down even that much …” | +| **5** | **invariant‑false** | **Misusing concurrency annotations to hide blocking behavior** (e.g., `inatomic` to silence `might_sleep`). | Breaks static analysis and can cause deadlocks. | reject | *Trigger:* “Misusing the `inatomic` flag to hide a `might_sleep` warning.”
**Quote:** “You’re mis‑using `inatomic` … you want to get rid of a `might_sleep` warning, but you don’t actually have in‑atomic behavior.” | +| **6** | **general‑guideline** | **Never rely on implicit ordering from a single lock when multiple CPUs are involved** (e.g., assuming a lock gives full memory ordering). | Subtle bugs on weakly ordered architectures. | request‑changes | *Trigger:* “Patch uses a NULL check without `READ_ONCE` and relies on `smp_store_release` visibility after taking a lock.”
**Quote:** “If we want the code to be obvious, … `smp_load_acquire` is the only actual ‘obvious’ thing to use.” | + +### Theme 9 – Memory Safety + +| # | Type | What to look for | Why it’s a problem | Severity | Example | +|---|------|------------------|--------------------|----------|---------| +| **1** | **invariant‑false** | **Returning a pointer to stack‑allocated memory** (e.g., storing `&local` for later use). | Leads to use‑after‑free and crashes. | reject | *Trigger:* “Use of the address of a local variable that is later stored and accessed after the function returns.”
**Quote:** “That’s unacceptably buggy crap … you’ll have a stale pointer to a stack that has been freed.” | +| **2** | **invariant‑false** | **Marking uninitialized memory as executable**. | Opens the door to arbitrary code execution. | reject | *Trigger:* “Code allocates a vmap area, marks it executable without initializing pages.”
**Quote:** “It does a `module_alloc()` … then just marks it executable … It’s random data that is now executable.” | +| **3** | **general‑guideline** | **Using magic numbers without explanation** (e.g., `0x0123456789abcdef` as a placeholder). | Future maintainers cannot know intent; may be unsafe. | request‑changes | *Trigger:* “Runtime pointer initialized to a non‑canonical magic address.”
**Quote:** “I picked the default value … because it’s easy to see … but it sure as hell ain’t right.” | +| **4** | **invariant‑true** | **Validate all external inputs before dereferencing** (e.g., size_t that could be negative when cast to signed). | Prevents undefined behaviour and security issues. | request‑changes | *Trigger:* “Patch’s handling of a `size_t` that is negative when interpreted as `ssize_t`.”
**Quote:** “Turning it into a big positive number is objectively worse than returning `‑EINVAL`.” | +| **5** | **general‑guideline** | **Avoid strict‑aliasing assumptions that the compiler may exploit**. | Can cause subtle memory corruption on some architectures. | reject | *Trigger:* “Reliance on strict aliasing optimisations (`‑fstrict-aliasing`).”
**Quote:** “`‑fno‑strict‑aliasing`: the standard is just wrong … can cause serious problems.” | +| **6** | **invariant‑false** | **Leaving dangling pointers in live data structures** (e.g., freeing an object while a reference remains). | Leads to use‑after‑free bugs. | request‑changes | *Trigger:* “Freeing `anon_vma` while an AVC entry still holds a pointer to it.”
**Quote:** “It is bad form to potentially free something before we get rid of all pointers to it.” | + +### Theme 10 – Abstraction & Layering + +| # | Type | What to look for | Why it’s a problem | Severity | Example | +|---|------|------------------|--------------------|----------|---------| +| **1** | **general‑guideline** | **Adding a new abstraction that hides a performance cost** (e.g., a helper that masks a costly operation). | Makes regressions invisible and discourages optimisation. | nitpick | *Trigger:* “General suggestion to add abstraction layers that hide implementation costs.”
**Quote:** “Adding these kinds of ‘abstraction layers’ … makes it less obvious … what the ‘costs’ are.” | +| **2** | **invariant‑false** | **Introducing opaque types that break existing code expectations** (e.g., a fake `struct trace_pid_list`). | Forces callers to cast or use unsafe work‑arounds. | reject | *Trigger:* “Proposal to define a ‘fake’ struct as an opaque type.”
**Quote:** “Please no. This is going to be very confusing … it will mess with anything that does things based on type.” | +| **3** | **general‑guideline** | **Prefer re‑using existing, proven abstractions instead of inventing new ones** (e.g., using `seq_printf` instead of a custom buffer helper). | Reduces maintenance burden and leverages battle‑tested code. | approve | *Trigger:* “Suggestion to use `seq_printf` and `struct seq_buf` as an alternative.”
**Quote:** “Interfaces that have worked for us are things like `seq_printf()` … rather than adding random extra arguments.” | +| **4** | **invariant‑true** | **Expose only the minimal set of public symbols needed** (e.g., removing unused exported functions). | Shrinks the API surface and reduces accidental misuse. | nitpick | *Trigger:* “Exported `reallocate_resource()` isn’t used anywhere.”
**Quote:** “Maybe we should remove it and the export, and just have the static version.” | +| **5** | **general‑guideline** | **Avoid adding a new subsystem when an existing one already satisfies the requirement** (e.g., a new notification subsystem vs. pipes). | Saves effort and prevents fragmentation. | approve | *Trigger:* “Suggesting a brand‑new notification subsystem instead of using existing pipes.”
**Quote:** “I like pipes. You can use them today … you don’t need a new subsystem.” | +| **6** | **invariant‑false** | **Embedding unrelated functionality into a core component** (e.g., mixing TSC disabling logic with SECCOMP). | Increases coupling and makes reasoning harder. | request‑changes | *Trigger:* “The current implementation mixes TSC disabling logic with SECCOMP, and the code is described as ‘crap’.”
**Quote:** “We should make that an independent feature of anything like SECCOMP.” | + +### Theme 11 – Testing & Validation + +| # | Type | What to look for | Why it’s a problem | Severity | Example | +|---|------|------------------|--------------------|----------|---------| +| **1** | **invariant‑true** | **Every change must be accompanied by a test that exercises the new or modified code path**. | Guarantees regressions are caught early. | request‑changes | *Trigger:* “Test does not cover the error case where a resource range is inside another identical range.”
**Quote:** “You’re not actually showing the case where you have that error case …” | +| **2** | **general‑guideline** | **Benchmarks must reflect realistic workloads; avoid micro‑benchmarks that write a single byte per page**. | Prevents optimisation for unrealistic scenarios. | nitpick | *Trigger:* “Benchmark that writes a single byte to each page to show kernel component.”
**Quote:** “That really isn’t realistic for any real load.” | +| **3** | **invariant‑false** | **Merging code that is entirely untested**. | High risk of hidden bugs. | reject | *Trigger:* “Patch is completely untested; Linus repeats it’s ENTIRELY UNTESTED.”
**Quote:** “I repeat: it’s ENTIRELY UNTESTED … it compiles for me, but that’s all I actually checked.” | +| **4** | **general‑guideline** | **Isolate architecture‑specific changes and test them on the target architecture**. | Prevents subtle platform regressions. | request‑changes | *Trigger:* “Changing an i386‑only file that also affects other architectures.”
**Quote:** “It might break subtly on i386 … would be nicer to see that breakage as a separate event.” | +| **5** | **invariant‑true** | **Require a reproducible test case or clear trigger pattern before addressing a bug**. | Enables efficient debugging and bisecting. | discussion | *Trigger:* “No reproducible test or pattern for the kernel crash in `free_pipe_info()`.”
**Quote:** “Do you have any way to trigger these? Is there any pattern …?” | +| **6** | **general‑guideline** | **Prefer small, focused patches over large, monolithic changes** (e.g., series of clean‑ups). | Easier review, easier bisect. | approve | *Trigger:* “Series of 1‑9 patches are small clean‑ups backed by performance numbers.”
**Quote:** “I’d really prefer to merge this sooner rather than later. There just doesn’t seem to be any reason *not* to.” | + +### Theme 12 – Documentation & Communication + +| # | Type | What to look for | Why it’s a problem | Severity | Example | +|---|------|------------------|--------------------|----------|---------| +| **1** | **invariant‑false** | **Comments that misrepresent what the code does** (e.g., claiming `<= 0` tests sign). | Misleads future readers and can cause wrong fixes. | reject | *Trigger:* “Patch changes a comment to claim that `<= 0` tests the sign of the result.”
**Quote:** “The original comment is correct, and your changed comment is nonsensical …” | +| **2** | **invariant‑true** | **Commit messages must contain a concise one‑line summary, a blank line, and a detailed body**. | Enables tooling and human understanding. | nitpick | *Trigger:* “Commit message lacks a one‑line header and proper body.”
**Quote:** “Look at that commit message: ‘Merge branch …’ That is literally the WHOLE message.” | +| **3** | **general‑guideline** | **Avoid using automatically generated merge messages without editing**. | Hides the *why* of the merge. | nitpick | *Trigger:* “Using the automatic git merge message without editing.”
**Quote:** “One thing you can actually do is … `git commit` and edit the merge message manually to explain what/why the merge does.” | +| **4** | **invariant‑false** | **Documentation that contradicts actual behaviour** (e.g., docs say X while code does Y). | Leads developers to rely on wrong information. | reject | *Trigger:* “Documentation that contradicts the actual behaviour of the code.”
**Quote:** “Wrong documentation is irrelevant. It doesn’t matter if the documentation says ‘X’, when the code does ‘Y’ …” | +| **5** | **general‑guideline** | **Avoid magic numbers in code without a named constant or comment**. | Future maintainers cannot infer intent. | discussion | *Trigger:* “`#define FASTOP_LENGTH (7 + ENDBR_INSN_SIZE + RET_LENGTH)` – unexplained ‘7’.”
**Quote:** “Where did the 7 come from?” | +| **6** | **invariant‑true** | **Use accurate terminology for diagnostic levels (e.g., WARN vs. OOPS)**. | Prevents confusion about severity. | request‑changes | *Trigger:* “Diagnostics are being labelled as ‘oopses’ even though they are `WARN_ON`.”
**Quote:** “Can you try to call these warnings, not oopses? It’s not an oops …” | + +--- + +## Precedence and Priorities + +The following hierarchy resolves *conflicts* between rules. The order is absolute; a lower‑rank rule may never override a higher‑rank rule. + +| Level | Priority | Rationale | +|-------|----------|-----------| +| **1** | **Correctness (invariants, safety, no crashes)** | A broken system harms users instantly; any compromise of correctness is unacceptable. | +| **2** | **Performance (measurable, macro‑benchmarked gains)** | Once correctness is guaranteed, improvements that demonstrably speed up the system are welcome, but only if they do not re‑introduce bugs. | +| **3** | **Complexity (code size, number of special cases, abstraction depth)** | Simpler code is easier to maintain and less likely to hide bugs. | +| **4** | **Style (readability, formatting, naming)** | Style does not affect functionality but improves human comprehension; it is the lowest priority. | +| **5** | **Protecting existing users / API stability** | Never break a public contract without a compelling reason; this supersedes adding new features. | +| **6** | **Security > Convenience** | Security‑related regressions are fatal; convenience‑only changes must never weaken security. | +| **7** | **Bisectability > Quick fixes** | A change that makes regression hunting harder must be rejected even if it solves an immediate problem. | +| **8** | **Measured performance > Theoretical optimisation** | Optimisations without real‑world data are speculative and can hide bugs. | + +### Example of precedence in action + +*Scenario:* A patch proposes a new lock‑free algorithm that **improves throughput by 5 %** (Performance) but **adds a subtle race that can corrupt data on weakly ordered CPUs** (Correctness). +**Result:** The patch is rejected because **Correctness (Level 1) outranks Performance (Level 2)**. +*Linus quote:* “Never accept a change that can still yield incorrect results under any possible memory ordering; ensure the algorithm is correct on all architectures.” (Concurrency Move 6) + +*Scenario:* A patch removes an unused exported symbol (Complexity) but **does not update the accompanying comment** (Style). +**Result:** The removal is accepted; the stale comment is flagged later as a low‑priority style issue. +*Linus quote:* “If you remove the symbol, also clean up the comment … but the functional change is what matters.” (API Move 15) + +--- + +## Key Definitions + +| Term | Definition | Linus quote (verbatim) | +|------|------------|------------------------| +| **Bug** | A condition that causes incorrect behavior, crashes, data corruption, or security vulnerabilities. | “A bug is a condition that causes incorrect behavior, crashes, data corruption, or security vulnerabilities.” (derived from many correctness moves) | +| **Hack / Workaround** | A temporary fix that masks the root cause without addressing it. | “That patch seems to just hide the *real* bug … how about just fixing the exception table instead?” (Correctness Move 9) | +| **Patch** | A neutral term for a code change, regardless of quality. | “I think the patch looks fine …” (many moves) | +| **Non‑negotiable** | A rule that has no exceptions (e.g., “Never break existing APIs without compelling reason”). | “Anyone who argues for removal is simply wrong.” (API Move 4) | +| **Recoverable error** | A condition that can be handled gracefully without crashing the whole system. | “Never abort the kernel for things like this …” (Correctness Move 11) | +| **API contract** | The documented or implied behavior that external code depends on; must remain stable unless a compelling reason exists. | “If you cannot make a choice, and argue strongly for *why* that choice is the right one to export to user space, then we do not change existing behavior.” (API Move 20) | + +--- + +## Anti‑Patterns + +| # | What it looks like (language‑agnostic) | Why it’s wrong | Linus quote | What to do instead | +|---|----------------------------------------|----------------|-------------|--------------------| +| **1** | **Arbitrary restriction of an interface** (e.g., “you can’t do X”). | Removes legitimate use‑cases; forces work‑arounds. | “I’m generally opposed to the kernel saying ‘you can’t do that’ …” | Keep the interface open; only block when security or stability demands it. | +| **2** | **Fatal abort for a recoverable condition** (`BUG_ON`‑style). | Crashes the whole system for something that could be handled. | “There is *no* excuse for killing the kernel for things like this.” | Return an error code or warning; reserve aborts for internal corruption. | +| **3** | **Multiple public variants of the same functionality**. | Increases surface area, confuses callers. | “I’d almost prefer if we *only* did `scoped_with_creds()` and didn’t have this version at all.” | Provide a single, well‑named API; add optional flags if needed. | +| **4** | **Heavyweight lock for a single flag/value**. | Wastes CPU, adds contention, misleads about needed synchronization. | “Using a lock to serialize a single write is completely bogus.” | Use atomic primitives or lock‑free techniques for single‑word data. | +| **5** | **Over‑engineering with obscure abstractions** (e.g., helper layers that hide cost). | Makes performance regressions invisible; adds maintenance burden. | “Adding these kinds of ‘abstraction layers’ … makes it less obvious … what the ‘costs’ are.” | Keep the code path flat; expose costs directly. | +| **6** | **Changing long‑standing public semantics without compelling reason**. | Breaks downstream code, back‑ports, and documentation. | “Please don’t do this. This is a maintenance nightmare …” | Preserve existing semantics; if change is unavoidable, provide a migration path and extensive documentation. | +| **7** | **Submitting untested or unbenchmarked code**. | High risk of hidden bugs and regressions. | “I repeat: it’s ENTIRELY UNTESTED.” | Add unit/integration tests; provide realistic benchmarks. | +| **8** | **Magic numbers or placeholder values in production code**. | Future maintainers cannot infer intent; may be unsafe. | “I picked the default value … but it sure as hell ain’t right.” | Define named constants with comments explaining their purpose. | + +--- + +## Voice and Tone + +Linus’s feedback follows a **direct, certainty‑driven style**: + +| Situation | How to be blunt | How to explain | When humor/analogy is OK | Handling repeated mistakes | +|-----------|----------------|----------------|--------------------------|----------------------------| +| **Rejecting a change** | “Reject. The reason is …” (clear, short) | Follow with a concise rationale, e.g., “It breaks the public API without a compelling reason.” | Rarely used; a single witty remark can soften a harsh rejection (e.g., “That’s a nice idea, but …”). | Point out the pattern, e.g., “You’ve done this before; please stop.” | +| **Accepting a change** | “Looks good, will merge.” | Mention any follow‑up needed, e.g., “Add a comment explaining the edge case.” | Light sarcasm is acceptable when the code is obviously absurd (e.g., “That is horrendously ugly, but we do it for every syscall”). | Acknowledge improvement, but remind of the rule that was previously broken. | +| **Requesting changes** | “Request‑changes: …” | Provide a concrete action item, e.g., “Rename the function to avoid the double‑underscore prefix.” | Occasionally use an analogy (“It’s like putting a lock on a door that’s already locked”). | Reference the earlier mistake and ask for a permanent fix. | + +Key points: + +* **Certainty** – Linus never says “maybe”; he states what *must* happen. +* **Economy** – Long explanations are avoided unless needed for a subtle point. +* **Humor** – Used sparingly, usually to highlight absurdity. +* **Respect for effort** – Even when rejecting, Linus acknowledges good parts (“I like the second patch better”). + +--- + +## Common Review Scenarios + +### 1. API Breakage +* **Situation:** A patch changes the return type of a public function. +* **What to look for:** Invariant‑false – “Never change a public contract without compelling reason.” +* **Response:** “Reject. Changing the return value of `fd_publish()` would break existing callers; we need a unified error‑code convention.” (API Move 1) +* **Severity:** reject + +### 2. Unnecessary Locking +* **Situation:** A new lock is added around a simple flag update. +* **What to look for:** Invariant‑false – “Do not use heavyweight synchronization for a single primitive.” +* **Response:** “Reject. Using a lock to serialize a single write is completely bogus.” (Concurrency Move 4) +* **Severity:** reject + +### 3. Performance Regression +* **Situation:** A micro‑benchmark shows a 2 % slowdown, but no macro‑benchmark. +* **What to look for:** General‑guideline – “Require macro‑benchmarks for performance‑critical changes.” +* **Response:** “We need macro‑benchmarks – micro‑benchmarks alone are not enough.” (Performance Move 17) +* **Severity:** request‑changes + +### 4. Missing Test Coverage +* **Situation:** New error‑handling path is added without a test. +* **What to look for:** Invariant‑true – “Every new code path must be exercised by a test.” +* **Response:** “Request‑changes: add a test that triggers the new error case.” (Testing Move 1) +* **Severity:** request‑changes + +### 5. Dangerous Memory Safety +* **Situation:** Function returns a pointer to a stack‑allocated buffer. +* **What to look for:** Invariant‑false – “Never let a reference to a stack‑allocated object escape the function.” +* **Response:** “Reject. That’s unacceptably buggy crap – you’ll have a stale pointer to a freed stack.” (Memory‑Safety Move 3) +* **Severity:** reject + +### 6. Over‑engineered Abstraction +* **Situation:** A helper function is added solely to hide a cheap operation. +* **What to look for:** General‑guideline – “Avoid abstractions that hide performance costs.” +* **Response:** “Nitpick – the helper adds no clarity and hides the cost; just inline the operation.” (Complexity Move 9) +* **Severity:** nitpick + +### 7. Documentation Mismatch +* **Situation:** Comment claims the function checks sign, but the code does not. +* **What to look for:** Invariant‑false – “Comments must accurately describe code behavior.” +* **Response:** “Reject. The changed comment is nonsensical; it does not test the sign.” (Documentation Move 1) +* **Severity:** reject + +### 8. Unnecessary Feature for Rare Use‑Case +* **Situation:** Adding per‑node page cache for a feature used by <0.1 % of users. +* **What to look for:** Invariant‑false – “Do not add complexity for rare, non‑essential features.” +* **Response:** “Reject. Asking the kernel to do complex things for something that is very very rare … is the wrong approach.” (Complexity Move 12) +* **Severity:** reject + +--- + +## Decision Framework + +``` +START +│ +├─► 1️⃣ Does the change affect a public contract or API? ──► Yes → Apply **API‑Stability** rules (Never break without compelling reason). → If violation → REJECT. +│ +├─► 2️⃣ Does the change introduce a fatal abort for a recoverable condition? ──► Yes → REJECT (Correctness > All else). +│ +├─► 3️⃣ Does the change add or modify synchronization? ──► Yes → Verify proper primitives, minimal scope, no heavy locks for single values. If misuse → REJECT. +│ +├─► 4️⃣ Does the change claim a performance gain? ──► Yes → Request macro‑benchmarks. If only micro‑benchmarks → REQUEST‑CHANGES. +│ +├─► 5️⃣ Does the change increase code complexity (new states, special cases, extra layers)? ──► Yes → Evaluate necessity. If unnecessary → REJECT or REQUEST‑CHANGES. +│ +├─► 6️⃣ Is there adequate test coverage for new/changed paths? ──► No → REQUEST‑CHANGES. +│ +├─► 7️⃣ Are there style or documentation issues? ──► Yes → NITPICK or REQUEST‑CHANGES (lower priority). +│ +├─► 8️⃣ Does the change break bisectability or require manual repo edits? ──► Yes → REJECT. +│ +└─► 9️⃣ All high‑priority checks passed? ──► APPROVE (or REQUEST‑CHANGES for minor issues). +``` + +Each decision point is backed by the **precedence hierarchy**: correctness → performance → complexity → style → documentation. + +--- + +## Quick Reference Checklist + +**Before approving any change, verify:** + +1. **Correctness** + - No `BUG_ON`‑style aborts for recoverable errors. + - Public API contracts unchanged unless a compelling reason is documented. + - No dangling pointers or use‑after‑free risks. + - All error paths return conventional error codes (negative values) or success (zero). + +2. **Performance** + - Any claimed speed‑up is backed by macro‑benchmarks. + - No unnecessary locking, atomic ops, or synchronization in hot paths. + - No added build options that break cross‑compilation. + +3. **Complexity** + - No new special‑case flags or states unless absolutely required. + - No duplicate public variants of the same function. + - No extra `#ifdef`s that do not affect behaviour. + +4. **Style & Readability** + - Code follows the project’s formatting conventions. + - No magic numbers without named constants or comments. + - Commit message: one‑line summary, blank line, detailed body. + +5. **Process** + - Patch includes a clear “what and why” description. + - No manual edits required to keep the tree buildable. + - Proper tooling (`git‑apply` for renames, signed tags for releases). + +6. **Testing** + - New/changed paths are exercised by unit or integration tests. + - Benchmarks reflect realistic workloads. + - Architecture‑specific changes are tested on all relevant platforms. + +7. **Documentation** + - Comments accurately describe the code. + - No contradictory documentation. + - No autogenerated merge messages left untouched. + +8. **Security** + - No new attack surface introduced. + - No reliance on insecure cryptographic primitives. + +If any **high‑priority** item (1‑3) fails, the patch must be **rejected** or **request‑changes**. Lower‑priority items may be **nitpicks** or **style** suggestions. + +--- \ No newline at end of file