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

Groups > linux.kernel > #1638979 > unrolled thread

Re: [RFC GIT PULL, v2] 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: [RFC GIT PULL, v2] RCU changes for v4.12 Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-10 19:30 +0200
    Re: [RFC GIT PULL, v2] RCU changes for v4.12 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-10 22:00 +0200
      Re: [RFC GIT PULL, v2] RCU changes for v4.12 Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-10 22:20 +0200
        Re: [RFC GIT PULL, v2] RCU changes for v4.12 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-10 23:00 +0200
          Re: [RFC GIT PULL, v2] RCU changes for v4.12 Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-10 23:10 +0200
            Re: [RFC GIT PULL, v2] RCU changes for v4.12 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-05-11 01:00 +0200
    Re: [RFC GIT PULL, v2] RCU changes for v4.12 Ingo Molnar <mingo@kernel.org> - 2017-05-10 22:00 +0200
      Re: [RFC GIT PULL, v2] RCU changes for v4.12 Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-10 22:10 +0200
        Re: [RFC GIT PULL, v2] RCU changes for v4.12 Ingo Molnar <mingo@kernel.org> - 2017-05-11 09:00 +0200

#1638979 — Re: [RFC GIT PULL, v2] RCU changes for v4.12

On Tue, May 9, 2017 at 12:26 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> The main changes are:

So I've pulled it now (although it is showing signs of semantic
conflicts, so I'll have to look at those), but I've got two requests,
one for Ingo, one for Paul.

Ingo: please don't bother sending me stupid crap.

And by that I mean the whole patch WHEN IT IS 300kB IN SIZE!

That's just idiotic. Nobody is ever going to review a 300kB patch that
is ~7500 lines. All it does is waste time, and make it a pain to even
reply to your emails (since I have "include quoted original" on by
default in order to be able to quote and reply sanely).

There's a reason "git request-pull" only does the diffstat and the shortlog.

So please fix your scripts. If a patch is so big that is is not worth
reviewing, don't include it. I don't know exactly where that limit is,
but I would suggest that it is on the order of 1000 lines or so.

This is particularly annoying, because your pull request is one huge
pile of shit. It has all that completely useless stuff that nobody is
ever going to look at, and it didn't actually mention the *important*
parts, namely how the RCU changes apparently mess with the DRM
selftest changes.

So stop sending me stupid crap, and please send me the *relevant* stuff instead.

>   - Debloat RCU headers
>
>   - Parallelize SRCU callback handling (plus overlapping patches)
>
>   - Improve the performance of Tree SRCU on a CPU-hotplug stress test

And for Paul: the RCU subsystem is starting to get ridiculous. Seriously.

That is *particularly* true for srcu. We don't even have all that many
users, and I suspect a large subset of those users are just crap to
begin with. The biggest reason for srcu seems to be bad callbacks,
particularly shit like the mmu notifier code. Things that we probably
shouldn't have done in the first place, and where srcu just encouraged
people to do bad things.

Seriously, do this:

   git grep srcu.*lock -- :^Documentation/ :^kernel/rcu/

and notice that we have only a few hundred lines in the kernel that do
srcu locking. kvm seems to be the main big user.

This annoys me, because the main reason people use srcu is bad design
and lazyness, where they can't be arsed to try to minimize locking and
sleeping things.  The "sleeping callbacks" in particular tend to be a
huge design mistake.

Yet, despite this fairly limited use, rscu seems to be just growing
and bloating, and making more and more excuses fro bad behavior.

And it was *years* since I asked you to look at getting rid of the
absolutely insane proliferations of different RCU models. I don't
think anything ever happened. We *still* have TREE_RCU,| PREEMPT_RCU,
and TINY_SRCU.

And with this pull request we now have CLASSIC_SRCU, TINY_SRCU,
TREE_SRCU and TASKS_RCU.

That's in addition to all the other insane tweaks that nobody uses (eg
RCU_FANOUT etc) and that I made sure got removed from any sane
questionnaire.

Paul, this really needs to stop.

I'm now going to stop pulling any more crazy RCU crap. Seriously. If
the RCU subsystem doesn't start shrinking, I'm no longer pulling. Send
me fixes, but don't send me more of this crazy stuff.

So this is me putting my foot down. I should have done it long ago.
I'm done with crazy. Don't waste your time doing yet another RCU mode,
because I will not take it. And don't waste your time expanding on the
existing ones without looking at which of those things can be removed.

                    Linus

[toc] | [next] | [standalone]

#1639062

