Re: [GIT PULL] RCU changes for v4.12
From: Ingo Molnar
Date: Tue May 02 2017 - 03:59:20 EST
* Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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
> 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.