Messages in this thread | | | Date | Tue, 2 May 2017 09:59:10 +0200 | From | Ingo Molnar <> | Subject | Re: [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
| |