Re: [PATCH -tip v3 09/11] data_race: Avoid nested statement expression

[Marco Elver <elver@google.com>]

On Tue, 26 May 2020, Marco Elver wrote:

> On Tue, 26 May 2020 at 14:19, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, May 26, 2020 at 2:02 PM Will Deacon <will@kernel.org> wrote:
> > > On Tue, May 26, 2020 at 12:42:16PM +0200, Arnd Bergmann wrote:
> > > >
> > > > I find this patch only solves half the problem: it's much faster than
> > > > without the
> > > > patch, but still much slower than the current mainline version. As far as I'm
> > > > concerned, I think the build speed regression compared to mainline is not yet
> > > > acceptable, and we should try harder.
> > > >
> > > > I have not looked too deeply at it yet, but this is what I found from looking
> > > > at a file in a randconfig build:
> > > >
> > > > Configuration: see https://pastebin.com/raw/R9erCwNj
> > >
> > > So this .config actually has KCSAN enabled. Do you still see the slowdown
> > > with that disabled?
> >
> > Yes, enabling or disabling KCSAN seems to make no difference to
> > compile speed in this config and source file, I still get the 12 seconds
> > preprocessing time and 9MB file size with KCSAN disabled, possibly
> > a few percent smaller/faster. I actually thought that CONFIG_FTRACE
> > had a bigger impact, but disabling that also just reduces the time
> > by a few percent rather than getting it down to the expected milliseconds.
> >
> > > Although not ideal, having a longer compiler time when
> > > the compiler is being asked to perform instrumentation doesn't seem like a
> > > show-stopper to me.
> >
> > I agree in general, but building an allyesconfig kernel is still an important
> > use case that should not take twice as long after a small kernel change
> > regardless of whether a new feature is used or not. (I have not actually
> > compared the overall build speed for allmodconfig, as this takes a really
> > long time at the moment)
> 
> Note that an 'allyesconfig' selects KASAN and not KCSAN by default.
> But I think that's not relevant, since KCSAN-specific code was removed
> from ONCEs. In general though, it is entirely expected that we have a
> bit longer compile times when we have the instrumentation passes
> enabled.
> 
> But as you pointed out, that's irrelevant, and the significant
> overhead is from parsing and pre-processing. FWIW, we can probably
> optimize Clang itself a bit:
> https://github.com/ClangBuiltLinux/linux/issues/1032#issuecomment-633712667

Found that optimizing __unqual_scalar_typeof makes a noticeable
difference. We could use C11's _Generic if the compiler supports it (and
all supported versions of Clang certainly do).

Could you verify if the below patch improves compile-times for you? E.g.
on fs/ocfs2/journal.c I was able to get ~40% compile-time speedup.

Thanks,
-- Marco

------ >8 ------

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 5faf68eae204..a529fa263906 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -245,7 +245,9 @@ struct ftrace_likely_data {
 /*
  * __unqual_scalar_typeof(x) - Declare an unqualified scalar type, leaving
  *			       non-scalar types unchanged.
- *
+ */
+#if defined(CONFIG_CC_IS_GCC) && CONFIG_GCC_VERSION < 40900
+/*
  * We build this out of a couple of helper macros in a vain attempt to
  * help you keep your lunch down while reading it.
  */
@@ -267,6 +269,24 @@ struct ftrace_likely_data {
 			__pick_integer_type(x, int,				\
 				__pick_integer_type(x, long,			\
 					__pick_integer_type(x, long long, x))))))
+#else
+/*
+ * If supported, prefer C11 _Generic for better compile-times. As above, 'char'
+ * is not type-compatible with 'signed char', and we define a separate case.
+ */
+#define __scalar_type_to_expr_cases(type)				\
+		type: (type)0, unsigned type: (unsigned type)0
+
+#define __unqual_scalar_typeof(x) typeof(				\
+		_Generic((x),						\
+			 __scalar_type_to_expr_cases(char),		\
+			 signed char: (signed char)0,			\
+			 __scalar_type_to_expr_cases(short),		\
+			 __scalar_type_to_expr_cases(int),		\
+			 __scalar_type_to_expr_cases(long),		\
+			 __scalar_type_to_expr_cases(long long),	\
+			 default: (x)))
+#endif
 
 /* Is this type a native word size -- useful for atomic operations */
 #define __native_word(t) \