Message-ID: <90364f44-98e7-4a9a-af57-881cae8e81a7@linux.microsoft.com>
Date: Thu, 31 Oct 2024 11:47:30 -0700
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>,
 "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
 "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
 "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
 "virtualization@...ts.linux.dev" <virtualization@...ts.linux.dev>
Cc: "kys@...rosoft.com" <kys@...rosoft.com>,
 "haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
 "wei.liu@...nel.org" <wei.liu@...nel.org>,
 "decui@...rosoft.com" <decui@...rosoft.com>,
 "catalin.marinas@....com" <catalin.marinas@....com>,
 "will@...nel.org" <will@...nel.org>, "luto@...nel.org" <luto@...nel.org>,
 "tglx@...utronix.de" <tglx@...utronix.de>,
 "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de" <bp@...en8.de>,
 "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
 "x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
 "seanjc@...gle.com" <seanjc@...gle.com>,
 "pbonzini@...hat.com" <pbonzini@...hat.com>,
 "peterz@...radead.org" <peterz@...radead.org>,
 "daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>,
 "joro@...tes.org" <joro@...tes.org>,
 "robin.murphy@....com" <robin.murphy@....com>,
 "davem@...emloft.net" <davem@...emloft.net>,
 "edumazet@...gle.com" <edumazet@...gle.com>,
 "kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com"
 <pabeni@...hat.com>, "lpieralisi@...nel.org" <lpieralisi@...nel.org>,
 "kw@...ux.com" <kw@...ux.com>, "robh@...nel.org" <robh@...nel.org>,
 "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "arnd@...db.de"
 <arnd@...db.de>, "sgarzare@...hat.com" <sgarzare@...hat.com>,
 "jinankjain@...ux.microsoft.com" <jinankjain@...ux.microsoft.com>,
 "muminulrussell@...il.com" <muminulrussell@...il.com>,
 "skinsburskii@...ux.microsoft.com" <skinsburskii@...ux.microsoft.com>,
 "mukeshrathor@...rosoft.com" <mukeshrathor@...rosoft.com>
Subject: Re: [PATCH 2/5] hyperv: Remove unnecessary #includes

On 10/10/2024 11:21 AM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@...ux.microsoft.com> Sent: Thursday, October 3, 2024 12:51 PM
>>
>> asm/hyperv-tlfs.h is already included implicitly wherever mshyperv.h
>> or linux/hyperv.h is included. Remove those redundancies.
> 
> I've been under the impression that it is preferable to directly include
> .h files for all definitions that a source code file uses, even if the
> needed .h file is indirectly included by some other .h file. I looked through

I've also heard this idea, and generally try to stick to it. I myself
haven't yet encountered a situation where doing this or not has greatly
impacted the readability of the code.

> the coding style documentation, and didn't find anything addressing
> this topic, so maybe I'm wrong. But I know I've seen patches to other
> parts of the kernel that were changes to follow the direct inclusion
> approach.  Direct inclusion is less fragile if the #include file nesting
> structure changes (and perhaps removes the indirect inclusion).
> 
> The mshyperv.h and linux/hyperv.h dependency on hyperv-tlfs.h is
> highly unlikely to change, so the chance of breakage is minimal. But

You probably already understood this after seeing the later patches, but
it's worth pointing out that the goal of this series is to change that
dependency (situationally), which is exactly why I created this precursor
patch.

> I wonder if your approach is going the wrong direction vs. preferred
> kernel practices.
> 

I could replace <asm/hyperv-tlfs.h> with <hyperv/hvhdk.h> in most of these
cases (in v2), to preserve the explicit include of the definitions before
mshyperv.h or linux/hyperv.h. I will give it a try in v2.

Thanks,
Nuno