On Wed, May 10, 2017 at 10:27:24AM -0700, Linus Torvalds wrote:

[ . . . ]

> parts, namely how the RCU changes apparently mess with the DRM
> selftest changes.

I am testing a merge with current linus/master, and I looked through
the commits in -next selected by:

	gitk v4.11.. --no-merges --all-match --grep=drm --grep=selftest

I didn't find anything obvious.  If the tests complete successfully,
I will try running the DRM selftest.

[ . . . ]

> And for Paul: the RCU subsystem is starting to get ridiculous. Seriously.
> 
> That is *particularly* true for srcu. We don't even have all that many
> users, and I suspect a large subset of those users are just crap to
> begin with. The biggest reason for srcu seems to be bad callbacks,
> particularly shit like the mmu notifier code. Things that we probably
> shouldn't have done in the first place, and where srcu just encouraged
> people to do bad things.
> 
> Seriously, do this:
> 
>    git grep srcu.*lock -- :^Documentation/ :^kernel/rcu/
> 
> and notice that we have only a few hundred lines in the kernel that do
> srcu locking. kvm seems to be the main big user.
> 
> This annoys me, because the main reason people use srcu is bad design
> and lazyness, where they can't be arsed to try to minimize locking and
> sleeping things.  The "sleeping callbacks" in particular tend to be a
> huge design mistake.
> 
> Yet, despite this fairly limited use, rscu seems to be just growing
> and bloating, and making more and more excuses fro bad behavior.

I can certainly revisit the uses.  If it ends up that no SRCU users
really need SRCU, then it should of course be removed.

> And it was *years* since I asked you to look at getting rid of the
> absolutely insane proliferations of different RCU models. I don't
> think anything ever happened. We *still* have TREE_RCU,| PREEMPT_RCU,
> and TINY_SRCU.

I did remove TINY_PREEMPT_RCU in response to your request.

I have also removed the Kconfig parameters SRCU_SYNCHRONIZE_DELAY,
RCU_CPU_STALL_DETECTOR, PROVE_RCU_DELAY, RCU_CPU_STALL_VERBOSE,
CONFIG_RCU_FANOUT_EXACT, CONFIG_RCU_CPU_STALL_INFO, and
RCU_TORTURE_TEST_RUNNABLE.  There are quite a few more that could
be removed:

o	RCU_TORTURE_TEST_SLOW_PREINIT, RCU_TORTURE_TEST_SLOW_PREINIT_DELAY,
	RCU_TORTURE_TEST_SLOW_PREINIT_DELAY, RCU_TORTURE_TEST_SLOW_INIT,
	RCU_TORTURE_TEST_SLOW_INIT_DELAY, RCU_TORTURE_TEST_SLOW_CLEANUP,
	and RCU_TORTURE_TEST_SLOW_CLEANUP_DELAY.  I will queue patches
	to remove these.

o	I believe that RCU_CPU_STALL_TIMEOUT could be dropped in favor
	of the existing rcupdate.rcu_cpu_stall_timeout kernel parameter,
	I will queue a patch and see if anyone screams.

o	RCU_KTHREAD_PRIO could be dropped in favor of the existing
	rcutree.kthread_prio kernel parameter.  I will queue a patch,
	and I would be very surprised if anyone screamed.

o	RCU_BOOST_DELAY could be a boot parameter.  I will queue a
	patch.

o	I believe that PROVE_RCU_REPEATEDLY could be a boot parameter,
	I will queue a patch and see if anyone screams.

o	Not sure about SPARSE_RCU_POINTER, will try making this
	unconditional and see if anyone screams.

o	It might be possible to eliminate RCU_NOCB_CPU_NONE,
	RCU_NOCB_CPU_ZERO, and RCU_NOCB_CPU_ALL.  I will look
	into this.

> And with this pull request we now have CLASSIC_SRCU, TINY_SRCU,
> TREE_SRCU and TASKS_RCU.

I have a calendar reminder to remove CLASSIC_SRCU near the end of this
year.  Given the testing results thus far, I would be happy to remove
it sooner if you would prefer.

> That's in addition to all the other insane tweaks that nobody uses (eg
> RCU_FANOUT etc) and that I made sure got removed from any sane
> questionnaire.

I use RCU_FANOUT and RCU_FANOUT_LEAF to test code paths on small systems
that would otherwise only be exercised on systems with thousands of CPUs.
I am not aware of any other use.  If it would help, I would be happy to
move them to lib/Kconfig.debug and make them depend on TORTURE_TEST.

