| Message ID | 1507479013-5207-11-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | radix-tree: split out struct radix_tree_root out to <linux/radix-tree-root.h> | expand |
From: Masahiro Yamada <yamada.masahiro@socionext.com> Date: Mon, 9 Oct 2017 01:10:11 +0900 > The headers > - include/linux/mlx4/device.h > - drivers/net/ethernet/mellanox/mlx4/mlx4.h > require the definition of struct radix_tree_root, but do not need to > know anything about other radix tree stuff. > > Include <linux/radix-tree-root.h> instead of <linux/radix-tree.h> to > reduce the header dependency. > > While we are here, let's add missing <linux/radix-tree.h> where > radix tree accessors are used. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> Honestly this makes things more complicated. The driver was trying to consolidate all of the header needs by including them all in one place, the main driver header. Now you're including headers in several different files. I really don't like the results of this change and would ask you to reconsider. Just add both radix-tree-root.h _and_ radix-tree.h to mlx4.h and leave the rest of the driver alone.
2017-10-09 2:00 GMT+09:00 David Miller <davem@davemloft.net>: > From: Masahiro Yamada <yamada.masahiro@socionext.com> > Date: Mon, 9 Oct 2017 01:10:11 +0900 > >> The headers >> - include/linux/mlx4/device.h >> - drivers/net/ethernet/mellanox/mlx4/mlx4.h >> require the definition of struct radix_tree_root, but do not need to >> know anything about other radix tree stuff. >> >> Include <linux/radix-tree-root.h> instead of <linux/radix-tree.h> to >> reduce the header dependency. >> >> While we are here, let's add missing <linux/radix-tree.h> where >> radix tree accessors are used. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > Honestly this makes things more complicated. The idea is simple; include necessary headers explicitly. Putting everything into one common header means most of C files are forced to parse unnecessary headers. > The driver was trying to consolidate all of the header needs > by including them all in one place, the main driver header. > > Now you're including headers in several different files. > > I really don't like the results of this change and would > ask you to reconsider. > > Just add both radix-tree-root.h _and_ radix-tree.h to mlx4.h > and leave the rest of the driver alone. If you do not like this, you can just throw it away. <linux/radix-tree.h> includes <linux/radix-tree-root.h>. You do not need to include both.
On Mon, 2017-10-09 at 02:29 +0900, Masahiro Yamada wrote:
> The idea is simple; include necessary headers explicitly.
Try that for kernel.h
There's a reason aggregation of #includes is useful.
On Mon, Oct 09, 2017 at 02:29:15AM +0900, Masahiro Yamada wrote: > 2017-10-09 2:00 GMT+09:00 David Miller <davem@davemloft.net>: > > From: Masahiro Yamada <yamada.masahiro@socionext.com> > > Date: Mon, 9 Oct 2017 01:10:11 +0900 > > > >> The headers > >> - include/linux/mlx4/device.h > >> - drivers/net/ethernet/mellanox/mlx4/mlx4.h > >> require the definition of struct radix_tree_root, but do not need to > >> know anything about other radix tree stuff. > >> > >> Include <linux/radix-tree-root.h> instead of <linux/radix-tree.h> to > >> reduce the header dependency. > >> > >> While we are here, let's add missing <linux/radix-tree.h> where > >> radix tree accessors are used. > >> > >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > Honestly this makes things more complicated. > > > The idea is simple; include necessary headers explicitly. > > Putting everything into one common header > means most of C files are forced to parse unnecessary headers. It is neglected, only first caller will actually parse that header file, other callers will check the #ifndef pragma without need to reparse the whole file. > > > > > The driver was trying to consolidate all of the header needs > > by including them all in one place, the main driver header. > > > > Now you're including headers in several different files. > > > > I really don't like the results of this change and would > > ask you to reconsider. > > > > Just add both radix-tree-root.h _and_ radix-tree.h to mlx4.h > > and leave the rest of the driver alone. > > > If you do not like this, you can just throw it away. > > <linux/radix-tree.h> includes <linux/radix-tree-root.h>. > You do not need to include both. > > > > > -- > Best Regards > Masahiro Yamada > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
2017-10-09 2:32 GMT+09:00 Joe Perches <joe@perches.com>: > On Mon, 2017-10-09 at 02:29 +0900, Masahiro Yamada wrote: >> The idea is simple; include necessary headers explicitly. > > Try that for kernel.h > > There's a reason aggregation of #includes is useful. > We should use a common sense for the balance between - aggregation of #includes - reduce the number of parsed headers As I had already said, if you do not like this change, you can just throw it away. That's fine because the impact is very limited. Honestly, I am not so interested in this driver. I changed sh, net/mlx4, net/mlx5, drm/i915 for bonus while I am here. I assume your objection is addressed to this driver change, not the whole series. I am more interested in the global headers like include/linux/{irqdomain.h, fs.h} etc. radix-tree-root.h vs radix-tree.h has a big difference in terms of parsed headers (17 vs 128). For example, the following is the analysis on arm64. <linux/radix-tree-root.h> include the following 17 headers: include/linux/radix-tree-root.h \ include/linux/types.h \ include/uapi/linux/types.h \ arch/arm64/include/generated/uapi/asm/types.h \ include/uapi/asm-generic/types.h \ include/asm-generic/int-ll64.h \ include/uapi/asm-generic/int-ll64.h \ arch/arm64/include/uapi/asm/bitsperlong.h \ include/asm-generic/bitsperlong.h \ include/uapi/asm-generic/bitsperlong.h \ include/uapi/linux/posix_types.h \ include/linux/stddef.h \ include/uapi/linux/stddef.h \ include/linux/compiler.h \ include/linux/compiler-gcc.h \ arch/arm64/include/uapi/asm/posix_types.h \ include/uapi/asm-generic/posix_types.h \ <linux/radix-tree.h> include the following 128 headers: include/linux/radix-tree.h \ include/linux/bitops.h \ arch/arm64/include/generated/uapi/asm/types.h \ include/uapi/asm-generic/types.h \ include/asm-generic/int-ll64.h \ include/uapi/asm-generic/int-ll64.h \ arch/arm64/include/uapi/asm/bitsperlong.h \ include/asm-generic/bitsperlong.h \ include/uapi/asm-generic/bitsperlong.h \ arch/arm64/include/asm/bitops.h \ include/linux/compiler.h \ include/linux/compiler-gcc.h \ include/uapi/linux/types.h \ include/uapi/linux/posix_types.h \ include/linux/stddef.h \ include/uapi/linux/stddef.h \ arch/arm64/include/uapi/asm/posix_types.h \ include/uapi/asm-generic/posix_types.h \ arch/arm64/include/asm/barrier.h \ include/asm-generic/barrier.h \ include/asm-generic/bitops/builtin-__ffs.h \ include/asm-generic/bitops/builtin-ffs.h \ include/asm-generic/bitops/builtin-__fls.h \ include/asm-generic/bitops/builtin-fls.h \ include/asm-generic/bitops/ffz.h \ include/asm-generic/bitops/fls64.h \ include/asm-generic/bitops/find.h \ include/asm-generic/bitops/sched.h \ include/asm-generic/bitops/hweight.h \ include/asm-generic/bitops/arch_hweight.h \ include/asm-generic/bitops/const_hweight.h \ include/asm-generic/bitops/lock.h \ include/asm-generic/bitops/non-atomic.h \ include/asm-generic/bitops/le.h \ arch/arm64/include/uapi/asm/byteorder.h \ include/linux/byteorder/big_endian.h \ include/uapi/linux/byteorder/big_endian.h \ include/linux/types.h \ include/linux/swab.h \ include/uapi/linux/swab.h \ arch/arm64/include/generated/uapi/asm/swab.h \ include/uapi/asm-generic/swab.h \ include/linux/byteorder/generic.h \ include/linux/bug.h \ arch/arm64/include/asm/bug.h \ include/linux/stringify.h \ arch/arm64/include/asm/asm-bug.h \ arch/arm64/include/asm/brk-imm.h \ include/asm-generic/bug.h \ include/linux/kernel.h \ /home/masahiro/toolchains/aarch64-linaro-4.9/gcc-linaro-4.9-2016.02-x86_64_aarch64-linux-gnu/lib/gcc/aarch64-linux-gnu/4.9.4/include/stdarg.h \ include/linux/linkage.h \ include/linux/export.h \ arch/arm64/include/asm/linkage.h \ include/linux/log2.h \ include/linux/typecheck.h \ include/linux/printk.h \ include/linux/init.h \ include/linux/kern_levels.h \ include/linux/cache.h \ include/uapi/linux/kernel.h \ include/uapi/linux/sysinfo.h \ arch/arm64/include/asm/cache.h \ arch/arm64/include/asm/cputype.h \ arch/arm64/include/asm/sysreg.h \ include/linux/dynamic_debug.h \ include/linux/jump_label.h \ arch/arm64/include/asm/jump_label.h \ arch/arm64/include/asm/insn.h \ include/linux/build_bug.h \ include/linux/list.h \ include/linux/poison.h \ include/uapi/linux/const.h \ include/linux/preempt.h \ arch/arm64/include/generated/asm/preempt.h \ include/asm-generic/preempt.h \ include/linux/thread_info.h \ include/linux/restart_block.h \ arch/arm64/include/asm/current.h \ arch/arm64/include/asm/thread_info.h \ arch/arm64/include/asm/memory.h \ arch/arm64/include/asm/page-def.h \ arch/arm64/include/generated/asm/sizes.h \ include/asm-generic/sizes.h \ include/linux/sizes.h \ include/linux/mmdebug.h \ include/asm-generic/memory_model.h \ include/linux/pfn.h \ arch/arm64/include/asm/stack_pointer.h \ include/linux/radix-tree-root.h \ include/linux/rcupdate.h \ include/linux/atomic.h \ arch/arm64/include/asm/atomic.h \ arch/arm64/include/asm/lse.h \ arch/arm64/include/asm/atomic_ll_sc.h \ arch/arm64/include/asm/cmpxchg.h \ include/asm-generic/atomic-long.h \ include/linux/irqflags.h \ arch/arm64/include/asm/irqflags.h \ arch/arm64/include/asm/ptrace.h \ arch/arm64/include/uapi/asm/ptrace.h \ arch/arm64/include/asm/hwcap.h \ arch/arm64/include/uapi/asm/hwcap.h \ include/asm-generic/ptrace.h \ include/linux/bottom_half.h \ include/linux/lockdep.h \ include/linux/debug_locks.h \ include/linux/stacktrace.h \ arch/arm64/include/asm/processor.h \ include/linux/string.h \ include/uapi/linux/string.h \ arch/arm64/include/asm/string.h \ arch/arm64/include/asm/alternative.h \ arch/arm64/include/asm/cpucaps.h \ arch/arm64/include/asm/fpsimd.h \ arch/arm64/include/asm/hw_breakpoint.h \ arch/arm64/include/asm/cpufeature.h \ arch/arm64/include/asm/virt.h \ arch/arm64/include/asm/sections.h \ include/asm-generic/sections.h \ arch/arm64/include/asm/pgtable-hwdef.h \ include/linux/cpumask.h \ include/linux/threads.h \ include/linux/bitmap.h \ include/linux/rcutree.h \ include/linux/spinlock_types.h \ arch/arm64/include/asm/spinlock_types.h \ include/linux/rwlock_types.h \
2017-10-09 3:55 GMT+09:00 Leon Romanovsky <leon@kernel.org>: > On Mon, Oct 09, 2017 at 02:29:15AM +0900, Masahiro Yamada wrote: >> 2017-10-09 2:00 GMT+09:00 David Miller <davem@davemloft.net>: >> > From: Masahiro Yamada <yamada.masahiro@socionext.com> >> > Date: Mon, 9 Oct 2017 01:10:11 +0900 >> > >> >> The headers >> >> - include/linux/mlx4/device.h >> >> - drivers/net/ethernet/mellanox/mlx4/mlx4.h >> >> require the definition of struct radix_tree_root, but do not need to >> >> know anything about other radix tree stuff. >> >> >> >> Include <linux/radix-tree-root.h> instead of <linux/radix-tree.h> to >> >> reduce the header dependency. >> >> >> >> While we are here, let's add missing <linux/radix-tree.h> where >> >> radix tree accessors are used. >> >> >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> > >> > Honestly this makes things more complicated. >> >> >> The idea is simple; include necessary headers explicitly. >> >> Putting everything into one common header >> means most of C files are forced to parse unnecessary headers. > > It is neglected, only first caller will actually parse that header file, > other callers will check the #ifndef pragma without need to reparse the > whole file. > You completely neglected the point of the discussion. I am trying to not include unnecessary headers at all.
On Mon, Oct 09, 2017 at 02:56:56PM +0900, Masahiro Yamada wrote: > 2017-10-09 3:55 GMT+09:00 Leon Romanovsky <leon@kernel.org>: > > On Mon, Oct 09, 2017 at 02:29:15AM +0900, Masahiro Yamada wrote: > >> 2017-10-09 2:00 GMT+09:00 David Miller <davem@davemloft.net>: > >> > From: Masahiro Yamada <yamada.masahiro@socionext.com> > >> > Date: Mon, 9 Oct 2017 01:10:11 +0900 > >> > > >> >> The headers > >> >> - include/linux/mlx4/device.h > >> >> - drivers/net/ethernet/mellanox/mlx4/mlx4.h > >> >> require the definition of struct radix_tree_root, but do not need to > >> >> know anything about other radix tree stuff. > >> >> > >> >> Include <linux/radix-tree-root.h> instead of <linux/radix-tree.h> to > >> >> reduce the header dependency. > >> >> > >> >> While we are here, let's add missing <linux/radix-tree.h> where > >> >> radix tree accessors are used. > >> >> > >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > >> > > >> > Honestly this makes things more complicated. > >> > >> > >> The idea is simple; include necessary headers explicitly. > >> > >> Putting everything into one common header > >> means most of C files are forced to parse unnecessary headers. > > > > It is neglected, only first caller will actually parse that header file, > > other callers will check the #ifndef pragma without need to reparse the > > whole file. > > > > > You completely neglected the point of the discussion. > > I am trying to not include unnecessary headers at all. > I understand it and have no issues with that, just have hard time to justify for myself any benefit of doing it. Thanks > > > > -- > Best Regards > Masahiro Yamada
2017-10-09 2:32 GMT+09:00 Joe Perches <joe@perches.com>: > On Mon, 2017-10-09 at 02:29 +0900, Masahiro Yamada wrote: >> The idea is simple; include necessary headers explicitly. > > Try that for kernel.h > > There's a reason aggregation of #includes is useful. > BTW, talking about <linux/kernel.h>, it is too much aggregation, isn't it? It exceed 850 lines, and contains lots of unrelated stuff in one header. Perhaps, it could be a good time to think about splitting? For example, I see many trace_... things in it. I wonder if linux/kernel.h is a good home for them, or a separate file.
diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c index 72eb50c..4cbe65c 100644 --- a/drivers/net/ethernet/mellanox/mlx4/cq.c +++ b/drivers/net/ethernet/mellanox/mlx4/cq.c @@ -36,6 +36,7 @@ #include <linux/hardirq.h> #include <linux/export.h> +#include <linux/radix-tree.h> #include <linux/mlx4/cmd.h> #include <linux/mlx4/cq.h> diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4.h b/drivers/net/ethernet/mellanox/mlx4/mlx4.h index c68da19..975ef70 100644 --- a/drivers/net/ethernet/mellanox/mlx4/mlx4.h +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4.h @@ -38,7 +38,7 @@ #define MLX4_H #include <linux/mutex.h> -#include <linux/radix-tree.h> +#include <linux/radix-tree-root.h> #include <linux/rbtree.h> #include <linux/timer.h> #include <linux/semaphore.h> diff --git a/drivers/net/ethernet/mellanox/mlx4/qp.c b/drivers/net/ethernet/mellanox/mlx4/qp.c index 728a2fb..50cbc62 100644 --- a/drivers/net/ethernet/mellanox/mlx4/qp.c +++ b/drivers/net/ethernet/mellanox/mlx4/qp.c @@ -35,6 +35,7 @@ #include <linux/gfp.h> #include <linux/export.h> +#include <linux/radix-tree.h> #include <linux/mlx4/cmd.h> #include <linux/mlx4/qp.h> diff --git a/drivers/net/ethernet/mellanox/mlx4/srq.c b/drivers/net/ethernet/mellanox/mlx4/srq.c index bedf521..4201a46 100644 --- a/drivers/net/ethernet/mellanox/mlx4/srq.c +++ b/drivers/net/ethernet/mellanox/mlx4/srq.c @@ -36,6 +36,7 @@ #include <linux/mlx4/srq.h> #include <linux/export.h> #include <linux/gfp.h> +#include <linux/radix-tree.h> #include "mlx4.h" #include "icm.h" diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h index b0a57e0..75eac23 100644 --- a/include/linux/mlx4/device.h +++ b/include/linux/mlx4/device.h @@ -36,7 +36,7 @@ #include <linux/if_ether.h> #include <linux/pci.h> #include <linux/completion.h> -#include <linux/radix-tree.h> +#include <linux/radix-tree-root.h> #include <linux/cpu_rmap.h> #include <linux/crash_dump.h> diff --git a/include/linux/mlx4/qp.h b/include/linux/mlx4/qp.h index 8e2828d..dfa7d8e 100644 --- a/include/linux/mlx4/qp.h +++ b/include/linux/mlx4/qp.h @@ -35,6 +35,7 @@ #include <linux/types.h> #include <linux/if_ether.h> +#include <linux/radix-tree.h> #include <linux/mlx4/device.h>
The headers - include/linux/mlx4/device.h - drivers/net/ethernet/mellanox/mlx4/mlx4.h require the definition of struct radix_tree_root, but do not need to know anything about other radix tree stuff. Include <linux/radix-tree-root.h> instead of <linux/radix-tree.h> to reduce the header dependency. While we are here, let's add missing <linux/radix-tree.h> where radix tree accessors are used. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- drivers/net/ethernet/mellanox/mlx4/cq.c | 1 + drivers/net/ethernet/mellanox/mlx4/mlx4.h | 2 +- drivers/net/ethernet/mellanox/mlx4/qp.c | 1 + drivers/net/ethernet/mellanox/mlx4/srq.c | 1 + include/linux/mlx4/device.h | 2 +- include/linux/mlx4/qp.h | 1 + 6 files changed, 6 insertions(+), 2 deletions(-)