I tested performance of these rrw lock changes by reading cached data with 28 CPUs, on illumos. I found no improvement, because lock contention from ARC's add_reference/remove_reference() slows things down so much that the rrw lock is not noticeable. However, after removing this source of contention, the rrw lock changes provided a 20% performance improvement.
details, including flame graphs <http://www.brendangregg.com/FlameGraphs/cpuflamegraphs.html> of kernel CPU time: reads of 8KB of cached data recordsize=8k; all other properties default 28 vCPUs on VMware ESX, 4 hardware sockets, each with 8 cores, each with 2 hyperthreads 28 processes 28 files (each process reads from its own file) see fio config below. Stock illumos: ~350,000 IOPS http://mahrens.org/flame/cached_reads_28_cpu_stock.svg primary problem is arc_buf_add_ref() / arc_buf_remove_ref() (>75% of all CPU) Applying Alexander's changes to break up the rrw lock: ~350,000 IOPS http://mahrens.org/flame/cached_reads_28_cpu_rrw.svg primary problem is still arc_buf_add_ref() / arc_buf_remove_ref() (>75% of all CPU) Fixing arc_buf_add/remove_ref() by creating per-cpu arcs_lists: 1,000,000 IOPS http://mahrens.org/flame/cached_reads_28_cpu_agg.svg rrw_enter/exit taking ~8% of all CPU Both arc_buf_add/removeref and rrw lock fixes: 1,200,000 IOPS http://mahrens.org/flame/cached_reads_28_cpu_agg_rrw.svg rrw_enter/exit taking ~1.5% of all CPU --matt fio config file: [global] fallocate=none ioengine=psync numjobs=28 iodepth=1 time_based runtime=10m bs=8k rw=randread filesize=1g size=1g randrepeat=0 use_os_rand=1 [test] directory=/test/fs On Mon, Jun 9, 2014 at 11:51 AM, Alexander Motin <[email protected]> wrote: > On 09.06.2014 21:33, Matthew Ahrens wrote: > >> Changes look good to me. I've ported them to illumos: >> >> http://reviews.csiden.org/r/46/diff/#index_header >> >> I'll do some more testing and then put them up for a "real" illumos code >> review. >> > > Thank you! I hope to see both patches upstream soon -- I hate to drag many > uncommitted patches after me. :) > > > Alexander, do you want to add any copyright notice to your patch before >> it goes to illumos? (it is not required) >> > > Do as you see better. I don't put too much importance to it -- I already > have enough of my name spread over FreeBSD sources. ;) If that is expected > -- you may put there one line like: "Alexander Motin <[email protected]>". > > On Sun, Jun 8, 2014 at 1:53 PM, Alexander Motin <[email protected] >> <mailto:[email protected]>> wrote: >> >> On 08.06.2014 21:32, Alexander Motin wrote: >> >> On 07.06.2014 22:35, Matthew Ahrens wrote: >> >> code review: >> >> zfsvfs_hold/rele need to pass the correct tags through to >> rrw_enter(). >> The existing code will not work (if >> reference_tracking_enable), because >> you will be holding with tag "zfsvfs_hold" and releasing with >> "zfsvfs_rele". The existing code holds and releases with tag >> e.g. >> "zfs_prop_set_userquota"; that needs to remain the case. >> >> >> Indeed there was a bug, thank you. >> >> I think it would be cleaner to encapsulate the multitude of >> locks within >> the rrw code, but I can live with it as is. >> >> >> I had such thought initially, but then decided it will be too >> big mess >> to do it inside. Also it may not always be needed. But now >> you've asked, >> I've made intermediate solution: let me introduce Recursable >> Read-Mostly >> locks, as a wrapper on top of existing Recursable Read-Write >> locks. :) >> >> The attached patch implements rrmlock, and uses them for >> teardown lock. >> It indeed looks much better. Thanks for pushing me to do it. :) >> What do >> you think? >> >> >> I'm sorry, I've missed a bug until built kernel with debugging. >> Fixed version is attached. >> >> -- >> Alexander Motin >> >> >> > > -- > Alexander Motin >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