> Paul, this really needs to stop.
> 
> I'm now going to stop pulling any more crazy RCU crap. Seriously. If
> the RCU subsystem doesn't start shrinking, I'm no longer pulling. Send
> me fixes, but don't send me more of this crazy stuff.
> 
> So this is me putting my foot down. I should have done it long ago.
> I'm done with crazy. Don't waste your time doing yet another RCU mode,
> because I will not take it. And don't waste your time expanding on the
> existing ones without looking at which of those things can be removed.

OK, nothing more from RCU other than fixes, code reduction, and
documentation for the time being.

							Thanx, Paul

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

#1639070

On Wed, May 10, 2017 at 12:54 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> I am testing a merge with current linus/master, and I looked through
> the commits in -next selected by:
>
>         gitk v4.11.. --no-merges --all-match --grep=drm --grep=selftest
>
> I didn't find anything obvious.  If the tests complete successfully,
> I will try running the DRM selftest.

The drivers/gpu/drm/i915/selftests/mock_gem_device.c had a new use of
SLAB_DESTROY_BY_RCU, which obviously conflicted with the rename to
SLAB_TYPESAFE_BY_RCU.

It doesn't show up as a merge-time code conflict, only as a build-time
failure. It's why I do allmodconfig builds after every pull. That
doesn't catch everything (I only do it for x86-64, for example), but
it catches a lot.

And no, it's not a problem. These things happen, and it's literally my
job to make sure my merges work out.

I don't actually expect submaintainers to figure things like that out,
although this *did* show up in linux-next, and it's a bit
disappointing how that information got lost somewhere on the way.

It kind of implies that the prep work that linux-next does doesn't get
fully used.

Normally I wouldn't even have mentioned it, if it wasn't for the fact
that I got a 300kB data dump in my mailbox, and that huge amount of
data wasn't actually even very relevant.

                Linus

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

#1639077

On Wed, May 10, 2017 at 01:17:54PM -0700, Linus Torvalds wrote:
> On Wed, May 10, 2017 at 12:54 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > I am testing a merge with current linus/master, and I looked through
> > the commits in -next selected by:
> >
> >         gitk v4.11.. --no-merges --all-match --grep=drm --grep=selftest
> >
> > I didn't find anything obvious.  If the tests complete successfully,
> > I will try running the DRM selftest.
> 
> The drivers/gpu/drm/i915/selftests/mock_gem_device.c had a new use of
> SLAB_DESTROY_BY_RCU, which obviously conflicted with the rename to
> SLAB_TYPESAFE_BY_RCU.
> 
> It doesn't show up as a merge-time code conflict, only as a build-time
> failure. It's why I do allmodconfig builds after every pull. That
> doesn't catch everything (I only do it for x86-64, for example), but
> it catches a lot.
> 
> And no, it's not a problem. These things happen, and it's literally my
> job to make sure my merges work out.
> 
> I don't actually expect submaintainers to figure things like that out,
> although this *did* show up in linux-next, and it's a bit
> disappointing how that information got lost somewhere on the way.
> 
> It kind of implies that the prep work that linux-next does doesn't get
> fully used.

I did see that from linux-next.  For future reference, what should I
have done with it?  Added it to my pull request or to the commit log of
my merge commit?

> Normally I wouldn't even have mentioned it, if it wasn't for the fact
> that I got a 300kB data dump in my mailbox, and that huge amount of
> data wasn't actually even very relevant.

Well, my testing did find a lockdep splat, so the effort was
not wasted.  ;-)

						Thanx, Paul

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

#1639081

On Wed, May 10, 2017 at 1:51 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Wed, May 10, 2017 at 01:17:54PM -0700, Linus Torvalds wrote:
>>
>> It kind of implies that the prep work that linux-next does doesn't get
>> fully used.
>
> I did see that from linux-next.  For future reference, what should I
> have done with it?  Added it to my pull request or to the commit log of
> my merge commit?

Basically, just forward the information along with the description of
what's in the branch, so that I know to expect it.

In this case it didn't actually *matter*, since I noticed it on my
own, but particularly if I'm on the road I don't generally have the
compute power with me to do a full allmodconfig build between each
pull (I usually do one or two a day), so I can miss these things more
easily. And if it happens on other architectures, I wouldn't notice.

It doesn't have to be exhaustive. Just a note saying  that "there's
going to be a semantic merge conflict in file xyz due to abc" means
that I can then specifically take it into account. Even if I were to
be on the road, I can then check that particular driver out and make
sure to check that it builds, etc.