> Michael
> 
>>
>> Remove includes of linux/hyperv.h and mshyperv.h where they are not
>> needed.
>>
>> Signed-off-by: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
>> ---
>>  arch/arm64/hyperv/hv_core.c        | 2 --
>>  arch/x86/hyperv/hv_apic.c          | 1 -
>>  arch/x86/hyperv/hv_init.c          | 2 --
>>  arch/x86/hyperv/hv_proc.c          | 1 -
>>  arch/x86/hyperv/ivm.c              | 1 -
>>  arch/x86/hyperv/mmu.c              | 1 -
>>  arch/x86/hyperv/nested.c           | 1 -
>>  arch/x86/include/asm/kvm_host.h    | 1 -
>>  arch/x86/include/asm/mshyperv.h    | 1 -
>>  arch/x86/kernel/cpu/mshyperv.c     | 1 -
>>  arch/x86/kvm/vmx/vmx_onhyperv.h    | 1 -
>>  arch/x86/mm/pat/set_memory.c       | 2 --
>>  drivers/clocksource/hyperv_timer.c | 1 -
>>  drivers/hv/hv_balloon.c            | 2 --
>>  drivers/hv/hv_common.c             | 1 -
>>  drivers/hv/hv_kvp.c                | 1 -
>>  drivers/hv/hv_snapshot.c           | 1 -
>>  drivers/hv/hyperv_vmbus.h          | 1 -
>>  net/vmw_vsock/hyperv_transport.c   | 1 -
>>  19 files changed, 23 deletions(-)
>>
>> diff --git a/arch/arm64/hyperv/hv_core.c b/arch/arm64/hyperv/hv_core.c
>> index f1ebc025e1df..9d1969b875e9 100644
>> --- a/arch/arm64/hyperv/hv_core.c
>> +++ b/arch/arm64/hyperv/hv_core.c
>> @@ -11,11 +11,9 @@
>>  #include <linux/types.h>
>>  #include <linux/export.h>
>>  #include <linux/mm.h>
>> -#include <linux/hyperv.h>
>>  #include <linux/arm-smccc.h>
>>  #include <linux/module.h>
>>  #include <asm-generic/bug.h>
>> -#include <asm/hyperv-tlfs.h>
>>  #include <asm/mshyperv.h>
>>
>>  /*
>> diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
>> index 0569f579338b..f022d5f64fb6 100644
>> --- a/arch/x86/hyperv/hv_apic.c
>> +++ b/arch/x86/hyperv/hv_apic.c
>> @@ -23,7 +23,6 @@
>>  #include <linux/vmalloc.h>
>>  #include <linux/mm.h>
>>  #include <linux/clockchips.h>
>> -#include <linux/hyperv.h>
>>  #include <linux/slab.h>
>>  #include <linux/cpuhotplug.h>
>>  #include <asm/hypervisor.h>
>> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> index 17a71e92a343..fc3c3d76c181 100644
>> --- a/arch/x86/hyperv/hv_init.c
>> +++ b/arch/x86/hyperv/hv_init.c
>> @@ -19,7 +19,6 @@
>>  #include <asm/sev.h>
>>  #include <asm/ibt.h>
>>  #include <asm/hypervisor.h>
>> -#include <asm/hyperv-tlfs.h>
>>  #include <asm/mshyperv.h>
>>  #include <asm/idtentry.h>
>>  #include <asm/set_memory.h>
>> @@ -27,7 +26,6 @@
>>  #include <linux/version.h>
>>  #include <linux/vmalloc.h>
>>  #include <linux/mm.h>
>> -#include <linux/hyperv.h>
>>  #include <linux/slab.h>
>>  #include <linux/kernel.h>
>>  #include <linux/cpuhotplug.h>
>> diff --git a/arch/x86/hyperv/hv_proc.c b/arch/x86/hyperv/hv_proc.c
>> index 3fa1f2ee7b0d..b74c06c04ff1 100644
>> --- a/arch/x86/hyperv/hv_proc.c
>> +++ b/arch/x86/hyperv/hv_proc.c
>> @@ -3,7 +3,6 @@
>>  #include <linux/vmalloc.h>
>>  #include <linux/mm.h>
>>  #include <linux/clockchips.h>
>> -#include <linux/hyperv.h>
>>  #include <linux/slab.h>
>>  #include <linux/cpuhotplug.h>
>>  #include <linux/minmax.h>
>> diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
>> index 60fc3ed72830..b56d70612734 100644
>> --- a/arch/x86/hyperv/ivm.c
>> +++ b/arch/x86/hyperv/ivm.c
>> @@ -7,7 +7,6 @@
>>   */
>>
>>  #include <linux/bitfield.h>
>> -#include <linux/hyperv.h>
>>  #include <linux/types.h>
>>  #include <linux/slab.h>
>>  #include <asm/svm.h>
>> diff --git a/arch/x86/hyperv/mmu.c b/arch/x86/hyperv/mmu.c
>> index 1cc113200ff5..cc8c3bd0e7c2 100644
>> --- a/arch/x86/hyperv/mmu.c
>> +++ b/arch/x86/hyperv/mmu.c
>> @@ -1,6 +1,5 @@
>>  #define pr_fmt(fmt)  "Hyper-V: " fmt
>>
>> -#include <linux/hyperv.h>
>>  #include <linux/log2.h>
>>  #include <linux/slab.h>
>>  #include <linux/types.h>
>> diff --git a/arch/x86/hyperv/nested.c b/arch/x86/hyperv/nested.c
>> index 9dc259fa322e..ee06d0315c24 100644
>> --- a/arch/x86/hyperv/nested.c
>> +++ b/arch/x86/hyperv/nested.c
>> @@ -11,7 +11,6 @@
>>
>>
>>  #include <linux/types.h>
>> -#include <asm/hyperv-tlfs.h>
>>  #include <asm/mshyperv.h>
>>  #include <asm/tlbflush.h>
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 4a68cb3eba78..3627eab994a3 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -24,7 +24,6 @@
>>  #include <linux/pvclock_gtod.h>
>>  #include <linux/clocksource.h>
>>  #include <linux/irqbypass.h>
>> -#include <linux/hyperv.h>
>>  #include <linux/kfifo.h>
>>
>>  #include <asm/apic.h>
>> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
>> index 390c4d13956d..47ca48062547 100644
>> --- a/arch/x86/include/asm/mshyperv.h
>> +++ b/arch/x86/include/asm/mshyperv.h
>> @@ -9,7 +9,6 @@
>>  #include <asm/hyperv-tlfs.h>
>>  #include <asm/nospec-branch.h>
>>  #include <asm/paravirt.h>
>> -#include <asm/mshyperv.h>
>>
>>  /*
>>   * Hyper-V always provides a single IO-APIC at this MMIO address.
>> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
>> index e0fd57a8ba84..8e8fd23b1439 100644
>> --- a/arch/x86/kernel/cpu/mshyperv.c
>> +++ b/arch/x86/kernel/cpu/mshyperv.c
>> @@ -20,7 +20,6 @@
>>  #include <linux/random.h>
>>  #include <asm/processor.h>
>>  #include <asm/hypervisor.h>
>> -#include <asm/hyperv-tlfs.h>
>>  #include <asm/mshyperv.h>
>>  #include <asm/desc.h>
>>  #include <asm/idtentry.h>
>> diff --git a/arch/x86/kvm/vmx/vmx_onhyperv.h
>> b/arch/x86/kvm/vmx/vmx_onhyperv.h
>> index eb48153bfd73..f4b081eb6521 100644
>> --- a/arch/x86/kvm/vmx/vmx_onhyperv.h
>> +++ b/arch/x86/kvm/vmx/vmx_onhyperv.h
>> @@ -3,7 +3,6 @@
>>  #ifndef __ARCH_X86_KVM_VMX_ONHYPERV_H__
>>  #define __ARCH_X86_KVM_VMX_ONHYPERV_H__
>>
>> -#include <asm/hyperv-tlfs.h>
>>  #include <asm/mshyperv.h>
>>
>>  #include <linux/jump_label.h>
>> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
>> index 44f7b2ea6a07..85fa0d4509f0 100644
>> --- a/arch/x86/mm/pat/set_memory.c
>> +++ b/arch/x86/mm/pat/set_memory.c
>> @@ -32,8 +32,6 @@
>>  #include <asm/pgalloc.h>
>>  #include <asm/proto.h>
>>  #include <asm/memtype.h>
>> -#include <asm/hyperv-tlfs.h>
>> -#include <asm/mshyperv.h>
>>
>>  #include "../mm_internal.h"
>>
>> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
>> index b2a080647e41..1b7de45a7185 100644
>> --- a/drivers/clocksource/hyperv_timer.c
>> +++ b/drivers/clocksource/hyperv_timer.c
>> @@ -23,7 +23,6 @@
>>  #include <linux/acpi.h>
>>  #include <linux/hyperv.h>
>>  #include <clocksource/hyperv_timer.h>
>> -#include <asm/hyperv-tlfs.h>
>>  #include <asm/mshyperv.h>
>>
>>  static struct clock_event_device __percpu *hv_clock_event;
>> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
>> index c38dcdfcb914..a120e9b80ded 100644
>> --- a/drivers/hv/hv_balloon.c
>> +++ b/drivers/hv/hv_balloon.c
>> @@ -28,8 +28,6 @@
>>  #include <linux/sizes.h>
>>
>>  #include <linux/hyperv.h>
>> -#include <asm/hyperv-tlfs.h>
>> -
>>  #include <asm/mshyperv.h>
>>
>>  #define CREATE_TRACE_POINTS
>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>> index 9c452bfbd571..a5217f837237 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -28,7 +28,6 @@
>>  #include <linux/slab.h>
>>  #include <linux/dma-map-ops.h>
>>  #include <linux/set_memory.h>
>> -#include <asm/hyperv-tlfs.h>
>>  #include <asm/mshyperv.h>
>>
>>  /*
>> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
>> index d35b60c06114..68e73ec7ab28 100644
>> --- a/drivers/hv/hv_kvp.c
>> +++ b/drivers/hv/hv_kvp.c
>> @@ -27,7 +27,6 @@
>>  #include <linux/connector.h>
>>  #include <linux/workqueue.h>
>>  #include <linux/hyperv.h>
>> -#include <asm/hyperv-tlfs.h>
>>
>>  #include "hyperv_vmbus.h"
>>  #include "hv_utils_transport.h"
>> diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
>> index 0d2184be1691..90fc935e89bd 100644
>> --- a/drivers/hv/hv_snapshot.c
>> +++ b/drivers/hv/hv_snapshot.c
>> @@ -12,7 +12,6 @@
>>  #include <linux/connector.h>
>>  #include <linux/workqueue.h>
>>  #include <linux/hyperv.h>
>> -#include <asm/hyperv-tlfs.h>
>>
>>  #include "hyperv_vmbus.h"
>>  #include "hv_utils_transport.h"
>> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
>> index 76ac5185a01a..321770d7ce25 100644
>> --- a/drivers/hv/hyperv_vmbus.h
>> +++ b/drivers/hv/hyperv_vmbus.h
>> @@ -15,7 +15,6 @@
>>  #include <linux/list.h>
>>  #include <linux/bitops.h>
>>  #include <asm/sync_bitops.h>
>> -#include <asm/hyperv-tlfs.h>
>>  #include <linux/atomic.h>
>>  #include <linux/hyperv.h>
>>  #include <linux/interrupt.h>
>> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
>> index e2157e387217..f77f0ea1ddad 100644
>> --- a/net/vmw_vsock/hyperv_transport.c
>> +++ b/net/vmw_vsock/hyperv_transport.c
>> @@ -13,7 +13,6 @@
>>  #include <linux/hyperv.h>
>>  #include <net/sock.h>
>>  #include <net/af_vsock.h>
>> -#include <asm/hyperv-tlfs.h>
>>
>>  /* Older (VMBUS version 'VERSION_WIN10' or before) Windows hosts have some
>>   * stricter requirements on the hv_sock ring buffer size of six 4K pages.
>> --
>> 2.34.1
>>

Powered by blists - more mailing lists