Re: [PATCH] KVM: x86: Clean up included but non-essential header declarations
From: Like Xu
Date: Tue Oct 17 2023 - 22:23:34 EST
On 18/10/2023 5:31 am, Sean Christopherson wrote:
On Tue, Oct 17, 2023, Like Xu wrote:
From: Like Xu <likexu@xxxxxxxxxxx>
Removing these declarations as part of KVM code refactoring can also (when
the compiler isn't so smart) reduce compile time, compile warnings, and the
Really, warnings? On what W= level? W=1 builds just fine with KVM_WERROR=y.
If any of the "supported" warn levels triggers a warn=>error, then we'll fix it.
size of compiled artefacts, and more importantly can help developers better
consider decoupling when adding/refactoring unmerged code, thus relieving
some of the burden on the code review process.
Can you provide an example? KVM certainly has its share of potential circular
dependency pitfalls, e.g. it's largely why we have the ugly and seemingly
arbitrary split between x86.h and asm/kvm_host.h. But outside of legitimate
collisions like that, I can't think of a single instance where superfluous existing
includes caused problems. On the other hand, I distinctly recall multiple
instances where a header didn't include what it used and broke the build when the
buggy header was included in a new file.
I've noticed that during patch iterations, developers add or forget to
remove header declarations from previous versions (just so the compiler
doesn't complain), and the status quo is that these header declarations
are rapidly ballooning.
Specific header declaration is supposed to be retained if it makes more
sense for reviewers to understand. No functional changes intended.
[*] https://lore.kernel.org/all/YdIfz+LMewetSaEB@xxxxxxxxx/
This patch violates one of the talking points of Ingo's work:
- "Make headers standalone": over 80 headers don't build standalone and
depend on various accidental indirect dependencies they gain through
other headers, especially once headers get their unnecessary dependencies
removed. So there's over 80 commits changing these headers.
I think it's also worth noting that Ingo boosted build times by eliminating
includes in common "high use" headers, not by playing whack-a-mole with "private"
headers and C files.
[**] https://include-what-you-use.org/
Signed-off-by: Like Xu <likexu@xxxxxxxxxxx>
---
arch/x86/kvm/cpuid.c | 3 ---
arch/x86/kvm/cpuid.h | 1 -
arch/x86/kvm/emulate.c | 2 --
arch/x86/kvm/hyperv.c | 3 ---
arch/x86/kvm/i8259.c | 1 -
arch/x86/kvm/ioapic.c | 10 ----------
arch/x86/kvm/irq.c | 3 ---
arch/x86/kvm/irq.h | 3 ---
arch/x86/kvm/irq_comm.c | 2 --
arch/x86/kvm/lapic.c | 8 --------
arch/x86/kvm/mmu.h | 1 -
arch/x86/kvm/mmu/mmu.c | 11 -----------
arch/x86/kvm/mmu/spte.c | 1 -
arch/x86/kvm/mmu/tdp_iter.h | 1 -
arch/x86/kvm/mmu/tdp_mmu.c | 3 ---
arch/x86/kvm/mmu/tdp_mmu.h | 4 ----
arch/x86/kvm/smm.c | 1 -
arch/x86/kvm/smm.h | 3 ---
arch/x86/kvm/svm/avic.c | 2 --
arch/x86/kvm/svm/hyperv.h | 2 --
arch/x86/kvm/svm/nested.c | 2 --
arch/x86/kvm/svm/pmu.c | 4 ----
arch/x86/kvm/svm/sev.c | 7 -------
arch/x86/kvm/svm/svm.c | 29 -----------------------------
arch/x86/kvm/svm/svm.h | 3 ---
arch/x86/kvm/vmx/hyperv.c | 4 ----
arch/x86/kvm/vmx/hyperv.h | 7 -------
arch/x86/kvm/vmx/nested.c | 2 --
arch/x86/kvm/vmx/nested.h | 1 -
arch/x86/kvm/vmx/pmu_intel.c | 1 -
arch/x86/kvm/vmx/posted_intr.c | 1 -
arch/x86/kvm/vmx/sgx.h | 5 -----
arch/x86/kvm/vmx/vmx.c | 11 -----------
arch/x86/kvm/x86.c | 17 -----------------
arch/x86/kvm/xen.c | 1 -
virt/kvm/async_pf.c | 2 --
virt/kvm/binary_stats.c | 1 -
virt/kvm/coalesced_mmio.h | 2 --
virt/kvm/eventfd.c | 2 --
virt/kvm/irqchip.c | 1 -
virt/kvm/kvm_main.c | 13 -------------
virt/kvm/pfncache.c | 1 -
virt/kvm/vfio.c | 2 --
43 files changed, 184 deletions(-)
NAK, I am not taking a wholesale purge of includes. I have no objection to
removing truly unnecessary includes, e.g. there are definitely some includes that
are no longer necessary due to code being moved around. But changes like the
removal of all includes from tdp_mmu.h and smm.h are completely bogus. If anything,
smm.h clearly needs more includes, because it is certainly not including everything
it is using.
Thanks, this patch being nak is to be expected. As you've noticed in the
smm.h story, sensible dependencies should appear in sensible header files,
and are assembled correctly to promote better understanding (the compiler
seems to be happy on weird dependency combinations and doesn't complain
until something goes wrong).
In addition to "x86.h and asm/kvm_host.h", we could have gone further in
the direction of "Make headers standalone", couldn't we ?
diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
index 733a3aef3a96..66afdf3e262a 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.h
+++ b/arch/x86/kvm/mmu/tdp_mmu.h
@@ -3,10 +3,6 @@
#ifndef __KVM_X86_MMU_TDP_MMU_H
#define __KVM_X86_MMU_TDP_MMU_H
-#include <linux/kvm_host.h>
-
-#include "spte.h"
-
void kvm_mmu_init_tdp_mmu(struct kvm *kvm);
void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm);
...
diff --git a/arch/x86/kvm/smm.h b/arch/x86/kvm/smm.h
index a1cf2ac5bd78..3e067ce1ea1d 100644
--- a/arch/x86/kvm/smm.h
+++ b/arch/x86/kvm/smm.h
@@ -2,11 +2,8 @@
#ifndef ASM_KVM_SMM_H
#define ASM_KVM_SMM_H
-#include <linux/build_bug.h>
-
#ifdef CONFIG_KVM_SMM
-
/*
* 32 bit KVM's emulated SMM layout. Based on Intel P6 layout
* (https://www.sandpile.org/x86/smm.htm).