lkml.org 
[lkml]   [2017]   [May]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [GIT PULL] RCU changes for v4.12

* Paul E. McKenney <paulmck@linux.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@kernel.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

\
 
 \ /
  Last update: 2017-05-02 10:00    [W:0.133 / U:0.440 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site