diff mbox series

[10/12] net/mlx4: replace <linux/radix-tree.h> with <linux/radix-tree-root.h>

Message ID 1507479013-5207-11-git-send-email-yamada.masahiro@socionext.com
State New
Headers show
Series radix-tree: split out struct radix_tree_root out to <linux/radix-tree-root.h> | expand

Commit Message

Masahiro Yamada Oct. 8, 2017, 4:10 p.m. UTC
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(-)

-- 
2.7.4

Comments

David Miller Oct. 8, 2017, 5 p.m. UTC | #1
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.
Masahiro Yamada Oct. 8, 2017, 5:29 p.m. UTC | #2
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.




-- 
Best Regards
Masahiro Yamada
Joe Perches Oct. 8, 2017, 5:32 p.m. UTC | #3
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.
Leon Romanovsky Oct. 8, 2017, 6:55 p.m. UTC | #4
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
Masahiro Yamada Oct. 9, 2017, 5:55 a.m. UTC | #5
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 \





-- 
Best Regards
Masahiro Yamada
Masahiro Yamada Oct. 9, 2017, 5:56 a.m. UTC | #6
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.




-- 
Best Regards
Masahiro Yamada
Leon Romanovsky Oct. 9, 2017, 6:10 a.m. UTC | #7
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
Masahiro Yamada Oct. 9, 2017, 12:02 p.m. UTC | #8
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.


-- 
Best Regards
Masahiro Yamada
diff mbox series

Patch

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>