Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1223064 > unrolled thread
| Started by | Chris Mason <clm@fb.com> |
|---|---|
| First post | 2015-09-11 21:40 +0200 |
| Last post | 2015-09-12 01:20 +0200 |
| Articles | 20 on this page of 50 — 8 participants |
Back to article view | Back to linux.kernel
[PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-11 21:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-11 22:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-11 22:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Josef Bacik <jbacik@fb.com> - 2015-09-11 22:50 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-11 23:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-12 00:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-12 01:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-12 01:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-12 03:00 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-12 04:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-12 04:30 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-13 01:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-13 01:30 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-13 01:50 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-13 15:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-14 01:00 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-14 01:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-14 22:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Jan Kara <jack@suse.cz> - 2015-09-16 22:00 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-16 22:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-17 00:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-17 02:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-17 03:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-17 04:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-17 21:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-18 00:50 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 01:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-18 02:00 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-18 02:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 04:00 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-18 07:50 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 08:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 08:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Jens Axboe <axboe@fb.com> - 2015-09-18 16:30 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-18 15:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Jens Axboe <axboe@fb.com> - 2015-09-18 16:30 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 17:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Peter Zijlstra <peterz@infradead.org> - 2015-09-18 18:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Peter Zijlstra <peterz@infradead.org> - 2015-09-18 18:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 18:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-19 01:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Jan Kara <jack@suse.cz> - 2015-09-21 11:30 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Andrew Morton <akpm@linux-foundation.org> - 2015-09-21 22:30 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-18 01:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 01:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-17 05:50 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-17 06:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-17 14:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-12 01:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-12 01:20 +0200
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2015-09-17 00:20 +0200 |
| Message-ID | <q9tdw-M9-15@gated-at.bofh.it> |
| In reply to | #1226437 |
On Wed, Sep 16, 2015 at 04:00:12PM -0400, Chris Mason wrote: > On Wed, Sep 16, 2015 at 09:58:06PM +0200, Jan Kara wrote: > > On Wed 16-09-15 11:16:21, Chris Mason wrote: > > > Short version, Linus' patch still gives bigger IOs and similar perf to > > > Dave's original. I should have done the blktrace runs for 60 seconds > > > instead of 30, I suspect that would even out the average sizes between > > > the three patches. > > > > Thanks for the data Chris. So I guess we are fine with what's currently in, > > right? > > Looks like it works well to me. Graph looks good, though I'll confirm it on my test rig once I get out from under the pile of email and other stuff that is queued up after being away for a week... Cheers, Dave. -- Dave Chinner david@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2015-09-17 02:40 +0200 |
| Message-ID | <q9voZ-3Qg-7@gated-at.bofh.it> |
| In reply to | #1226539 |
[cc Tejun]
On Thu, Sep 17, 2015 at 08:07:04AM +1000, Dave Chinner wrote:
> On Wed, Sep 16, 2015 at 04:00:12PM -0400, Chris Mason wrote:
> > On Wed, Sep 16, 2015 at 09:58:06PM +0200, Jan Kara wrote:
> > > On Wed 16-09-15 11:16:21, Chris Mason wrote:
> > > > Short version, Linus' patch still gives bigger IOs and similar perf to
> > > > Dave's original. I should have done the blktrace runs for 60 seconds
> > > > instead of 30, I suspect that would even out the average sizes between
> > > > the three patches.
> > >
> > > Thanks for the data Chris. So I guess we are fine with what's currently in,
> > > right?
> >
> > Looks like it works well to me.
>
> Graph looks good, though I'll confirm it on my test rig once I get
> out from under the pile of email and other stuff that is queued up
> after being away for a week...
I ran some tests in the background while reading other email.....
TL;DR: Results look really bad - not only is the plugging
problematic, baseline writeback performance has regressed
significantly. We need to revert the plugging changes until the
underlying writeback performance regressions are sorted out.
In more detail, these tests were run on my usual 16p/16GB RAM
performance test VM with storage set up as described here:
http://permalink.gmane.org/gmane.linux.kernel/1768786
The test:
$ ~/tests/fsmark-10-4-test-xfs.sh
meta-data=/dev/vdc isize=512 agcount=500, agsize=268435455 blks
= sectsz=512 attr=2, projid32bit=1
= crc=1 finobt=1, sparse=0
data = bsize=4096 blocks=134217727500, imaxpct=1
= sunit=0 swidth=0 blks
naming =version 2 bsize=4096 ascii-ci=0 ftype=1
log =internal log bsize=4096 blocks=131072, version=2
= sectsz=512 sunit=1 blks, lazy-count=1
realtime =none extsz=4096 blocks=0, rtextents=0
# ./fs_mark -D 10000 -S0 -n 10000 -s 4096 -L 120 -d /mnt/scratch/0 -d /mnt/scratch/1 -d /mnt/scratch/2 -d /mnt/scratch/3 -d /mnt/scratch/4 -d /mnt/scratch/5 -d /mnt/scratch/6 -d /mnt/scratch/7
# Version 3.3, 8 thread(s) starting at Thu Sep 17 08:08:36 2015
# Sync method: NO SYNC: Test does not issue sync() or fsync() calls.
# Directories: Time based hash between directories across 10000 subdirectories with 180 seconds per subdirectory.
# File names: 40 bytes long, (16 initial bytes of time stamp with 24 random bytes at end of name)
# Files info: size 4096 bytes, written with an IO size of 16384 bytes per write
# App overhead is time in microseconds spent in the test not doing file writing related system calls.
FSUse% Count Size Files/sec App Overhead
0 80000 4096 106938.0 543310
0 160000 4096 102922.7 476362
0 240000 4096 107182.9 538206
0 320000 4096 107871.7 619821
0 400000 4096 99255.6 622021
0 480000 4096 103217.8 609943
0 560000 4096 96544.2 640988
0 640000 4096 100347.3 676237
0 720000 4096 87534.8 483495
0 800000 4096 72577.5 2556920
0 880000 4096 97569.0 646996
<RAM fills here, sustained performance is now dependent on writeback>
0 960000 4096 80147.0 515679
0 1040000 4096 100394.2 816979
0 1120000 4096 91466.5 739009
0 1200000 4096 85868.1 977506
0 1280000 4096 89691.5 715207
0 1360000 4096 52547.5 712810
0 1440000 4096 47999.1 685282
0 1520000 4096 47894.3 697261
0 1600000 4096 47549.4 789977
0 1680000 4096 40029.2 677885
0 1760000 4096 16637.4 12804557
0 1840000 4096 16883.6 24295975
0 1920000 4096 18852.1 22264137
0 2000000 4096 27107.3 10310408
0 2080000 4096 23654.1 19823567
0 2160000 4096 24588.9 19769484
0 2240000 4096 13959.3 40168163
0 2320000 4096 26239.2 17806813
0 2400000 4096 17858.5 24596715
0 2480000 4096 12072.9 38177650
0 2560000 4096 9989.7 43413729
<This is Real Bad. It gets significantly worse as the test goes on:>
0 6880000 4096 11773.4 16470400
0 6960000 4096 5556.5 27325343
0 7040000 4096 13816.6 22440991
0 7120000 4096 7322.4 42086686
0 7200000 4096 8033.0 31182907
0 7280000 4096 10589.4 30876729
0 7360000 4096 15844.5 19903105
0 7440000 4096 9187.7 35041718
0 7520000 4096 11800.9 12254920
0 7600000 4096 20049.9 13052020
0 7680000 4096 17444.9 12055262
0 7760000 4096 25900.6 10633885
0 7840000 4096 6398.8 12014901
0 7920000 4096 12147.4 14655678
<...>
0 9360000 4096 19406.0 5874193
0 9440000 4096 7261.3 14542806
0 9520000 4096 14978.7 21921186
0 9600000 4096 12656.7 10539894
real 10m9.240s
user 0m51.817s
sys 13m23.874s
A quick reading of the IO behaviour shows an average of about 1000
IOPS and 100MB/s, which matches roughly with the average files/s
created. There's no obvious peaks and gaps in the IO throughput, but
CPU usage has regular gaps where it falls to nothing and the only
"usage" is IO wait time.
Reverting just Linus' patch, so no plugging at all in the code
(i.e. the baseline numbers):
FSUse% Count Size Files/sec App Overhead
0 80000 4096 109017.5 586268
0 160000 4096 93548.1 553333
0 240000 4096 104141.7 817182
0 320000 4096 112846.2 597494
0 400000 4096 98628.6 641524
0 480000 4096 86586.1 1274195
0 560000 4096 95965.0 776961
0 640000 4096 84585.9 583330
0 720000 4096 93787.1 695658
0 800000 4096 94112.7 645925
0 880000 4096 92259.2 792950
<RAM fills here, sustained performance is now dependent on writeback>
0 960000 4096 62936.9 763642
0 1040000 4096 22484.8 737471
0 1120000 4096 22038.6 706791
0 1200000 4096 24096.5 680878
0 1280000 4096 23973.1 839826
0 1360000 4096 24761.9 779486
0 1440000 4096 25467.7 976022
0 1520000 4096 25338.7 935552
0 1600000 4096 27484.8 683351
0 1680000 4096 24383.2 3039781
0 1760000 4096 24857.0 2657008
<maintains 13-26k files/s all the way to the end .... >
0 9040000 4096 21438.6 4869317
0 9120000 4096 25387.1 642198
0 9200000 4096 26815.9 727763
0 9280000 4096 25175.5 4458811
0 9360000 4096 19832.8 982122
0 9440000 4096 17643.8 805599
0 9520000 4096 13923.4 7819152
0 9600000 4096 18101.3 1071163
real 7m7.926s
user 0m57.976s
sys 14m14.620s
The baseline of no plugging is a full 3 minutes faster than the
plugging behaviour of Linus' patch. The IO behaviour demonstrates
that, sustaining between 25-30,000 IOPS and throughput of
130-150MB/s. Hence, while Linus' patch does change the IO patterns,
it does not result in a performance improvement like the original
plugging patch did.
So I went back and had a look at my original patch, which I've been
using locally for a couple of years and was similar to the original
commit. It has this description from when I last updated the perf
numbers from testing done on 3.17:
| Test VM: 16p, 16GB RAM, 2xSSD in RAID0, 500TB sparse XFS filesystem,
| metadata CRCs enabled.
|
| Test:
|
| $ ./fs_mark -D 10000 -S0 -n 10000 -s 4096 -L 120 -d
| /mnt/scratch/0 -d /mnt/scratch/1 -d /mnt/scratch/2 -d
| /mnt/scratch/3 -d /mnt/scratch/4 -d /mnt/scratch/5 -d
| /mnt/scratch/6 -d /mnt/scratch/7
|
| Result:
| wall sys create rate Physical write IO
| time CPU (avg files/s) IOPS Bandwidth
| ----- ----- ------------- ------ ---------
| unpatched 5m54s 15m32s 32,500+/-2200 28,000 150MB/s
| patched 3m19s 13m28s 52,900+/-1800 1,500 280MB/s
| improvement -43.8% -13.3% +62.7% -94.6% +86.6%
IOWs, what we are seeing here is that the baseline writeback
performance has regressed quite significantly since I took these
numbers back on 3.17. I'm running on exactly the same test setup;
the only difference is the kernel and so the current kernel baseline
is ~20% slower than the baseline numbers I have in my patch.
So, let's put the original [broken] plugging back in:
.....
0 8960000 4096 28420.0 2129129
0 9040000 4096 26863.0 6893094
0 9120000 4096 46721.4 705927
0 9200000 4096 46384.0 1081627
0 9280000 4096 39692.3 2148938
0 9360000 4096 41976.0 3540489
0 9440000 4096 28025.1 10364440
0 9520000 4096 38580.2 2457019
0 9600000 4096 33526.1 7661316
real 5m12.114s
user 0m50.294s
sys 12m54.425s
While we get back that regression and then some more, it's still
much slower than the high level plugging on slightly older kernels.
And the performance is, again, highly variable through the midpoint
of the test run. e.g:
....
0 4720000 4096 39224.4 9401401
0 4800000 4096 25342.9 18889665
0 4880000 4096 14841.7 36668633
0 4960000 4096 27624.6 17731873
0 5040000 4096 37874.8 11390831
0 5120000 4096 29009.2 12331965
0 5200000 4096 49568.0 4346746
0 5280000 4096 83074.5 877142
0 5360000 4096 32498.8 5401470
0 5440000 4096 55468.1 3188117
0 5520000 4096 19156.0 4112120
0 5600000 4096 44894.0 3186559
....
We have a low of ~15k files/s and a high of 83k files/s whilst
averaging about 38k files/s.
IOWs, ignoring the plugging, the recent changes to the writeback
code have really screwed up writeback performance and changed the
way it behaves from a throughput consistency point of view. I'd
suggest we should start by reverting all the plugging changes for
now, because there are other problems these plugging changes have
exposed in the current writeback code that need to be fixed first...
Cheers
Dave.
PS: I was going to run this on an ext4 filesystem to check it
wasn't just XFS seeing this, but that reminded me why I don't run
this test on ext4:
FSUse% Count Size Files/sec App Overhead
5 80000 4096 211668.3 534504
5 160000 4096 191731.8 532701
5 240000 4096 196251.2 489772
5 320000 4096 207597.1 464032
5 400000 4096 121616.0 601156
5 480000 4096 197729.9 573925
<Really, really fast in memory, but background writeback starts here>
5 560000 4096 128698.4 546362
5 640000 4096 137904.3 543718
5 720000 4096 63680.9 498110
5 800000 4096 25825.9 1538213
5 880000 4096 32708.7 632235
<dirty page threshold (20% RAM) is hit here. performance sucks from
here on in (still 10GB of free RAM) because the writeback IO
pattern is a random 4k write pattern>
5 960000 4096 6940.2 7250666
5 1040000 4096 7474.8 711872
5 1120000 4096 4127.5 14034167
5 1200000 4096 4822.1 752398
....
Hence it takes about around hour to do a single run of this test on
ext4 because it's delayed allocation algorithm does not linearise
writes to small files with similar inode locality like XFS does, and
that's the behaviour that high level plugging really helps. IOws,
this specific workload does not demonstrate any significant
performance difference on ext4 with the different plugging
algorithms because it does random IO rather than sequential IO.
I also ran it on btrfs, but it's pushing peaks of 15-25,000 IOPS
every 30s regardless of whether there is plugging at a high level or
not, so again btrfs behaviour is pretty much unaffected by the high
level plugging on this workload....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-17 03:20 +0200 |
| Message-ID | <q9w1H-4OT-5@gated-at.bofh.it> |
| In reply to | #1226603 |
On Wed, Sep 16, 2015 at 5:37 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> TL;DR: Results look really bad - not only is the plugging
> problematic, baseline writeback performance has regressed
> significantly.
Dave, if you're testing my current -git, the other performance issue
might still be the spinlock thing.
I haven't gotten the -tip pull fox the spinlock regression yet, and
since you were the only one who noticed, I didn't consider it critical
enough to bypass the normal schedule, so -rc1 still has the crazy-bad
test-and-set loop for spinlocks.
So it's waiting in the locking fix branch (and is cc'd for stable),
but hasn't reached me yet.
If you want to test that theory out, you can make virt_spin_lock()
just unconditionally return false to disable the bad cmpxchg loop for
now.
The plugging IO pauses are interesting, though. Plugging really
*shouldn't* cause that kind of pauses, _regardless_ of what level it
happens on, so I wonder if the patch ends up just exposing some really
basic problem that just normally goes hidden.
Can you match up the IO wait times with just *where* it is waiting? Is
it waiting for that inode I_SYNC thing in inode_sleep_on_writeback()?
But it does sound like we should just revert the whole plugging for
now, if only because "it has odd effects".
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2015-09-17 04:20 +0200 |
| Message-ID | <q9wXL-6hr-5@gated-at.bofh.it> |
| In reply to | #1226610 |
On Wed, Sep 16, 2015 at 06:12:29PM -0700, Linus Torvalds wrote: > On Wed, Sep 16, 2015 at 5:37 PM, Dave Chinner <david@fromorbit.com> wrote: > > > > TL;DR: Results look really bad - not only is the plugging > > problematic, baseline writeback performance has regressed > > significantly. > > Dave, if you're testing my current -git, the other performance issue > might still be the spinlock thing. I have the fix as the first commit in my local tree - it'll remain there until I get a conflict after an update. :) > The plugging IO pauses are interesting, though. Plugging really > *shouldn't* cause that kind of pauses, _regardless_ of what level it > happens on, so I wonder if the patch ends up just exposing some really > basic problem that just normally goes hidden. Right, that's what I suspect - it didn't happen on older kernels, but we've just completely reworked the writeback code for the control group awareness since I last looked really closely at this... > Can you match up the IO wait times with just *where* it is > waiting? Is it waiting for that inode I_SYNC thing in > inode_sleep_on_writeback()? I'll do some more investigation. > But it does sound like we should just revert the whole plugging for > now, if only because "it has odd effects". Yup - we can add it again next merge window once we get to the bottom of whatever is going on and have had time to test the new code properly. Cheers, Dave. -- Dave Chinner david@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-17 21:40 +0200 |
| Message-ID | <q9Nce-4WP-23@gated-at.bofh.it> |
| In reply to | #1226629 |
On Wed, Sep 16, 2015 at 7:14 PM, Dave Chinner <david@fromorbit.com> wrote:
>>
>> Dave, if you're testing my current -git, the other performance issue
>> might still be the spinlock thing.
>
> I have the fix as the first commit in my local tree - it'll remain
> there until I get a conflict after an update. :)
Ok. I'm happy to report that you should get a conflict now, and that
the spinlock code should work well for your virtualized case again.
No updates on the plugging thing yet, I'll wait a bit and follow this
thread and see if somebody comes up with any explanations or theories
in the hope that we might not need to revert (or at least have a more
targeted change).
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Chris Mason <clm@fb.com> |
|---|---|
| Date | 2015-09-18 00:50 +0200 |
| Message-ID | <q9Qa5-OT-11@gated-at.bofh.it> |
| In reply to | #1227366 |
On Thu, Sep 17, 2015 at 12:39:51PM -0700, Linus Torvalds wrote:
> On Wed, Sep 16, 2015 at 7:14 PM, Dave Chinner <david@fromorbit.com> wrote:
> >>
> >> Dave, if you're testing my current -git, the other performance issue
> >> might still be the spinlock thing.
> >
> > I have the fix as the first commit in my local tree - it'll remain
> > there until I get a conflict after an update. :)
>
> Ok. I'm happy to report that you should get a conflict now, and that
> the spinlock code should work well for your virtualized case again.
>
> No updates on the plugging thing yet, I'll wait a bit and follow this
> thread and see if somebody comes up with any explanations or theories
> in the hope that we might not need to revert (or at least have a more
> targeted change).
Playing around with the plug a little, most of the unplugs are coming
from the cond_resched_lock(). Not really sure why we are doing the
cond_resched() there, we should be doing it before we retake the lock
instead.
This patch takes my box (with dirty thresholds at 1.5GB/3GB) from 195K
files/sec up to 213K. Average IO size is the same as 4.3-rc1.
It probably won't help Dave, since most of his unplugs should have been
from the cond_resched_locked() too.
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 587ac08..05ed541 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1481,6 +1481,19 @@ static long writeback_sb_inodes(struct super_block *sb,
wbc_detach_inode(&wbc);
work->nr_pages -= write_chunk - wbc.nr_to_write;
wrote += write_chunk - wbc.nr_to_write;
+
+ if (need_resched()) {
+ /*
+ * we're plugged and don't want to hand off to kblockd
+ * for the actual unplug work. But we do want to
+ * reschedule. So flush our plug and then
+ * schedule away
+ */
+ blk_flush_plug(current);
+ cond_resched();
+ }
+
+
spin_lock(&wb->list_lock);
spin_lock(&inode->i_lock);
if (!(inode->i_state & I_DIRTY_ALL))
@@ -1488,7 +1501,7 @@ static long writeback_sb_inodes(struct super_block *sb,
requeue_inode(inode, wb, &wbc);
inode_sync_complete(inode);
spin_unlock(&inode->i_lock);
- cond_resched_lock(&wb->list_lock);
+
/*
* bail out to wb_writeback() often enough to check
* background threshold and other termination conditions.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-18 01:10 +0200 |
| Message-ID | <q9Qts-1ra-9@gated-at.bofh.it> |
| In reply to | #1227467 |
On Thu, Sep 17, 2015 at 3:42 PM, Chris Mason <clm@fb.com> wrote:
>
> Playing around with the plug a little, most of the unplugs are coming
> from the cond_resched_lock(). Not really sure why we are doing the
> cond_resched() there, we should be doing it before we retake the lock
> instead.
>
> This patch takes my box (with dirty thresholds at 1.5GB/3GB) from 195K
> files/sec up to 213K. Average IO size is the same as 4.3-rc1.
Ok, so at least for you, part of the problem really ends up being that
there's a mix of the "synchronous" unplugging (by the actual explicit
"blk_finish_plug(&plug);") and the writeback that is handed off to
kblockd_workqueue.
I'm not seeing why that should be an issue. Sure, there's some CPU
overhead to context switching, but I don't see that it should be that
big of a deal.
I wonder if there is something more serious wrong with the kblockd_workqueue.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Chris Mason <clm@fb.com> |
|---|---|
| Date | 2015-09-18 02:00 +0200 |
| Message-ID | <q9RfQ-2nh-17@gated-at.bofh.it> |
| In reply to | #1227476 |
On Thu, Sep 17, 2015 at 04:08:19PM -0700, Linus Torvalds wrote: > On Thu, Sep 17, 2015 at 3:42 PM, Chris Mason <clm@fb.com> wrote: > > > > Playing around with the plug a little, most of the unplugs are coming > > from the cond_resched_lock(). Not really sure why we are doing the > > cond_resched() there, we should be doing it before we retake the lock > > instead. > > > > This patch takes my box (with dirty thresholds at 1.5GB/3GB) from 195K > > files/sec up to 213K. Average IO size is the same as 4.3-rc1. > > Ok, so at least for you, part of the problem really ends up being that > there's a mix of the "synchronous" unplugging (by the actual explicit > "blk_finish_plug(&plug);") and the writeback that is handed off to > kblockd_workqueue. > > I'm not seeing why that should be an issue. Sure, there's some CPU > overhead to context switching, but I don't see that it should be that > big of a deal. > > I wonder if there is something more serious wrong with the kblockd_workqueue. I'm driving the box pretty hard, it's right on the line between CPU bound and IO bound. So I've got 32 fs_mark processes banging away and 32 CPUs (16 really, with hyperthreading). They are popping in and out of balance_dirty_pages() so I have high CPU utilization alternating with high IO wait times. There no reads at all, so all of these waits are for buffered writes. People in balance_dirty_pages are indirectly waiting on the unplug, so maybe the context switch overhead on a loaded box is enough to explain it. We've definitely gotten more than 9% by inlining small synchronous items in btrfs in the past, but those were more explicitly synchronous. I know it's painfully hand wavy. I don't see any other users of the kblockd workqueues, and the perf profiles don't jump out at me. I'll feel better about the patch if Dave confirms any gains. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2015-09-18 02:40 +0200 |
| Message-ID | <q9RSx-3n6-1@gated-at.bofh.it> |
| In reply to | #1227492 |
On Thu, Sep 17, 2015 at 07:56:47PM -0400, Chris Mason wrote: > On Thu, Sep 17, 2015 at 04:08:19PM -0700, Linus Torvalds wrote: > > On Thu, Sep 17, 2015 at 3:42 PM, Chris Mason <clm@fb.com> wrote: > > > > > > Playing around with the plug a little, most of the unplugs are coming > > > from the cond_resched_lock(). Not really sure why we are doing the > > > cond_resched() there, we should be doing it before we retake the lock > > > instead. > > > > > > This patch takes my box (with dirty thresholds at 1.5GB/3GB) from 195K > > > files/sec up to 213K. Average IO size is the same as 4.3-rc1. > > > > Ok, so at least for you, part of the problem really ends up being that > > there's a mix of the "synchronous" unplugging (by the actual explicit > > "blk_finish_plug(&plug);") and the writeback that is handed off to > > kblockd_workqueue. > > > > I'm not seeing why that should be an issue. Sure, there's some CPU > > overhead to context switching, but I don't see that it should be that > > big of a deal. It may well change the dispatch order of enough IOs for it to be significant on an IO bound device. > > I wonder if there is something more serious wrong with the kblockd_workqueue. > > I'm driving the box pretty hard, it's right on the line between CPU > bound and IO bound. So I've got 32 fs_mark processes banging away and > 32 CPUs (16 really, with hyperthreading). I'm only using 8 threads right now, so I have ~6-7 idle CPUs on this workload. Hence if it's CPU load related, I probably won't see any change in behaviour. > They are popping in and out of balance_dirty_pages() so I have high CPU > utilization alternating with high IO wait times. There no reads at all, > so all of these waits are for buffered writes. > > People in balance_dirty_pages are indirectly waiting on the unplug, so > maybe the context switch overhead on a loaded box is enough to explain > it. We've definitely gotten more than 9% by inlining small synchronous > items in btrfs in the past, but those were more explicitly synchronous. > > I know it's painfully hand wavy. I don't see any other users of the > kblockd workqueues, and the perf profiles don't jump out at me. I'll > feel better about the patch if Dave confirms any gains. In outright performance on my test machine, the difference in files/s is noise. However, the consistency looks to be substantially improved and the context switch rate is now running at under 3,000/sec. Numbers, including the std deviation of the files/s number output during the fsmark run (averaged across 3 separate benahmark runs): files/s std-dev wall time 4.3-rc1-noplug 34400 2.0e04 5m25s 4.3-rc1 56600 2.3e04 3m23s 4.3-rc1-flush 56079 1.4e04 3m14s std-dev is well down, and the improvement in wall time is large enough to be significant. Looks good to me. Cheers, Dave. -- Dave Chinner david@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-18 04:00 +0200 |
| Message-ID | <q9T7X-55L-1@gated-at.bofh.it> |
| In reply to | #1227499 |
[Multipart message — attachments visible in raw view] — view raw
On Thu, Sep 17, 2015 at 5:37 PM, Dave Chinner <david@fromorbit.com> wrote:
>> >
>> > I'm not seeing why that should be an issue. Sure, there's some CPU
>> > overhead to context switching, but I don't see that it should be that
>> > big of a deal.
>
> It may well change the dispatch order of enough IOs for it to be
> significant on an IO bound device.
Hmm. Maybe. We obviously try to order the IO's a bit by inode, and I
could see the use of a workqueue maybe changing that sufficiently. But
it still sounds a bit unlikely.
And in fact, I think I have a better explanation.
> In outright performance on my test machine, the difference in
> files/s is noise. However, the consistency looks to be substantially
> improved and the context switch rate is now running at under
> 3,000/sec.
Hmm. I don't recall seeing you mention how many context switches per
second you had before. What is it down from?
However, I think I may have found something more interesting here.
The fact is that a *normal* schedule will trigger that whole
blk_schedule_flush_plug(), but a cond_sched() or a cond_sched_lock()
doesn't actually do a normal schedule at all. Those trigger a
*preemption* schedule.
And a preemption schedule does not trigger that unplugging at all.
Why? A kernel "preemption" very much tries to avoid touching thread
state, because the whole point is that normally we may be preempting
threads in random places, so we don't run things like
sched_submit_work(), because the thread may be in the middle of
*creating* that work, and we don't want to introduce races. The
preemption scheduling can also be done with "task->state" set to
sleeping, and it won't actually sleep.
Now, for the explicit schedules like "cond_resched()" and
"cond_resched_lock()", those races with obviously don't exist, but
they happen to share the same preemption scheduling logic.
So it turns out that as far as I can see, the whole "cond_resched()"
will not start any IO at all, and it will just be left on the thread
plug until we schedule back to the thread.
So I don't think this has anything to do with kblockd_workqueue. I
don't think it even gets to that point.
I may be missing something, but just to humor me, can you test the
attached patch *without* Chris's patch to do explicit plugging? This
should make cond_resched() and cond_resched_lock() run the unplugging.
It may be entirely broken, I haven't thought this entirely through.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2015-09-18 07:50 +0200 |
| Message-ID | <q9WIy-1Wn-11@gated-at.bofh.it> |
| In reply to | #1227522 |
On Thu, Sep 17, 2015 at 06:50:29PM -0700, Linus Torvalds wrote:
> On Thu, Sep 17, 2015 at 5:37 PM, Dave Chinner <david@fromorbit.com> wrote:
> >> >
> >> > I'm not seeing why that should be an issue. Sure, there's some CPU
> >> > overhead to context switching, but I don't see that it should be that
> >> > big of a deal.
> >
> > It may well change the dispatch order of enough IOs for it to be
> > significant on an IO bound device.
>
> Hmm. Maybe. We obviously try to order the IO's a bit by inode, and I
> could see the use of a workqueue maybe changing that sufficiently. But
> it still sounds a bit unlikely.
>
> And in fact, I think I have a better explanation.
>
> > In outright performance on my test machine, the difference in
> > files/s is noise. However, the consistency looks to be substantially
> > improved and the context switch rate is now running at under
> > 3,000/sec.
>
> Hmm. I don't recall seeing you mention how many context switches per
> second you had before. What is it down from?
Around 4000-4500/sec, so there's not a huge amount of context
switches going on. It's not like btrfs, where this workload
generates over 250,000 context switches/sec....
> However, I think I may have found something more interesting here.
>
> The fact is that a *normal* schedule will trigger that whole
> blk_schedule_flush_plug(), but a cond_sched() or a cond_sched_lock()
> doesn't actually do a normal schedule at all. Those trigger a
> *preemption* schedule.
Ok, makes sense - the plug is not being flushed as we switch away,
but Chris' patch makes it do that.
> So it turns out that as far as I can see, the whole "cond_resched()"
> will not start any IO at all, and it will just be left on the thread
> plug until we schedule back to the thread.
>
> So I don't think this has anything to do with kblockd_workqueue. I
> don't think it even gets to that point.
>
> I may be missing something, but just to humor me, can you test the
> attached patch *without* Chris's patch to do explicit plugging? This
> should make cond_resched() and cond_resched_lock() run the unplugging.
Context switches go back to the 4-4500/sec range. Otherwise
behaviour and performance is indistinguishable from Chris' patch.
Cheers,
Dave.
PS: just hit another "did this just get broken in 4.3-rc1" issue - I
can't run blktrace while there's a IO load because:
$ sudo blktrace -d /dev/vdc
BLKTRACESETUP(2) /dev/vdc failed: 5/Input/output error
Thread 1 failed open /sys/kernel/debug/block/(null)/trace1: 2/No such file or directory
....
[ 641.424618] blktrace: page allocation failure: order:5, mode:0x2040d0
[ 641.425952] CPU: 13 PID: 11163 Comm: blktrace Not tainted 4.3.0-rc1-dgc+ #430
[ 641.427321] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Debian-1.8.2-1 04/01/2014
[ 641.429066] 0000000000000005 ffff8800b731faa0 ffffffff81785609 00000000002040d0
[ 641.430585] ffff8800b731fb28 ffffffff81183688 0000000000000000 ffffffff824926f8
[ 641.432143] ffff8800b731faf0 0000000000000042 0000000000000010 0000000000000100
[ 641.433695] Call Trace:
[ 641.434196] [<ffffffff81785609>] dump_stack+0x4b/0x72
[ 641.435219] [<ffffffff81183688>] warn_alloc_failed+0xd8/0x130
[ 641.436402] [<ffffffff811863bb>] __alloc_pages_nodemask+0x66b/0x7d0
[ 641.437684] [<ffffffff811c0e19>] cache_grow.constprop.61+0xc9/0x2d0
[ 641.438933] [<ffffffff811c1569>] kmem_cache_alloc_trace+0x129/0x400
[ 641.440240] [<ffffffff811424f8>] relay_open+0x68/0x2c0
[ 641.441299] [<ffffffff8115deb1>] do_blk_trace_setup+0x191/0x2d0
[ 641.442492] [<ffffffff8115e041>] blk_trace_setup+0x51/0x90
[ 641.443586] [<ffffffff8115e229>] blk_trace_ioctl+0xb9/0x110
[ 641.444702] [<ffffffff811e586c>] ? mntput_no_expire+0x2c/0x1a0
[ 641.446419] [<ffffffff8176fa58>] blkdev_ioctl+0x528/0x690
[ 641.447489] [<ffffffff811fd37d>] block_ioctl+0x3d/0x50
[ 641.449194] [<ffffffff811d93da>] do_vfs_ioctl+0x2ba/0x490
[ 641.450491] [<ffffffff811d9629>] SyS_ioctl+0x79/0x90
[ 641.451575] [<ffffffff81d800ae>] entry_SYSCALL_64_fastpath+0x12/0x71
[ 641.453231] Mem-Info:
[ 641.453714] active_anon:11651 inactive_anon:2411 isolated_anon:0
active_file:8456 inactive_file:2070698 isolated_file:0
unevictable:1011 dirty:374605 writeback:1176 unstable:0
slab_reclaimable:1514989 slab_unreclaimable:233692
mapped:9332 shmem:2250 pagetables:1891 bounce:0
free:24305 free_pcp:1448 free_cma:0
[ 641.461950] DMA free:15908kB min:12kB low:12kB high:16kB active_anon:0kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:15992kB managed:15908kB mlocked:0kB dirty:0kB writeback:0kB mapped:0kB shmem:0kB slab_reclaimable:0kB slab_unreclaimable:0kB kernel_stack:0kB pagetables:0kB unstable:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? yes
[ 641.470017] lowmem_reserve[]: 0 2960 16005 16005
[ 641.471041] DMA32 free:57076kB min:2992kB low:3740kB high:4488kB active_anon:8692kB inactive_anon:2572kB active_file:6224kB inactive_file:1507352kB unevictable:1004kB isolated(anon):0kB isolated(file):0kB present:3129212kB managed:3032864kB mlocked:1004kB dirty:275336kB writeback:896kB mapped:6800kB shmem:1836kB slab_reclaimable:1086292kB slab_unreclaimable:183168kB kernel_stack:23600kB pagetables:1372kB unstable:0kB bounce:0kB free_pcp:3440kB local_pcp:16kB free_cma:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[ 641.479677] lowmem_reserve[]: 0 0 13044 13044
[ 641.480888] Normal free:18040kB min:13188kB low:16484kB high:19780kB active_anon:38264kB inactive_anon:7072kB active_file:27600kB inactive_file:6775516kB unevictable:3040kB isolated(anon):0kB isolated(file):0kB present:13631488kB managed:13357908kB mlocked:3040kB dirty:1223624kB writeback:5144kB mapped:30528kB shmem:7164kB slab_reclaimable:4975584kB slab_unreclaimable:755076kB kernel_stack:44912kB pagetables:6192kB unstable:0kB bounce:0kB free_pcp:3432kB local_pcp:88kB free_cma:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[ 641.492392] lowmem_reserve[]: 0 0 0 0
[ 641.493170] DMA: 1*4kB (U) 0*8kB 0*16kB 1*32kB (U) 2*64kB (U) 1*128kB (U) 1*256kB (U) 0*512kB 1*1024kB (U) 1*2048kB (M) 3*4096kB (M) = 15908kB
[ 641.496035] DMA32: 6584*4kB (UEM) 3783*8kB (UM) 17*16kB (M) 9*32kB (M) 3*64kB (M) 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 57352kB
[ 641.498705] Normal: 3372*4kB (UE) 393*8kB (UEM) 37*16kB (UEM) 33*32kB (UEM) 6*64kB (EM) 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 18664kB
[ 641.501479] Node 0 hugepages_total=0 hugepages_free=0 hugepages_surp=0 hugepages_size=2048kB
[ 641.503019] 2082056 total pagecache pages
[ 641.503759] 0 pages in swap cache
[ 641.504403] Swap cache stats: add 0, delete 0, find 0/0
[ 641.505374] Free swap = 497976kB
[ 641.506002] Total swap = 497976kB
[ 641.506622] 4194173 pages RAM
[ 641.507171] 0 pages HighMem/MovableOnly
[ 641.507878] 92503 pages reserved
This is from:
gdb) l *(relay_open+0x68)
0xffffffff811424f8 is in relay_open (kernel/relay.c:582).
577 return NULL;
578 if (subbuf_size > UINT_MAX / n_subbufs)
579 return NULL;
580
581 chan = kzalloc(sizeof(struct rchan), GFP_KERNEL);
582 if (!chan)
583 return NULL;
584
585 chan->version = RELAYFS_CHANNEL_VERSION;
586 chan->n_subbufs = n_subbufs;
and struct rchan has a member struct rchan_buf *buf[NR_CPUS];
and CONFIG_NR_CPUS=8192, hence the attempt at an order 5 allocation
that fails here....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-18 08:10 +0200 |
| Message-ID | <q9X1T-2y9-3@gated-at.bofh.it> |
| In reply to | #1227572 |
On Thu, Sep 17, 2015 at 10:40 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> Ok, makes sense - the plug is not being flushed as we switch away,
> but Chris' patch makes it do that.
Yup.
And I actually think Chris' patch is better than the one I sent out
(but maybe the scheduler people should take a look at the behavior of
cond_resched()), I just wanted you to test that to verify the
behavior.
The fact that Chris' patch ends up lowering the context switches
(because it does the unplugging directly) is also an argument for his
approach.
I just wanted to understand the oddity with kblockd_workqueue. And I
think that's solved.
> Context switches go back to the 4-4500/sec range. Otherwise
> behaviour and performance is indistinguishable from Chris' patch.
.. this was exactly what I wanted to hear. So it sounds like we have
no odd unexplained behavior left in this area.
Which is not to say that there wouldn't be room for improvement, but
it just makes me much happier about the state of these patches to feel
like we understand what was going on.
> PS: just hit another "did this just get broken in 4.3-rc1" issue - I
> can't run blktrace while there's a IO load because:
>
> $ sudo blktrace -d /dev/vdc
> BLKTRACESETUP(2) /dev/vdc failed: 5/Input/output error
> Thread 1 failed open /sys/kernel/debug/block/(null)/trace1: 2/No such file or directory
> ....
>
> [ 641.424618] blktrace: page allocation failure: order:5, mode:0x2040d0
> [ 641.438933] [<ffffffff811c1569>] kmem_cache_alloc_trace+0x129/0x400
> [ 641.440240] [<ffffffff811424f8>] relay_open+0x68/0x2c0
> [ 641.441299] [<ffffffff8115deb1>] do_blk_trace_setup+0x191/0x2d0
>
> gdb) l *(relay_open+0x68)
> 0xffffffff811424f8 is in relay_open (kernel/relay.c:582).
> 577 return NULL;
> 578 if (subbuf_size > UINT_MAX / n_subbufs)
> 579 return NULL;
> 580
> 581 chan = kzalloc(sizeof(struct rchan), GFP_KERNEL);
> 582 if (!chan)
> 583 return NULL;
> 584
> 585 chan->version = RELAYFS_CHANNEL_VERSION;
> 586 chan->n_subbufs = n_subbufs;
>
> and struct rchan has a member struct rchan_buf *buf[NR_CPUS];
> and CONFIG_NR_CPUS=8192, hence the attempt at an order 5 allocation
> that fails here....
Hm. Have you always had MAX_SMP (and the NR_CPU==8192 that it causes)?
From a quick check, none of this code seems to be new.
That said, having that
struct rchan_buf *buf[NR_CPUS];
in "struct rchan" really is something we should fix. We really should
strive to not allocate things by CONFIG_NR_CPU's, but by the actual
real CPU count.
This looks to be mostly Jens' code, and much of it harkens back to 2006. Jens?
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-18 08:10 +0200 |
| Message-ID | <q9X1U-2y9-17@gated-at.bofh.it> |
| In reply to | #1227583 |
Gaah, my mailer autocompleted Jens' email with an old one.. Sorry for the repeat email with the correct address. On Thu, Sep 17, 2015 at 11:04 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Sep 17, 2015 at 10:40 PM, Dave Chinner <david@fromorbit.com> wrote: >> >> PS: just hit another "did this just get broken in 4.3-rc1" issue - I >> can't run blktrace while there's a IO load because: >> >> $ sudo blktrace -d /dev/vdc >> BLKTRACESETUP(2) /dev/vdc failed: 5/Input/output error >> Thread 1 failed open /sys/kernel/debug/block/(null)/trace1: 2/No such file or directory >> .... >> >> [ 641.424618] blktrace: page allocation failure: order:5, mode:0x2040d0 >> [ 641.438933] [<ffffffff811c1569>] kmem_cache_alloc_trace+0x129/0x400 >> [ 641.440240] [<ffffffff811424f8>] relay_open+0x68/0x2c0 >> [ 641.441299] [<ffffffff8115deb1>] do_blk_trace_setup+0x191/0x2d0 >> >> gdb) l *(relay_open+0x68) >> 0xffffffff811424f8 is in relay_open (kernel/relay.c:582). >> 577 return NULL; >> 578 if (subbuf_size > UINT_MAX / n_subbufs) >> 579 return NULL; >> 580 >> 581 chan = kzalloc(sizeof(struct rchan), GFP_KERNEL); >> 582 if (!chan) >> 583 return NULL; >> 584 >> 585 chan->version = RELAYFS_CHANNEL_VERSION; >> 586 chan->n_subbufs = n_subbufs; >> >> and struct rchan has a member struct rchan_buf *buf[NR_CPUS]; >> and CONFIG_NR_CPUS=8192, hence the attempt at an order 5 allocation >> that fails here.... > > Hm. Have you always had MAX_SMP (and the NR_CPU==8192 that it causes)? > From a quick check, none of this code seems to be new. > > That said, having that > > struct rchan_buf *buf[NR_CPUS]; > > in "struct rchan" really is something we should fix. We really should > strive to not allocate things by CONFIG_NR_CPU's, but by the actual > real CPU count. > > This looks to be mostly Jens' code, and much of it harkens back to 2006. Jens? > > Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Jens Axboe <axboe@fb.com> |
|---|---|
| Date | 2015-09-18 16:30 +0200 |
| Message-ID | <qa4PL-5bi-9@gated-at.bofh.it> |
| In reply to | #1227585 |
On 09/18/2015 12:06 AM, Linus Torvalds wrote: > Gaah, my mailer autocompleted Jens' email with an old one.. > > Sorry for the repeat email with the correct address. > > On Thu, Sep 17, 2015 at 11:04 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> On Thu, Sep 17, 2015 at 10:40 PM, Dave Chinner <david@fromorbit.com> wrote: >>> >>> PS: just hit another "did this just get broken in 4.3-rc1" issue - I >>> can't run blktrace while there's a IO load because: >>> >>> $ sudo blktrace -d /dev/vdc >>> BLKTRACESETUP(2) /dev/vdc failed: 5/Input/output error >>> Thread 1 failed open /sys/kernel/debug/block/(null)/trace1: 2/No such file or directory >>> .... >>> >>> [ 641.424618] blktrace: page allocation failure: order:5, mode:0x2040d0 >>> [ 641.438933] [<ffffffff811c1569>] kmem_cache_alloc_trace+0x129/0x400 >>> [ 641.440240] [<ffffffff811424f8>] relay_open+0x68/0x2c0 >>> [ 641.441299] [<ffffffff8115deb1>] do_blk_trace_setup+0x191/0x2d0 >>> >>> gdb) l *(relay_open+0x68) >>> 0xffffffff811424f8 is in relay_open (kernel/relay.c:582). >>> 577 return NULL; >>> 578 if (subbuf_size > UINT_MAX / n_subbufs) >>> 579 return NULL; >>> 580 >>> 581 chan = kzalloc(sizeof(struct rchan), GFP_KERNEL); >>> 582 if (!chan) >>> 583 return NULL; >>> 584 >>> 585 chan->version = RELAYFS_CHANNEL_VERSION; >>> 586 chan->n_subbufs = n_subbufs; >>> >>> and struct rchan has a member struct rchan_buf *buf[NR_CPUS]; >>> and CONFIG_NR_CPUS=8192, hence the attempt at an order 5 allocation >>> that fails here.... >> >> Hm. Have you always had MAX_SMP (and the NR_CPU==8192 that it causes)? >> From a quick check, none of this code seems to be new. >> >> That said, having that >> >> struct rchan_buf *buf[NR_CPUS]; >> >> in "struct rchan" really is something we should fix. We really should >> strive to not allocate things by CONFIG_NR_CPU's, but by the actual >> real CPU count. >> >> This looks to be mostly Jens' code, and much of it harkens back to 2006. Jens? The relayfs code mostly came out of IBM, but yes, that alloc doesn't look nice. Not a regression, though, I don't think that has changed in years. I'll take a stab at fixing this. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Chris Mason <clm@fb.com> |
|---|---|
| Date | 2015-09-18 15:20 +0200 |
| Message-ID | <qa3K2-3Fh-21@gated-at.bofh.it> |
| In reply to | #1227583 |
On Thu, Sep 17, 2015 at 11:04:03PM -0700, Linus Torvalds wrote: > On Thu, Sep 17, 2015 at 10:40 PM, Dave Chinner <david@fromorbit.com> wrote: > > > > Ok, makes sense - the plug is not being flushed as we switch away, > > but Chris' patch makes it do that. > > Yup. Huh, that does make much more sense, thanks Linus. I'm wondering where else I've assumed that cond_resched() unplugged. > > And I actually think Chris' patch is better than the one I sent out > (but maybe the scheduler people should take a look at the behavior of > cond_resched()), I just wanted you to test that to verify the > behavior. Ok, I'll fix up the description and comments and send out. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Jens Axboe <axboe@fb.com> |
|---|---|
| Date | 2015-09-18 16:30 +0200 |
| Message-ID | <qa4PM-5bi-33@gated-at.bofh.it> |
| In reply to | #1227854 |
On 09/18/2015 07:16 AM, Chris Mason wrote: > On Thu, Sep 17, 2015 at 11:04:03PM -0700, Linus Torvalds wrote: >> On Thu, Sep 17, 2015 at 10:40 PM, Dave Chinner <david@fromorbit.com> wrote: >>> >>> Ok, makes sense - the plug is not being flushed as we switch away, >>> but Chris' patch makes it do that. >> >> Yup. > > Huh, that does make much more sense, thanks Linus. I'm wondering where > else I've assumed that cond_resched() unplugged. > >> >> And I actually think Chris' patch is better than the one I sent out >> (but maybe the scheduler people should take a look at the behavior of >> cond_resched()), I just wanted you to test that to verify the >> behavior. It makes no sense for preemption schedule to NOT unplug, the fact that it doesn't is news to me as well. It was never the intent of the unplug-on-schedule to NOT unplug for certain schedule out events, that seems like very odd behavior. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-18 17:40 +0200 |
| Message-ID | <qa5Vx-6Jf-59@gated-at.bofh.it> |
| In reply to | #1227916 |
On Fri, Sep 18, 2015 at 7:23 AM, Jens Axboe <axboe@fb.com> wrote:
>
> It makes no sense for preemption schedule to NOT unplug, the fact that it
> doesn't is news to me as well. It was never the intent of the
> unplug-on-schedule to NOT unplug for certain schedule out events, that seems
> like very odd behavior.
Actually, even a *full* schedule doesn't unplug, unless the process is
going to sleep. See sched_submit_work(), which will only call the
unplugging if the process is actually going to sleep (ok, so it's a
bit subtle if you don't know the state rules, but it's the
"!tsk->state" check there)
So preemption and cond_resched() isn't _that_ odd. We've basically
treated a non-sleeping schedule as a no-op for the task work.
The thinking was probably that it might be better to delay starting
the IO in case we get scheduled back quickly, and we're obviously not
actually _sleeping_, so it's likely not too bad.
Now, that's probably bogus, and I think that we should perhaps just
make the rule be that "if we actually switch to another task, we run
blk_schedule_flush_plug()".
But it should be noted that that really *does* introduce a lot of new
potential races. Traditionally, our block layer plugging has been
entirely thread-synchronous, and would never happen asynchronously.
But with preemption, that "switch to another thread" really *does*
happen asynchronously.
So making things always happen on task switch is actually fairly
dangerous, and potentially adds the need for much more synchronization
for the IO submission.
What we possibly *could* make the scheduler rule be:
- if it's not an actual PREEMPT_ACTIVE (ie in a random place)
- _and_ we actually switch to another thread
- _then_ do the whole blk_schedule_flush_plug(tsk) thing.
adding some scheduler people to the explicit cc list.
That said, the "cond_resched[_lock]()" functions currently always set
PREEMPT_ACTIVE (indirectly - they use preempt_schedule_common()), so
even though those are synchronous, right now they *look* asynchronous
to the scheduler, so we'd still have to sort that out.
Ingo/Peter/Frederic? Comments?
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-09-18 18:10 +0200 |
| Message-ID | <qa6oy-7wu-27@gated-at.bofh.it> |
| In reply to | #1228079 |
On Fri, Sep 18, 2015 at 05:59:56PM +0200, Peter Zijlstra wrote: > > - if it's not an actual PREEMPT_ACTIVE (ie in a random place) > > PREEMPT_ACTIVE is actually a recursion flag, 'random place' does not > factor into it. That is, most 'random places' will not have > PREEMPT_ACTIVE set. Ah, _not_ PREEMPT_ACTIVE, its late and I can't read no moar. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-09-18 18:10 +0200 |
| Message-ID | <qa6oy-7wu-29@gated-at.bofh.it> |
| In reply to | #1228079 |
On Fri, Sep 18, 2015 at 08:32:15AM -0700, Linus Torvalds wrote: > Actually, even a *full* schedule doesn't unplug, unless the process is > going to sleep. See sched_submit_work(), which will only call the > unplugging if the process is actually going to sleep (ok, so it's a > bit subtle if you don't know the state rules, but it's the > "!tsk->state" check there) > > So preemption and cond_resched() isn't _that_ odd. We've basically > treated a non-sleeping schedule as a no-op for the task work. > > The thinking was probably that it might be better to delay starting > the IO in case we get scheduled back quickly, and we're obviously not > actually _sleeping_, so it's likely not too bad. > > Now, that's probably bogus, and I think that we should perhaps just > make the rule be that "if we actually switch to another task, we run > blk_schedule_flush_plug()". That gets to be a tad tricky. We must either do it while holding scheduler locks (and I would really want to avoid calling block layer code with those held) or when the new task is already scheduled (and that would suck too, because then we account stuff to the wrong task). > But it should be noted that that really *does* introduce a lot of new > potential races. Traditionally, our block layer plugging has been > entirely thread-synchronous, and would never happen asynchronously. > But with preemption, that "switch to another thread" really *does* > happen asynchronously. > > So making things always happen on task switch is actually fairly > dangerous, and potentially adds the need for much more synchronization > for the IO submission. Can't say I'm a fan of it :/ > What we possibly *could* make the scheduler rule be: > > - if it's not an actual PREEMPT_ACTIVE (ie in a random place) PREEMPT_ACTIVE is actually a recursion flag, 'random place' does not factor into it. That is, most 'random places' will not have PREEMPT_ACTIVE set. > - _and_ we actually switch to another thread This is 'hard', as per the above. > - _then_ do the whole blk_schedule_flush_plug(tsk) thing. > > adding some scheduler people to the explicit cc list. > > That said, the "cond_resched[_lock]()" functions currently always set > PREEMPT_ACTIVE (indirectly - they use preempt_schedule_common()), so > even though those are synchronous, right now they *look* asynchronous > to the scheduler, so we'd still have to sort that out. See PREEMPT_ACTIVE being a recursion flag, we set it there so we won't preempt while we're already scheduling. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-18 18:20 +0200 |
| Message-ID | <qa6ye-7HP-33@gated-at.bofh.it> |
| In reply to | #1228118 |
On Fri, Sep 18, 2015 at 8:59 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> See PREEMPT_ACTIVE being a recursion flag, we set it there so we won't
> preempt while we're already scheduling.
PREEMPT_ACTIVE does more than that. It really is a sign that "this is
not synchronous". It causes the scheduler to ignore the current task
flags (because it might already be TASK_SLEEPING, but we aren't
_actually_ ready to sleep yet) etc.
So no. It's not "you can't be preempted during scheduling". That's the
*normal* preempt count, and all scheduling calls end up setting that
some way (ie "schedule()" just does preempt_disable()).
So I disagree with your notion that it's a recursion flag. It is
absolutely nothing of the sort. It gets set by preemption - and,
somewhat illogically, by cond_resched().
The fact that cond_resched() sets it is *probably* because some of the
callers end up calling it from page fault paths etc, and the same
"ignore TASK_SLEEPING etc" rules apply. But it does mean that
"cond_resched()" is a bit misleaning as a name. It's really a
"cond_preempt()".
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web