Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1634068 > unrolled thread

Re: [GIT PULL] RCU changes for v4.12

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [GIT PULL] RCU changes for v4.12 Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-02 03:20 +0200
    Re: [GIT PULL] RCU changes for v4.12 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-02 06:10 +0200
      Re: [GIT PULL] RCU changes for v4.12 Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-02 06:20 +0200
        Re: [GIT PULL] RCU changes for v4.12 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-02 06:40 +0200
      Re: [GIT PULL] RCU changes for v4.12 Ingo Molnar <mingo@kernel.org> - 2017-05-02 10:00 +0200
        Re: [PATCH] srcu: Debloat the <linux/rcu_segcblist.h> header "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-02 15:00 +0200

#1634068 — Re: [GIT PULL] RCU changes for v4.12

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.

               Linus

[toc] | [next] | [standalone]

#1634124

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 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?

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1634128

On Mon, May 1, 2017 at 9:02 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> 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?

That's the part I reacted to while reading through the patch, and your
suggested fix sounds good to me.

                     Linus

[toc] | [prev] | [next] | [standalone]

#1634132

On Mon, May 01, 2017 at 09:11:05PM -0700, Linus Torvalds wrote:
> On Mon, May 1, 2017 at 9:02 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > 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?
> 
> That's the part I reacted to while reading through the patch, and your
> suggested fix sounds good to me.

Thank you for the confirmation!

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1634210

* 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

[toc] | [prev] | [next] | [standalone]

#1634386 — Re: [PATCH] srcu: Debloat the <linux/rcu_segcblist.h> header

On Tue, May 02, 2017 at 03:25:03AM -0700, Paul E. McKenney wrote:
> On Tue, May 02, 2017 at 10:31:18AM +0200, Ingo Molnar wrote:
> > 
> > * Ingo Molnar <mingo@kernel.org> wrote:
> > 
> > > > 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.
> > 
> > I.e. the attached patch as a first step.
> > 
> > Only minimally build tested, but it appears to be do the trick.
> > 
> > This changes the <linux/rcu_segcblist.h> header from ~700 lines to only 90 lines - 
> > 60% of which is comments.
> > 
> > The deinlining of the inline functions within kernel/rcu/ should also be done, as 
> > a separate patch on top of this.
> > 
> > What do you think?
> 
> Thank you very much!
> 
> I have pulled this in and started a short rcutorture run.  I will do
> de-inline and create a .c file on top of this and retest after the
> rcutorture has finished.

And I am applying the following fixlet for !SMP builds, which I am
retesting.  Other than that, initial rcutorture ran fine!

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
index b8293527ee18..36e1f82faed1 100644
--- a/kernel/rcu/srcutiny.c
+++ b/kernel/rcu/srcutiny.c
@@ -30,6 +30,7 @@
 #include <linux/srcu.h>
 
 #include <linux/rcu_node_tree.h>
+#include "rcu_segcblist.h"
 #include "rcu.h"
 
 static int init_srcu_struct_fields(struct srcu_struct *sp)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web