Message-ID: <20170502075910.d7dl762muvuc5uoh@gmail.com>
Date:   Tue, 2 May 2017 09:59:10 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [GIT PULL] RCU changes for v4.12


* Paul E. McKenney <paulmck@...ux.vnet.ibm.com> wrote:

> On Mon, May 01, 2017 at 06:19:44PM -0700, Linus Torvalds wrote:
> > On Mon, May 1, 2017 at 2:59 AM, Ingo Molnar <mingo@...nel.org> wrote:
> > > Linus,
> > >
> > > Please pull the latest core-rcu-for-linus git tree from:
> > >
> > >    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git core-rcu-for-linus
> > 
> > I pulled this, and then after looking at it, ended up un-pulling it again.
> > 
> > I refuse to take that nasty <linux/rcu_segcblist.h> header file from hell.
> > 
> > I see absolutely no point in taking a header file of several hundred
> > lines of code.
> > 
> > We have traditionally done too much inline code anyway, but we've
> > learnt our lesson - and even back when we did too much of it, we
> > didn't put random code that nobody uses and by definition cannot be
> > performance-critical in big inline functions in header files.
> > 
> > If it was some one-liner helper function, that would be one thing. But
> > there are functions that don't even fit on the screen, and that have
> > multiple loops and memory barriers in them.
> > 
> > The one function I decided to grep for was used EXACTLY NOWHERE. Yet
> > it was apparently SO INCREDIBLY important that it needed to be inlined
> > in a huge header file despite being huge and complicated.
> > 
> > So no. This is too ugly to live, and certainly too ugly to be pulled.
> > 
> > The RCU code needs to start showing some good taste.
> > 
> > There are valid reasons to inline even large functions, if they have
> > constant arguments that make us expect them to generate a single
> > instruction of code in the end. But that was very much not the case
> > here.
> > 
> > Not pulling. Try again next merge window when the code has been
> > cleaned up and isn't too ugly to live.
> 
> Please accept my apologies!
> 
> I was patterning this code too much after the various *list*.h header
> files, and failed to notice that the functions were getting large.

I too should have noticed the large inline functions when pulling it. :-/

Header file bloat is a creeping problem that has gotten (much) worse over the
last 10 years, so the pushback from Linus against adding more bloat to 
include/linux/ is fully justified.

> I will get rid of the unused rcu_segcblist_extract_all() function and create a 
> kernel/rcu/segcblist.c for the functions that are either non-trivial or 
> performance-insensitive.
> 
> Does that cover it, or am I missing something?

I'd also suggest moving as much of the RCU internal data types into kernel/rcu/ as 
possible. It's not clear to me which part of it is supposed to be a public API and 
which bits are internal. It might make sense to keep it internal for the time 
being, and only export things once there are users.

I.e. a pretty good solution would be to move all of include/linux/rcu_segcblist.h 
to kernel/rcu/rcu_segcblist.c or so - and do a kernel/rcu/rcu_segcblist.h with the 
data types and function prototypes.

There's also appears to be inline functions wrappery that I think obfuscates the 
code: for example why is there rcu_cblist_n_cbs()? Users could directly 
dereference ->len. Once these are eliminated there's very few inline functions 
remaining that should truly be inline.

Thanks,

	Ingo

Powered by blists - more mailing lists