[ Sometimes I also take those kinds of conflict notes into account for
pull scheduling. For example, back when the kids were small, and I
ended up having to occasionally drive them around, I used to aim to do
the simple quick pulls first,  delaying things that might need more
care until I didn't have some driving schedule over my head. That
happens less these days when the kids are off to college and the sole
remaining one mostly drives herself around. ]

                     Linus

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

#1639099

On Wed, May 10, 2017 at 02:08:55PM -0700, Linus Torvalds wrote:
> On Wed, May 10, 2017 at 1:51 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, May 10, 2017 at 01:17:54PM -0700, Linus Torvalds wrote:
> >>
> >> It kind of implies that the prep work that linux-next does doesn't get
> >> fully used.
> >
> > I did see that from linux-next.  For future reference, what should I
> > have done with it?  Added it to my pull request or to the commit log of
> > my merge commit?
> 
> Basically, just forward the information along with the description of
> what's in the branch, so that I know to expect it.
> 
> In this case it didn't actually *matter*, since I noticed it on my
> own, but particularly if I'm on the road I don't generally have the
> compute power with me to do a full allmodconfig build between each
> pull (I usually do one or two a day), so I can miss these things more
> easily. And if it happens on other architectures, I wouldn't notice.
> 
> It doesn't have to be exhaustive. Just a note saying  that "there's
> going to be a semantic merge conflict in file xyz due to abc" means
> that I can then specifically take it into account. Even if I were to
> be on the road, I can then check that particular driver out and make
> sure to check that it builds, etc.

Got it, thank you!

> [ Sometimes I also take those kinds of conflict notes into account for
> pull scheduling. For example, back when the kids were small, and I
> ended up having to occasionally drive them around, I used to aim to do
> the simple quick pulls first,  delaying things that might need more
> care until I didn't have some driving schedule over my head. That
> happens less these days when the kids are off to college and the sole
> remaining one mostly drives herself around. ]

Time does fly -- my youngest graduated from college a couple of years ago.

							Thanx, Paul

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

#1639063

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, May 9, 2017 at 12:26 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > The main changes are:
> 
> So I've pulled it now (although it is showing signs of semantic
> conflicts, so I'll have to look at those), but I've got two requests,
> one for Ingo, one for Paul.
> 
> Ingo: please don't bother sending me stupid crap.
> 
> And by that I mean the whole patch WHEN IT IS 300kB IN SIZE!
> 
> That's just idiotic. Nobody is ever going to review a 300kB patch that
> is ~7500 lines. All it does is waste time, and make it a pain to even
> reply to your emails (since I have "include quoted original" on by
> default in order to be able to quote and reply sanely).

Yeah, you are right and sorry about that - I have removed the patch
generation from my pull request scripts, so it shouldn't happen in
the future.

Thanks,

	Ingo

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

#1639066

On Wed, May 10, 2017 at 12:54 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> Yeah, you are right and sorry about that - I have removed the patch
> generation from my pull request scripts, so it shouldn't happen in
> the future.

I do have to say, that during the later -rc series in particular when
people send me smaller fixes, I enjoy seeing the full patches.

But that's generally when they fit in a screenful or two (eg 100 lines
of actual unified diff or less - which tends to imply that maybe 10-20
lines were actually changed).

So the patches are fine when they are manageable and useful to see
details. It's just that 7500 lines of diff is definitely not
manageable.

            Linus

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

#1639208

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, May 10, 2017 at 12:54 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > Yeah, you are right and sorry about that - I have removed the patch
> > generation from my pull request scripts, so it shouldn't happen in
> > the future.
> 
> I do have to say, that during the later -rc series in particular when
> people send me smaller fixes, I enjoy seeing the full patches.

I find them useful too, and to answer your original question:

> > Nobody is ever going to review a 300kB patch that is ~7500 lines.

I _did_ skim over that 300K patch, because I always try to do a final manual check 
on the raw diffs I'm sending to you, and also to make it very clear what was sent 
from a full disclosure and security log POV, independent of the Git pull space. 

When patches are way too long, for example as the perf pull request diffs often 
are, I trim them, so it's never an absolute, script-only thing.

Still it's not an excuse:

 - I doubt anyone else but me would skim over a 30K (let alone a 300K) patch,

 - I also missed the pain large patches cause in Gmail replies (with Mutt that
   pain is considerably less),

 - plus, most importantly, I didn't notice that the extra RCU mode bloat was one 
   too many in an already sizable line-up of RCU complexity ...

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web