block: fix ioc locking warning
Subsystems (6)
PATCH 1/1 block: fix ioc locking warning
Meelis reported a warning:
WARNING: at kernel/timer.c:1122 run_timer_softirq+0x199/0x1ec() Hardware name: 939Dual-SATA2 timer: cfq_idle_slice_timer+0x0/0xaa preempt leak: 00000102 -> 00000103 Modules linked in: sr_mod cdrom videodev media drm_kms_helper ohci_hcd ehci_hcd v4l2_compat_ioctl32 usbcore i2c_ali15x3 snd_seq drm snd_timer snd_seq Pid: 0, comm: swapper Not tainted 3.3.0-rc2-00110-gd125666 #176 Call Trace: <IRQ> [<ffffffff81022aaa>] warn_slowpath_common+0x7e/0x96 [<ffffffff8114c485>] ? cfq_slice_expired+0x1d/0x1d [<ffffffff81022b56>] warn_slowpath_fmt+0x41/0x43 [<ffffffff8114c526>] ? cfq_idle_slice_timer+0xa1/0xaa [<ffffffff8114c485>] ? cfq_slice_expired+0x1d/0x1d [<ffffffff8102c124>] run_timer_softirq+0x199/0x1ec [<ffffffff81047a53>] ? timekeeping_get_ns+0x12/0x31 [<ffffffff810145fd>] ? apic_write+0x11/0x13 [<ffffffff81027475>] __do_softirq+0x74/0xfa [<ffffffff812f337a>] call_softirq+0x1a/0x30 [<ffffffff81002ff9>] do_softirq+0x31/0x68 [<ffffffff810276cf>] irq_exit+0x3d/0xa3 [<ffffffff81014aca>] smp_apic_timer_interrupt+0x6b/0x77 [<ffffffff812f2de9>] apic_timer_interrupt+0x69/0x70 <EOI> [<ffffffff81040136>] ? sched_clock_cpu+0x73/0x7d [<ffffffff81040136>] ? sched_clock_cpu+0x73/0x7d [<ffffffff8100801f>] ? default_idle+0x1e/0x32 [<ffffffff81008019>] ? default_idle+0x18/0x32 [<ffffffff810008b1>] cpu_idle+0x87/0xd1 [<ffffffff812de861>] rest_init+0x85/0x89 [<ffffffff81659a4d>] start_kernel+0x2eb/0x2f8 [<ffffffff8165926e>] x86_64_start_reservations+0x7e/0x82 [<ffffffff81659362>] x86_64_start_kernel+0xf0/0xf7
this_q == locked_q is possible. There are two problems here:
1. In UP case, there is preemption counter issue as spin_trylock always successes.
2. In SMP case, the loop breaks too earlier.
block/blk-ioc.c +3 -1
@@ -204,7 +204,9 @@ void put_io_context(struct io_context *ioc, struct request_queue *locked_q)spin_unlock (last_q->queue_lock); last_q =NULL ; -if (!spin_trylock (this_q->queue_lock)) +/* spin_trylock() always successes in UP case */ +if (this_q != locked_q && + !spin_trylock (this_q->queue_lock))break ; last_q = this_q;continue ;
Jens Axboe
maintainerQuoted 32 lines · Shaohua Li
Meelis reported a warning:
WARNING: at kernel/timer.c:1122 run_timer_softirq+0x199/0x1ec() Hardware name: 939Dual-SATA2 timer: cfq_idle_slice_timer+0x0/0xaa preempt leak: 00000102 -> 00000103 Modules linked in: sr_mod cdrom videodev media drm_kms_helper ohci_hcd ehci_hcd v4l2_compat_ioctl32 usbcore i2c_ali15x3 snd_seq drm snd_timer snd_seq Pid: 0, comm: swapper Not tainted 3.3.0-rc2-00110-gd125666 #176 Call Trace: <IRQ> [<ffffffff81022aaa>] warn_slowpath_common+0x7e/0x96 [<ffffffff8114c485>] ? cfq_slice_expired+0x1d/0x1d [<ffffffff81022b56>] warn_slowpath_fmt+0x41/0x43 [<ffffffff8114c526>] ? cfq_idle_slice_timer+0xa1/0xaa [<ffffffff8114c485>] ? cfq_slice_expired+0x1d/0x1d [<ffffffff8102c124>] run_timer_softirq+0x199/0x1ec [<ffffffff81047a53>] ? timekeeping_get_ns+0x12/0x31 [<ffffffff810145fd>] ? apic_write+0x11/0x13 [<ffffffff81027475>] __do_softirq+0x74/0xfa [<ffffffff812f337a>] call_softirq+0x1a/0x30 [<ffffffff81002ff9>] do_softirq+0x31/0x68 [<ffffffff810276cf>] irq_exit+0x3d/0xa3 [<ffffffff81014aca>] smp_apic_timer_interrupt+0x6b/0x77 [<ffffffff812f2de9>] apic_timer_interrupt+0x69/0x70 <EOI> [<ffffffff81040136>] ? sched_clock_cpu+0x73/0x7d [<ffffffff81040136>] ? sched_clock_cpu+0x73/0x7d [<ffffffff8100801f>] ? default_idle+0x1e/0x32 [<ffffffff81008019>] ? default_idle+0x18/0x32 [<ffffffff810008b1>] cpu_idle+0x87/0xd1 [<ffffffff812de861>] rest_init+0x85/0x89 [<ffffffff81659a4d>] start_kernel+0x2eb/0x2f8 [<ffffffff8165926e>] x86_64_start_reservations+0x7e/0x82 [<ffffffff81659362>] x86_64_start_kernel+0xf0/0xf7
Quoted 4 lines
this_q==locked_qis possible. There are two problems here:
1. In UP case, there is preemption counter issue asspin_trylockalways successes.
2. In SMP case, the loop breaks too earlier.
Thanks Shaohua, applied!
Signature · 1 line
Jens Axboe
Quoted 32 lines · Shaohua Li
Meelis reported a warning:
WARNING: at kernel/timer.c:1122 run_timer_softirq+0x199/0x1ec() Hardware name: 939Dual-SATA2 timer: cfq_idle_slice_timer+0x0/0xaa preempt leak: 00000102 -> 00000103 Modules linked in: sr_mod cdrom videodev media drm_kms_helper ohci_hcd ehci_hcd v4l2_compat_ioctl32 usbcore i2c_ali15x3 snd_seq drm snd_timer snd_seq Pid: 0, comm: swapper Not tainted 3.3.0-rc2-00110-gd125666 #176 Call Trace: <IRQ> [<ffffffff81022aaa>] warn_slowpath_common+0x7e/0x96 [<ffffffff8114c485>] ? cfq_slice_expired+0x1d/0x1d [<ffffffff81022b56>] warn_slowpath_fmt+0x41/0x43 [<ffffffff8114c526>] ? cfq_idle_slice_timer+0xa1/0xaa [<ffffffff8114c485>] ? cfq_slice_expired+0x1d/0x1d [<ffffffff8102c124>] run_timer_softirq+0x199/0x1ec [<ffffffff81047a53>] ? timekeeping_get_ns+0x12/0x31 [<ffffffff810145fd>] ? apic_write+0x11/0x13 [<ffffffff81027475>] __do_softirq+0x74/0xfa [<ffffffff812f337a>] call_softirq+0x1a/0x30 [<ffffffff81002ff9>] do_softirq+0x31/0x68 [<ffffffff810276cf>] irq_exit+0x3d/0xa3 [<ffffffff81014aca>] smp_apic_timer_interrupt+0x6b/0x77 [<ffffffff812f2de9>] apic_timer_interrupt+0x69/0x70 <EOI> [<ffffffff81040136>] ? sched_clock_cpu+0x73/0x7d [<ffffffff81040136>] ? sched_clock_cpu+0x73/0x7d [<ffffffff8100801f>] ? default_idle+0x1e/0x32 [<ffffffff81008019>] ? default_idle+0x18/0x32 [<ffffffff810008b1>] cpu_idle+0x87/0xd1 [<ffffffff812de861>] rest_init+0x85/0x89 [<ffffffff81659a4d>] start_kernel+0x2eb/0x2f8 [<ffffffff8165926e>] x86_64_start_reservations+0x7e/0x82 [<ffffffff81659362>] x86_64_start_kernel+0xf0/0xf7
Quoted 4 lines
this_q==locked_qis possible. There are two problems here:
1. In UP case, there is preemption counter issue asspin_trylockalways successes.
2. In SMP case, the loop breaks too earlier.
Thanks Shaohua. So is it the case where there are more than one cic's on ioc->ioc_list and first cic's queue is not same as locked_queue. But some other cic other than first has queue same as locked_queue.
In that case current code will still defer freeing of ioc and cic to a worker thread. So this patch will introduce one optimization to handle those cases and avoid calling worker thread.
Secondly it fixes the discrepancy of preemption count on UP machines, where we have one extra preemption count after finish of function put_io_context(). So for UP case spin_trylock() increases the preemption count and always returns success. As this_q == locked_q we never try to do unlock on this queue and hence never decrement the preemption count hence resulting in preemption count warning.
Changlog was not obivious atleast to me. I wished it was little more descriptive. Anyway, patch is already committed..
Thanks
Vivek
Jens Axboe
maintainerQuoted 53 lines · Vivek Goyal
Quoted 32 lines · Shaohua Li
Meelis reported a warning:
WARNING: at kernel/timer.c:1122 run_timer_softirq+0x199/0x1ec() Hardware name: 939Dual-SATA2 timer: cfq_idle_slice_timer+0x0/0xaa preempt leak: 00000102 -> 00000103 Modules linked in: sr_mod cdrom videodev media drm_kms_helper ohci_hcd ehci_hcd v4l2_compat_ioctl32 usbcore i2c_ali15x3 snd_seq drm snd_timer snd_seq Pid: 0, comm: swapper Not tainted 3.3.0-rc2-00110-gd125666 #176 Call Trace: <IRQ> [<ffffffff81022aaa>] warn_slowpath_common+0x7e/0x96 [<ffffffff8114c485>] ? cfq_slice_expired+0x1d/0x1d [<ffffffff81022b56>] warn_slowpath_fmt+0x41/0x43 [<ffffffff8114c526>] ? cfq_idle_slice_timer+0xa1/0xaa [<ffffffff8114c485>] ? cfq_slice_expired+0x1d/0x1d [<ffffffff8102c124>] run_timer_softirq+0x199/0x1ec [<ffffffff81047a53>] ? timekeeping_get_ns+0x12/0x31 [<ffffffff810145fd>] ? apic_write+0x11/0x13 [<ffffffff81027475>] __do_softirq+0x74/0xfa [<ffffffff812f337a>] call_softirq+0x1a/0x30 [<ffffffff81002ff9>] do_softirq+0x31/0x68 [<ffffffff810276cf>] irq_exit+0x3d/0xa3 [<ffffffff81014aca>] smp_apic_timer_interrupt+0x6b/0x77 [<ffffffff812f2de9>] apic_timer_interrupt+0x69/0x70 <EOI> [<ffffffff81040136>] ? sched_clock_cpu+0x73/0x7d [<ffffffff81040136>] ? sched_clock_cpu+0x73/0x7d [<ffffffff8100801f>] ? default_idle+0x1e/0x32 [<ffffffff81008019>] ? default_idle+0x18/0x32 [<ffffffff810008b1>] cpu_idle+0x87/0xd1 [<ffffffff812de861>] rest_init+0x85/0x89 [<ffffffff81659a4d>] start_kernel+0x2eb/0x2f8 [<ffffffff8165926e>] x86_64_start_reservations+0x7e/0x82 [<ffffffff81659362>] x86_64_start_kernel+0xf0/0xf7Quoted 4 lines
this_q==locked_qis possible. There are two problems here:
1. In UP case, there is preemption counter issue asspin_trylockalways successes.
2. In SMP case, the loop breaks too earlier.Thanks Shaohua. So is it the case where there are more than one cic's on
ioc->ioc_listand first cic's queue is not same aslocked_queue. But some other cic other than first has queue same aslocked_queue.In that case current code will still defer freeing of ioc and cic to a worker thread. So this patch will introduce one optimization to handle those cases and avoid calling worker thread.
Secondly it fixes the discrepancy of preemption count on UP machines, where we have one extra preemption count after finish of function
put_io_context(). So for UP casespin_trylock()increases the preemption count and always returns success. Asthis_q==locked_qwe never try to do unlock on this queue and hence never decrement the preemption count hence resulting in preemption count warning.
Quoted 2 lines
Changlog was not obivious atleast to me. I wished it was little more descriptive. Anyway, patch is already committed..
We can always amend the changelog, so don't worry about it already being committed. If you want to add/change something, just send it in.
Signature · 1 line
Jens Axboe
Vivek Goyal
Tested-by Knut PetersenOn Mon, Feb 06, 2012 at 05:09:05PM +0100, Jens Axboe wrote:
[..]
Quoted 5 lines
Quoted 2 lines
Changlog was not obivious atleast to me. I wished it was little more descriptive. Anyway, patch is already committed..
We can always amend the changelog, so don't worry about it already being committed. If you want to add/change something, just send it in.
How about something like as follows.
block: Do not lock try to lock already locked queue again
put_io_context() can be called with one of the request queue lock already held. But if the locked queue's cic is not first in the ioc->ioc_list, then we have two possible issues.
- For above condition, current code bails out and schedules the worker thread for freeing up ioc. This can be optimized.
- It might happen that we received the queue locked but we still do the trylock on the queue. For SMP case that's not a problem as we will fail to lock already locked queue, but in case of UP, we seem to succeed and in the process increment the preempt count. Once we are done with ioc_exit_icq(), we do not call spin_unlock() on locked queue as we are not supposed to. This leads to imbalance in preemtion count and following warning was reported.
This patch fixes both the above issues by making sure we do not try to lock already locked queue again.
WARNING: at kernel/timer.c:1122 run_timer_softirq+0x199/0x1ec() Hardware name: 939Dual-SATA2 timer: cfq_idle_slice_timer+0x0/0xaa preempt leak: 00000102 -> 00000103 Modules linked in: sr_mod cdrom videodev media drm_kms_helper ohci_hcd ehci_hcd v4l2_compat_ioctl32 usbcore i2c_ali15x3 snd_seq drm snd_timer snd_seq Pid: 0, comm: swapper Not tainted 3.3.0-rc2-00110-gd125666 #176 Call Trace: <IRQ> [<ffffffff81022aaa>] warn_slowpath_common+0x7e/0x96 [<ffffffff8114c485>] ? cfq_slice_expired+0x1d/0x1d [<ffffffff81022b56>] warn_slowpath_fmt+0x41/0x43 [<ffffffff8114c526>] ? cfq_idle_slice_timer+0xa1/0xaa [<ffffffff8114c485>] ? cfq_slice_expired+0x1d/0x1d [<ffffffff8102c124>] run_timer_softirq+0x199/0x1ec [<ffffffff81047a53>] ? timekeeping_get_ns+0x12/0x31 [<ffffffff810145fd>] ? apic_write+0x11/0x13 [<ffffffff81027475>] __do_softirq+0x74/0xfa [<ffffffff812f337a>] call_softirq+0x1a/0x30 [<ffffffff81002ff9>] do_softirq+0x31/0x68 [<ffffffff810276cf>] irq_exit+0x3d/0xa3 [<ffffffff81014aca>] smp_apic_timer_interrupt+0x6b/0x77 [<ffffffff812f2de9>] apic_timer_interrupt+0x69/0x70 <EOI> [<ffffffff81040136>] ? sched_clock_cpu+0x73/0x7d [<ffffffff81040136>] ? sched_clock_cpu+0x73/0x7d [<ffffffff8100801f>] ? default_idle+0x1e/0x32 [<ffffffff81008019>] ? default_idle+0x18/0x32 [<ffffffff810008b1>] cpu_idle+0x87/0xd1 [<ffffffff812de861>] rest_init+0x85/0x89 [<ffffffff81659a4d>] start_kernel+0x2eb/0x2f8 [<ffffffff8165926e>] x86_64_start_reservations+0x7e/0x82 [<ffffffff81659362>] x86_64_start_kernel+0xf0/0xf7
Thanks
Vivek
Tejun Heo
maintainerHello,
Quoted 51 lines · Vivek Goyal
block: Do not lock try to lock already locked queue again
put_io_context()can be called with one of the request queue lock already held. But if the locked queue's cic is not first in theioc->ioc_list, then we have two possible issues.- For above condition, current code bails out and schedules the worker thread for freeing up ioc. This can be optimized.
- It might happen that we received the queue locked but we still do the trylock on the queue. For SMP case that's not a problem as we will fail to lock already locked queue, but in case of UP, we seem to succeed and in the process increment the preempt count. Once we are done with
ioc_exit_icq(), we do not callspin_unlock()on locked queue as we are not supposed to. This leads to imbalance in preemtion count and following warning was reported.This patch fixes both the above issues by making sure we do not try to lock already locked queue again.
WARNING: at kernel/timer.c:1122 run_timer_softirq+0x199/0x1ec() Hardware name: 939Dual-SATA2 timer: cfq_idle_slice_timer+0x0/0xaa preempt leak: 00000102 -> 00000103 Modules linked in: sr_mod cdrom videodev media drm_kms_helper ohci_hcd ehci_hcd v4l2_compat_ioctl32 usbcore i2c_ali15x3 snd_seq drm snd_timer snd_seq Pid: 0, comm: swapper Not tainted 3.3.0-rc2-00110-gd125666 #176 Call Trace: <IRQ> [<ffffffff81022aaa>] warn_slowpath_common+0x7e/0x96 [<ffffffff8114c485>] ? cfq_slice_expired+0x1d/0x1d [<ffffffff81022b56>] warn_slowpath_fmt+0x41/0x43 [<ffffffff8114c526>] ? cfq_idle_slice_timer+0xa1/0xaa [<ffffffff8114c485>] ? cfq_slice_expired+0x1d/0x1d [<ffffffff8102c124>] run_timer_softirq+0x199/0x1ec [<ffffffff81047a53>] ? timekeeping_get_ns+0x12/0x31 [<ffffffff810145fd>] ? apic_write+0x11/0x13 [<ffffffff81027475>] __do_softirq+0x74/0xfa [<ffffffff812f337a>] call_softirq+0x1a/0x30 [<ffffffff81002ff9>] do_softirq+0x31/0x68 [<ffffffff810276cf>] irq_exit+0x3d/0xa3 [<ffffffff81014aca>] smp_apic_timer_interrupt+0x6b/0x77 [<ffffffff812f2de9>] apic_timer_interrupt+0x69/0x70 <EOI> [<ffffffff81040136>] ? sched_clock_cpu+0x73/0x7d [<ffffffff81040136>] ? sched_clock_cpu+0x73/0x7d [<ffffffff8100801f>] ? default_idle+0x1e/0x32 [<ffffffff81008019>] ? default_idle+0x18/0x32 [<ffffffff810008b1>] cpu_idle+0x87/0xd1 [<ffffffff812de861>] rest_init+0x85/0x89 [<ffffffff81659a4d>] start_kernel+0x2eb/0x2f8 [<ffffffff8165926e>] x86_64_start_reservations+0x7e/0x82 [<ffffffff81659362>] x86_64_start_kernel+0xf0/0xf7
Quoted 4 lines
Signed-off-by Shaohua Li <[email protected]>Reported-by Meelis Roos <[email protected]>Reported-by Knut Petersen <[email protected]>Tested-by Knut Petersen <[email protected]>
Yeah, this seems better to me. Jens, if you're gonna amend the commit, please consider collapsing the following patch into the original patch too. Thanks.
block/blk-ioc.c +8 -1
@@ -204,7 +204,14 @@ void put_io_context(struct io_context *ioc, struct request_queue *locked_q)spin_unlock (last_q->queue_lock); last_q =NULL ; -/* spin_trylock() always successes in UP case */ +/* +* If icq for locked_q wasn't at the head of +* icq_list, we can try to switch back to locked_q. +* On SMP, the following locked_q test avoids +* unnecessary deferring to release_work, on UP, +* incorrect lock state transition (trylock +* succeeding while holding the same lock). +*/ if (this_q != locked_q && !spin_trylock (this_q->queue_lock))break ;
Quoted 4 lines · Tejun Heo
Yeah, this seems better to me. Jens, if you're gonna amend the commit, please consider collapsing the following patch into the original patch too. Thanks.
Guys, is it *really* worth it to do all these crazy games?
How bad is it to just always use the async freeing, instead of this clearly very fragile crazy direct-freeing-with-serious-locking-issues thing?
Sure, even ioc_release_fn() isn't trivial wrt lock handling, but at least it doesn't have to play these *insane* games with recursive locking.
And if workqueues are too expensive, what saner alternatives might there be? This code really is insane.
Linus
Tejun Heo
maintainerHello,
Quoted 11 lines · Linus Torvalds
Quoted 4 lines · Tejun Heo
Yeah, this seems better to me. Jens, if you're gonna amend the commit, please consider collapsing the following patch into the original patch too. Thanks.
Guys, is it *really* worth it to do all these crazy games?
How bad is it to just always use the async freeing, instead of this clearly very fragile crazy direct-freeing-with-serious-locking-issues thing?
It's one wq scheduling on exit for any task which has issued an IO. I don't think it would matter except for task fork/exit microbenchs (or workloads which approximate to that). I'll get some measurements and strip the optimization if it doesn't really show up.
Thanks.
--
tejun
Jens Axboe
maintainerQuoted 11 lines · Tejun Heo
Hello,
Quoted 8 lines · Linus Torvalds
Quoted 4 lines · Tejun Heo
Yeah, this seems better to me. Jens, if you're gonna amend the commit, please consider collapsing the following patch into the original patch too. Thanks.
Guys, is it *really* worth it to do all these crazy games?
Quoted 8 lines
Quoted 3 lines
How bad is it to just always use the async freeing, instead of this clearly very fragile crazy direct-freeing-with-serious-locking-issues thing?
It's one wq scheduling on exit for any task which has issued an IO. I don't think it would matter except for task fork/exit microbenchs (or workloads which approximate to that). I'll get some measurements and strip the optimization if it doesn't really show up.
One (arguably stupid) thing that some users do do is something like:
$ find . -exec grep foo '{}' \;So that would probably be a good pathological test case for this.
Signature · 1 line
Jens Axboe
Tejun Heo
maintainerPATCH 1/1 block: strip out locking optimization in put_io_context()
put_io_context() performed a complex trylock dancing to avoid deferring ioc release to workqueue. It was also broken on UP because trylock was always assumed to succeed which resulted in unbalanced preemption count.
While there are ways to fix the UP breakage, even the most pathological microbench (forced ioc allocation and tight fork/exit loop) fails to show any appreciable performance benefit of the optimization. Strip it out. If there turns out to be workloads which are affected by this change, simpler optimization from the discussion thread can be applied later.
LKML-Reference: <1328514611.21268.66.camel@sli10-conroe>
---
I couldn't find any statiscally meaningful advantage of the optimization with tight fork/exit tests w/ forced ioc creation on fork, which gotta be the most pathological test case for the code path. So, let's remove the ugly optimization. If I missed sth, we can resurrect the simpler optimization later. Jens, this is on top of linus#master without Shaohua's patch.
Thanks.
block/blk-cgroup.c | 2 - block/blk-core.c | 2 - block/blk-ioc.c | 90 +++++----------------------------------------- block/cfq-iosched.c | 2 - fs/ioprio.c | 2 - include/linux/blkdev.h | 3 - include/linux/iocontext.h | 5 +- kernel/fork.c | 2 - 8 files changed, 18 insertions(+), 90 deletions(-)
block/blk-cgroup.c +1 -1
@@ -1659,7 +1659,7 @@ static void blkiocg_attach(struct cgroup ioc =get_task_io_context (task,GFP_ATOMIC ,NUMA_NO_NODE );if (ioc) {ioc_cgroup_changed (ioc); -put_io_context (ioc,NULL ); +put_io_context (ioc); } } }
block/blk-core.c +1 -1
@@ -642,7 +642,7 @@ static inline void blk_free_request(struif (rq->cmd_flags &REQ_ELVPRIV ) {elv_put_request (q, rq);if (rq->elv.icq) -put_io_context (rq->elv.icq->ioc, q); +put_io_context (rq->elv.icq->ioc); }mempool_free (rq, q->rq.rq_pool);
block/blk-ioc.c +11 -79
129 lines not on this page · raw message
block/cfq-iosched.c +1 -1
@@ -1794,7 +1794,7 @@ __cfq_slice_expired(struct cfq_data *cfq cfqd->active_queue =NULL ;if (cfqd->active_cic) { -put_io_context (cfqd->active_cic->icq.ioc, cfqd->queue); +put_io_context (cfqd->active_cic->icq.ioc); cfqd->active_cic =NULL ; } }
fs/ioprio.c +1 -1
@@ -51,7 +51,7 @@ int set_task_ioprio(struct task_struct * ioc =get_task_io_context (task,GFP_ATOMIC ,NUMA_NO_NODE );if (ioc) {ioc_ioprio_changed (ioc, ioprio); -put_io_context (ioc,NULL ); +put_io_context (ioc); }return err;
include/linux/blkdev.h +0 -3
@@ -399,9 +399,6 @@ struct request_queue {/* Throttle data */ struct throtl_data *td;#endif -#ifdef CONFIG_LOCKDEP -int ioc_release_depth; -#endif };#define QUEUE_FLAG_QUEUED 1/* uses generic tag queueing */
include/linux/iocontext.h +2 -3
@@ -133,7 +133,7 @@ static inline struct io_context *ioc_tasstruct task_struct ;#ifdef CONFIG_BLOCK -void put_io_context (struct io_context *ioc,struct request_queue *locked_q); +void put_io_context (struct io_context *ioc);void exit_io_context (struct task_struct *task);struct io_context *get_task_io_context (struct task_struct *task,gfp_t gfp_flags,int node);
@@ -141,8 +141,7 @@ void ioc_ioprio_changed(struct io_contexvoid ioc_cgroup_changed (struct io_context *ioc);#else struct io_context ; -static inline void put_io_context (struct io_context *ioc, -struct request_queue *locked_q) { } +static inline void put_io_context (struct io_context *ioc) { }static inline void exit_io_context (struct task_struct *task) { }#endif
kernel/fork.c +1 -1
@@ -890,7 +890,7 @@ static int copy_io(unsigned long clone_freturn -ENOMEM ; new_ioc->ioprio = ioc->ioprio; - put_io_context(new_ioc, NULL); + put_io_context(new_ioc); }#endif return 0 ;
Jens Axboe
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Quoted 13 lines · Tejun Heo
put_io_context()performed a complex trylock dancing to avoid deferring ioc release to workqueue. It was also broken on UP because trylock was always assumed to succeed which resulted in unbalanced preemption count.While there are ways to fix the UP breakage, even the most pathological microbench (forced ioc allocation and tight fork/exit loop) fails to show any appreciable performance benefit of the optimization. Strip it out. If there turns out to be workloads which are affected by this change, simpler optimization from the discussion thread can be applied later.
Signed-off-by Tejun Heo <[email protected]>
Quoted 8 lines
LKML-Reference: <1328514611.21268.66.camel@sli10-conroe>
---
I couldn't find any statiscally meaningful advantage of the optimization with tight fork/exit tests w/ forced ioc creation on fork, which gotta be the most pathological test case for the code path. So, let's remove the ugly optimization. If I missed sth, we can resurrect the simpler optimization later. Jens, this is on top of linus#master without Shaohua's patch.
OK, then I'm fine with cleaning it up. Applied, thanks Tejun.
Signature · 1 line
Jens Axboe
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Hello, Jens.
Quoted 8 lines · Jens Axboe
Quoted 6 lines
I couldn't find any statiscally meaningful advantage of the optimization with tight fork/exit tests w/ forced ioc creation on fork, which gotta be the most pathological test case for the code path. So, let's remove the ugly optimization. If I missed sth, we can resurrect the simpler optimization later. Jens, this is on top of linus#master without Shaohua's patch.
OK, then I'm fine with cleaning it up. Applied, thanks Tejun.
Hmmm... how about merging Shaohua's smaller fix first until we figure out what's going on with the performance regression he's seeing?
Thanks.
Signature · 1 line
tejun
Jens Axboe
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Quoted 10 lines · Tejun Heo
Hello, Jens.
Quoted 7 lines · Jens Axboe
Quoted 6 lines
I couldn't find any statiscally meaningful advantage of the optimization with tight fork/exit tests w/ forced ioc creation on fork, which gotta be the most pathological test case for the code path. So, let's remove the ugly optimization. If I missed sth, we can resurrect the simpler optimization later. Jens, this is on top of linus#master without Shaohua's patch.
Quoted 4 lines
Quoted 1 line
OK, then I'm fine with cleaning it up. Applied, thanks Tejun.
Hmmm... how about merging Shaohua's smaller fix first until we figure out what's going on with the performance regression he's seeing?
That was already merged in my tree. I don't see how it makes much difference in tracking the regression. You said that removing it made no difference for the find test case, so I'd be more comfortable getting rid of the nasty optimization.
I'll send it when I have pending tomorrow, so there's still a full day to change things. We can just shuffle patches before then, not a problem.
Signature · 1 line
Jens Axboe
Re: [PATCH] block: strip out locking optimization in put_io_context()
Quoted 5 lines · Jens Axboe
That was already merged in my tree. I don't see how it makes much difference in tracking the regression. You said that removing it made no difference for the find test case, so I'd be more comfortable getting rid of the nasty optimization.
Yeah, please just get rid of the crazy code. Maybe *that* fixes the regression too, who knows?
For all we know, the "fast case" is what causes extra locking only to then fail and not even be a fast-path.
I think our default action should always be to simplify and clean up code, unless you have seriously hard numbers to show that the code complexity is worth it.
Linus
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Hello,
Quoted 5 lines · Linus Torvalds
Yeah, please just get rid of the crazy code. Maybe *that* fixes the regression too, who knows?
For all we know, the "fast case" is what causes extra locking only to then fail and not even be a fast-path.
Yeah, I was about to ask Shaohua to test the version w/o optimization. With heavily loaded request_queue, trylock failure could be frequent, which I wasn't testing.
Shaohua, can you please test the version w/o optimization? Also, can you please give a bit more details on the setup? Are there multiple swap devices? Is it SSD or rotating disk?
Quoted 3 lines
I think our default action should always be to simplify and clean up code, unless you have seriously hard numbers to show that the code complexity is worth it.
Sure, it was originally all in put_io_context() and while moving things to wq, it got progressively complex and I lost sense of complexity from staring at it too long.
Thanks.
Signature · 1 line
tejun
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Quoted 7 lines · Tejun Heo
Yeah, I was about to ask Shaohua to test the version w/o optimization. With heavily loaded
request_queue, trylock failure could be frequent, which I wasn't testing.Shaohua, can you please test the version w/o optimization? Also, can you please give a bit more details on the setup? Are there multiple swap devices? Is it SSD or rotating disk?
Can you please also apply the following patch on top of block/for-linus and see how it behaves?
Thanks.
--- block/blk-ioc.c | 54 +++++++++++++++++------------------------------------- 1 file changed, 17 insertions(+), 37 deletions(-)
block/blk-ioc.c +17 -37
@@ -79,52 +79,32 @@ static void ioc_release_fn(struct work_s { struct io_context *ioc = container_of(work, struct io_context, release_work); -struct request_queue *last_q =NULL ; +unsigned long flags; -spin_lock_irq (&ioc->lock); +/* +* Exiting icq may call into put_io_context() through elevator +* which will trigger lockdep warning. The ioc's are guaranteed to +* be different, use a different locking subclass here. Using +* irqsave variant as there's no spin_lock_irq_nested(). +*/ +spin_lock_irqsave_nested (&ioc->lock, flags,1 );while (!hlist_empty (&ioc->icq_list)) {struct io_cq *icq =hlist_entry (ioc->icq_list.first, struct io_cq, ioc_node); -struct request_queue *this_q = icq->q; +struct request_queue *q = icq->q; -if (this_q != last_q) { -/* -* Need to switch to @this_q. Once we release -* @ioc->lock, it can go away along with @cic. -* Hold on to it. -*/ -__blk_get_queue (this_q); - -/* -* blk_put_queue() might sleep thanks to kobject -* idiocy. Always release both locks, put and -* restart. -*/ -if (last_q) { -spin_unlock (last_q->queue_lock); -spin_unlock_irq (&ioc->lock); -blk_put_queue (last_q); - }else { -spin_unlock_irq (&ioc->lock); - } - - last_q = this_q; -spin_lock_irq (this_q->queue_lock); -spin_lock (&ioc->lock); -continue ; +if (spin_trylock (q->queue_lock)) { +ioc_exit_icq (icq); +spin_unlock (q->queue_lock); + }else { +spin_unlock_irqrestore (&ioc->lock, flags); +cpu_relax (); +spin_lock_irqsave_nested (&ioc->lock, flags,1 ); } -ioc_exit_icq (icq); - } - -if (last_q) { -spin_unlock (last_q->queue_lock); -spin_unlock_irq (&ioc->lock); -blk_put_queue (last_q); - }else { -spin_unlock_irq (&ioc->lock); } +spin_unlock_irqrestore (&ioc->lock, flags);kmem_cache_free (iocontext_cachep, ioc); }
Re: [PATCH] block: strip out locking optimization in put_io_context()
In reply to Tejun Heo2012/2/8 Tejun Heo <[email protected]>:
Quoted 9 lines
Hello,
Quoted 5 lines · Linus Torvalds
Yeah, please just get rid of the crazy code. Maybe *that* fixes the regression too, who knows?
For all we know, the "fast case" is what causes extra locking only to then fail and not even be a fast-path.
Quoted 7 lines
Yeah, I was about to ask Shaohua to test the version w/o optimization. With heavily loaded
request_queue, trylock failure could be frequent, which I wasn't testing.Shaohua, can you please test the version w/o optimization? Also, can you please give a bit more details on the setup? Are there multiple swap devices? Is it SSD or rotating disk?
the test adds mem=4G in a 2 sockets 16 CPU machine.
just make several copy of kernel source in tmpfs (so there is swap depending on your
memory size) and run kernel build in the kernel source in the meaning time. there is only one swap device which is a rotating disk.
I'll test both patches soon.
Thanks,
Shaohua
Re: [PATCH] block: strip out locking optimization in put_io_context()
2012/2/8 Shaohua Li <[email protected]>:
Quoted 17 lines
2012/2/8 Tejun Heo <[email protected]>:
Quoted 9 lines
Hello,
Quoted 5 lines · Linus Torvalds
Yeah, please just get rid of the crazy code. Maybe *that* fixes the regression too, who knows?
For all we know, the "fast case" is what causes extra locking only to then fail and not even be a fast-path.
Quoted 7 lines
Yeah, I was about to ask Shaohua to test the version w/o optimization. With heavily loaded
request_queue, trylock failure could be frequent, which I wasn't testing.Shaohua, can you please test the version w/o optimization? Also, can you please give a bit more details on the setup? Are there multiple swap devices? Is it SSD or rotating disk?
Quoted 7 lines
the test adds mem=4G in a 2 sockets 16 CPU machine.
just make several copy of kernel source in tmpfs (so there is swap depending on your
memory size) and run kernel build in the kernel source in the meaning time. there is only one swap device which is a rotating disk.I'll test both patches soon.
Tried all the options, the regression still exists. Any new idea? I'll spend some time on it if I can get anything
Thanks,
Shaohua
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Hello, Shaohua.
Quoted 10 lines · Shaohua Li
Quoted 7 lines
the test adds mem=4G in a 2 sockets 16 CPU machine.
just make several copy of kernel source in tmpfs (so there is swap depending on your
memory size) and run kernel build in the kernel source in the meaning time. there is only one swap device which is a rotating disk.I'll test both patches soon.
Tried all the options, the regression still exists. Any new idea? I'll spend some time on it if I can get anything
Can you please try the following one? Thanks a lot!
--- block/blk-cgroup.c | 2 block/blk-core.c | 2 block/blk-ioc.c | 99 +++++++++++++--------------------------------- block/cfq-iosched.c | 2 fs/ioprio.c | 2 include/linux/iocontext.h | 8 +-- kernel/fork.c | 2 7 files changed, 37 insertions(+), 80 deletions(-)
block/blk-ioc.c +29 -70
@@ -8,9 +8,12 @@#include <linux/blkdev.h> #include <linux/bootmem.h> /* for max_pfn/max_low_pfn */ #include <linux/slab.h> +#include <linux/rwlock.h> #include "blk.h" +static DEFINE_RWLOCK (ioc_clear_rwlock); +/* * For io context allocations */
@@ -45,7 +48,7 @@ static void ioc_exit_icq(struct io_cq *istruct request_queue *q = icq->q;struct elevator_type *et = q->elevator->type; -lockdep_assert_held (&ioc->lock); +//lockdep_assert_held(&ioc->lock); lockdep_assert_held (q->queue_lock);radix_tree_delete (&ioc->icq_tree, icq->q->id);
@@ -71,63 +74,6 @@ static void ioc_exit_icq(struct io_cq *icall_rcu (&icq->__rcu_head, icq_free_icq_rcu); } -/* -* Slow path for ioc release in put_io_context(). Performs double-lock -* dancing to unlink all icq's and then frees ioc. -*/ -static void ioc_release_fn (struct work_struct *work) -{ - struct io_context *ioc = container_of(work, struct io_context, - release_work); -struct request_queue *last_q =NULL ; - -spin_lock_irq (&ioc->lock); - -while (!hlist_empty (&ioc->icq_list)) { -struct io_cq *icq =hlist_entry (ioc->icq_list.first, - struct io_cq, ioc_node); -struct request_queue *this_q = icq->q; - -if (this_q != last_q) { -/* -* Need to switch to @this_q. Once we release -* @ioc->lock, it can go away along with @cic. -* Hold on to it. -*/ -__blk_get_queue (this_q); - -/* -* blk_put_queue() might sleep thanks to kobject -* idiocy. Always release both locks, put and -* restart. -*/ -if (last_q) { -spin_unlock (last_q->queue_lock); -spin_unlock_irq (&ioc->lock); -blk_put_queue (last_q); - }else { -spin_unlock_irq (&ioc->lock); - } - - last_q = this_q; -spin_lock_irq (this_q->queue_lock); -spin_lock (&ioc->lock); -continue ; - } -ioc_exit_icq (icq); - } - -if (last_q) { -spin_unlock (last_q->queue_lock); -spin_unlock_irq (&ioc->lock); -blk_put_queue (last_q); - }else { -spin_unlock_irq (&ioc->lock); - } - -kmem_cache_free (iocontext_cachep, ioc); -} -/** * put_io_context - put a reference of io_context * @ioc: io_context to put
@@ -135,7 +81,7 @@ static void ioc_release_fn(struct work_s* Decrement reference count of @ioc and release it if the count reaches * zero. */ -void put_io_context (struct io_context *ioc) +void put_io_context (struct io_context *ioc,struct request_queue *locked_q) {unsigned long flags;
@@ -144,16 +90,26 @@ void put_io_context(struct io_context *iBUG_ON (atomic_long_read (&ioc->refcount) <=0 ); -/* -* Releasing ioc requires reverse order double locking and we may -* already be holding a queue_lock. Do it asynchronously from wq. -*/ -if (atomic_long_dec_and_test (&ioc->refcount)) { -spin_lock_irqsave (&ioc->lock, flags); -if (!hlist_empty (&ioc->icq_list)) -schedule_work (&ioc->release_work); -spin_unlock_irqrestore (&ioc->lock, flags); +if (!atomic_long_dec_and_test (&ioc->refcount)) +return ; + +read_lock_irqsave (&ioc_clear_rwlock, flags); + +while (!hlist_empty (&ioc->icq_list)) { +struct io_cq *icq =hlist_entry (ioc->icq_list.first, + struct io_cq, ioc_node); +struct request_queue *q = icq->q; + +if (q != locked_q) +spin_lock_nested (q->queue_lock,1 ); + +ioc_exit_icq (icq); + +if (q != locked_q) +spin_unlock (q->queue_lock); } + +read_unlock_irqrestore (&ioc_clear_rwlock, flags); }EXPORT_SYMBOL (put_io_context);
@@ -168,7 +124,7 @@ void exit_io_context(struct task_structtask_unlock (task);atomic_dec (&ioc->nr_tasks); -put_io_context (ioc); +put_io_context (ioc,NULL ); }/**
@@ -181,6 +137,8 @@ void ioc_clear_queue(struct request_queu {lockdep_assert_held (q->queue_lock); +write_lock (&ioc_clear_rwlock); +while (!list_empty (&q->icq_list)) {struct io_cq *icq =list_entry (q->icq_list.next, struct io_cq, q_node);
@@ -190,6 +148,8 @@ void ioc_clear_queue(struct request_queuioc_exit_icq (icq);spin_unlock (&ioc->lock); } + +write_unlock (&ioc_clear_rwlock); } void create_io_context_slowpath(struct task_struct *task, gfp_t gfp_flags,
@@ -208,7 +168,6 @@ void create_io_context_slowpath(struct tspin_lock_init (&ioc->lock);INIT_RADIX_TREE (&ioc->icq_tree,GFP_ATOMIC | __GFP_HIGH);INIT_HLIST_HEAD (&ioc->icq_list); -INIT_WORK (&ioc->release_work, ioc_release_fn);/* * Try to install. ioc shouldn't be installed if someone else
include/linux/iocontext.h +3 -5
@@ -3,7 +3,6 @@#include <linux/radix-tree.h> #include <linux/rcupdate.h> -#include <linux/workqueue.h> enum {ICQ_IOPRIO_CHANGED ,
@@ -113,8 +112,6 @@ struct io_context {struct radix_tree_root icq_tree;struct io_cq __rcu *icq_hint;struct hlist_head icq_list; - -struct work_struct release_work; };static inline struct io_context *ioc_task_link (struct io_context *ioc)
@@ -133,7 +130,7 @@ static inline struct io_context *ioc_tasstruct task_struct ;#ifdef CONFIG_BLOCK -void put_io_context (struct io_context *ioc); +void put_io_context (struct io_context *ioc,struct request_queue *locked_q);void exit_io_context (struct task_struct *task);struct io_context *get_task_io_context (struct task_struct *task,gfp_t gfp_flags,int node);
@@ -141,7 +138,8 @@ void ioc_ioprio_changed(struct io_contexvoid ioc_cgroup_changed (struct io_context *ioc);#else struct io_context ; -static inline void put_io_context (struct io_context *ioc) { } +static inline void put_io_context (struct io_context *ioc, +struct request_queue *locked_q) { }static inline void exit_io_context (struct task_struct *task) { }#endif
block/blk-cgroup.c +1 -1
@@ -1659,7 +1659,7 @@ static void blkiocg_attach(struct cgroup ioc =get_task_io_context (task,GFP_ATOMIC ,NUMA_NO_NODE );if (ioc) {ioc_cgroup_changed (ioc); -put_io_context (ioc); +put_io_context (ioc,NULL ); } } }
block/blk-core.c +1 -1
@@ -642,7 +642,7 @@ static inline void blk_free_request(struif (rq->cmd_flags &REQ_ELVPRIV ) {elv_put_request (q, rq);if (rq->elv.icq) -put_io_context (rq->elv.icq->ioc); +put_io_context (rq->elv.icq->ioc, q); }mempool_free (rq, q->rq.rq_pool);
block/cfq-iosched.c +1 -1
@@ -1787,7 +1787,7 @@ __cfq_slice_expired(struct cfq_data *cfq cfqd->active_queue =NULL ;if (cfqd->active_cic) { -put_io_context (cfqd->active_cic->icq.ioc); +put_io_context (cfqd->active_cic->icq.ioc, cfqd->queue); cfqd->active_cic =NULL ; } }
fs/ioprio.c +1 -1
@@ -51,7 +51,7 @@ int set_task_ioprio(struct task_struct * ioc =get_task_io_context (task,GFP_ATOMIC ,NUMA_NO_NODE );if (ioc) {ioc_ioprio_changed (ioc, ioprio); -put_io_context (ioc); +put_io_context (ioc,NULL ); }return err;
kernel/fork.c +1 -1
@@ -890,7 +890,7 @@ static int copy_io(unsigned long clone_freturn -ENOMEM ; new_ioc->ioprio = ioc->ioprio; - put_io_context(new_ioc); + put_io_context(new_ioc, NULL); }#endif return 0 ;
Re: [PATCH] block: strip out locking optimization in put_io_context()
Quoted 2 lines · Tejun Heo
Can you please try the following one? Thanks a lot!
If you can use it as a rwlock, why can't you do it with RCU?
Usually rwlocks are a bad idea. They tend to be more expensive than spinlocks, and the extra parallelism is almost never noticeable (except as "more cacheline bounces") for something that is appropriate for a non-sleeping lock.
There's a *very* few situations where rwlock is the right thing, but it really almost always is a horribly bad idea.
Linus
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Hello, Linus.
Quoted 5 lines · Linus Torvalds
Quoted 2 lines · Tejun Heo
Can you please try the following one? Thanks a lot!
If you can use it as a rwlock, why can't you do it with RCU?
The original locking scheme was using RCU which was very fragile and broken on corner cases. The locking restructuring was aimed to make things simpler. While the double locking isn't trivial, it's much easier to grasp and get right than RCU. We might have to revive RCU if the regression can't be tackled otherwise and it probably is possible to do it simpler. Let's see.
Quoted 7 lines
Usually rwlocks are a bad idea. They tend to be more expensive than spinlocks, and the extra parallelism is almost never noticeable (except as "more cacheline bounces") for something that is appropriate for a non-sleeping lock.
There's a *very* few situations where rwlock is the right thing, but it really almost always is a horribly bad idea.
I'm still a bit lost on where the regression is coming from and *suspecting* that queue_lock contention is making the reverse locking behave much worse than expected, so I mostly wanted to take that out and see what happens. rwlock might increase locking overhead per try but it avoids unlock/lock dancing. I'll try to reproduce the regression in a few days and do better analysis.
Thanks.
Signature · 1 line
tejun
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Quoted 4 lines · Tejun Heo
I'm still a bit lost on where the regression is coming from and *suspecting* that
queue_lockcontention is making the reverse locking behave much worse than expected, so I mostly wanted to take that out and see what happens.
IOW, we can achieve about the same thing by adding another lock in request_queue. The goal is using an inner lock for ioc clearing so that queue_lock doesn't have to be grabbed inside ioc lock.
Thanks.
Signature · 1 line
tejun
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Quoted 9 lines · Tejun Heo
Quoted 4 lines · Tejun Heo
I'm still a bit lost on where the regression is coming from and *suspecting* that
queue_lockcontention is making the reverse locking behave much worse than expected, so I mostly wanted to take that out and see what happens.IOW, we can achieve about the same thing by adding another lock in
request_queue. The goal is using an inner lock for ioc clearing so thatqueue_lockdoesn't have to be grabbed inside ioc lock.
Urgh....... forget about the above message. My head is still booting. We can't do this w/ per-queue lock as we don't have a way to traverse the associated queues w/o holding the lock and we need read locking on ioc exit path as we may recurse through elevator icq exit (it's not about concurrency). /me goes to brew coffee.
Signature · 1 line
tejun
Re: [PATCH] block: strip out locking optimization in put_io_context()
In reply to Tejun Heo2012/2/9 Tejun Heo <[email protected]>:
Quoted 9 lines
Hello, Shaohua.
Quoted 6 lines · Shaohua Li
Quoted 6 lines
the test adds mem=4G in a 2 sockets 16 CPU machine.
just make several copy of kernel source in tmpfs (so there is swap depending on your
memory size) and run kernel build in the kernel source in the meaning time. there is only one swap device which is a rotating disk.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
I'll test both patches soon.
Tried all the options, the regression still exists. Any new idea? I'll spend some time on it if I can get anything
Can you please try the following one? Thanks a lot!
doesn't work.
I also double confirmed b2efa05265d62 causes the regression
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Hello,
Quoted 6 lines · Shaohua Li
Quoted 4 lines
Quoted 2 lines
Tried all the options, the regression still exists. Any new idea? I'll spend some time on it if I can get anything
Can you please try the following one? Thanks a lot!
doesn't work.
I also double confirmedb2efa05265d62causes the regression
I'll soon send a RCU based version. I'm still having trouble reproducing the regression tho. I've tried a few different things.
* Heavy thrashing: Disk IO dominates everything and CPUs aren't too busy. While how swap behaves affects completion time, I don't see how CPU locking issue comes into play at all under this circumstance.
* Some swap load: Simulated w/ 1G memory limit and buliding defconfig kernel in tmpfs. Swap grows to a couple hundred megabytes but build time is still dominated by CPU. I didn't see any meanginful difference before and after the commit - both in terms of wallclock and CPU times.
Maybe these two were too extreme to show the problem and I need to push memory limit a bit further, but it would be great if you can give me a bit more details about your testing.
* How much memory does the machine have? How is the tmpfs setup and filled up? What's the size of the tmpfs and the output of "free -m" right before test starts? While the test is running, how occupied are the CPUs? On test completion, what's the output of "free -m"?
* What exactly is the test and what do you measure? What does "12% regression" mean? Is it wallclock time or CPU time? If it's CPU time, does systime increase dominate the regression?
Thanks.
Signature · 1 line
tejun
Re: [PATCH] block: strip out locking optimization in put_io_context()
Quoted 4 lines · Tejun Heo
* What exactly is the test and what do you measure? What does "12% regression" mean? Is it wallclock time or CPU time? If it's CPU time, does systime increase dominate the regression?
Shaohua, it might be interesting to see a profile of the bad case.
Now, quite often these kinds of things don't show anything at all - it's just due to cache issues and there's no obvious "we hold spinlock X for 15 seconds total". But if it's actual lock contention rather than just "more scheduling of worker threads", it should show up in the profile quite clearly.
That said, I do think the RCU approach is the right one. The whole delayed deallocation (and the replacement patch with rwlocks) really smells like "badly done RCU-like behavior" to me.
Linus
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Quoted 7 lines · Linus Torvalds
Quoted 4 lines · Tejun Heo
* What exactly is the test and what do you measure? What does "12% regression" mean? Is it wallclock time or CPU time? If it's CPU time, does systime increase dominate the regression?
Shaohua, it might be interesting to see a profile of the bad case.
Yeap, if CPUs are taking more time to do stuff, it would be helpful to obtain before and after profiles.
Quoted 5 lines
Now, quite often these kinds of things don't show anything at all - it's just due to cache issues and there's no obvious "we hold spinlock X for 15 seconds total". But if it's actual lock contention rather than just "more scheduling of worker threads", it should show up in the profile quite clearly.
Weird thing is that if it were wq, the rwlock patch should have removed the regression. It removed the reverse locking and the wq deferring. It does replace ioc locking with global readlock but it's weird that that can show up as >10% regression (whatever the measure may be). Even though kernel compiling is pretty fork/exit intensive....
Quoted 3 lines
That said, I do think the RCU approach is the right one. The whole delayed deallocation (and the replacement patch with rwlocks) really smells like "badly done RCU-like behavior" to me.
I'll probably post it in several hours and think it's gonna be pretty well contained. I probably avoided RCU too hard in that path from the original scary RCU usage.
Thanks.
Signature · 1 line
tejun
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Hello, Shaohua.
Can you please test the following one? It's probably the simplest version w/o RCU and wq deferring. RCUfying isn't too bad but I'm still a bit hesitant because RCU coverage needs to be extended to request_queue via conditional synchronize_rcu() in queue exit path (can't enforce delayed RCU free on request_queues and unconditional synchronize_rcu() may cause excessive delay during boot for certain configurations). It now can be done in the block core layer proper so it shouldn't be as bad tho. If this too flops, I'll get to that.
Thanks.
--- block/blk-cgroup.c | 2 block/blk-core.c | 2 block/blk-ioc.c | 112 +++++++++++++++++----------------------------- block/cfq-iosched.c | 2 fs/ioprio.c | 2 include/linux/blkdev.h | 3 + include/linux/iocontext.h | 8 +-- kernel/fork.c | 2 8 files changed, 54 insertions(+), 79 deletions(-)
block/blk-cgroup.c +1 -1
@@ -1659,7 +1659,7 @@ static void blkiocg_attach(struct cgroup ioc =get_task_io_context (task,GFP_ATOMIC ,NUMA_NO_NODE );if (ioc) {ioc_cgroup_changed (ioc); -put_io_context (ioc); +put_io_context (ioc,NULL ); } } }
block/blk-core.c +1 -1
@@ -642,7 +642,7 @@ static inline void blk_free_request(struif (rq->cmd_flags &REQ_ELVPRIV ) {elv_put_request (q, rq);if (rq->elv.icq) -put_io_context (rq->elv.icq->ioc); +put_io_context (rq->elv.icq->ioc, q); }mempool_free (rq, q->rq.rq_pool);
block/blk-ioc.c +43 -69
149 lines not on this page · raw message
block/cfq-iosched.c +1 -1
@@ -1787,7 +1787,7 @@ __cfq_slice_expired(struct cfq_data *cfq cfqd->active_queue =NULL ;if (cfqd->active_cic) { -put_io_context (cfqd->active_cic->icq.ioc); +put_io_context (cfqd->active_cic->icq.ioc, cfqd->queue); cfqd->active_cic =NULL ; } }
fs/ioprio.c +1 -1
@@ -51,7 +51,7 @@ int set_task_ioprio(struct task_struct * ioc =get_task_io_context (task,GFP_ATOMIC ,NUMA_NO_NODE );if (ioc) {ioc_ioprio_changed (ioc, ioprio); -put_io_context (ioc); +put_io_context (ioc,NULL ); }return err;
include/linux/iocontext.h +3 -5
@@ -3,7 +3,6 @@#include <linux/radix-tree.h> #include <linux/rcupdate.h> -#include <linux/workqueue.h> enum {ICQ_IOPRIO_CHANGED ,
@@ -113,8 +112,6 @@ struct io_context {struct radix_tree_root icq_tree;struct io_cq __rcu *icq_hint;struct hlist_head icq_list; - -struct work_struct release_work; };static inline struct io_context *ioc_task_link (struct io_context *ioc)
@@ -133,7 +130,7 @@ static inline struct io_context *ioc_tasstruct task_struct ;#ifdef CONFIG_BLOCK -void put_io_context (struct io_context *ioc); +void put_io_context (struct io_context *ioc,struct request_queue *locked_q);void exit_io_context (struct task_struct *task);struct io_context *get_task_io_context (struct task_struct *task,gfp_t gfp_flags,int node);
@@ -141,7 +138,8 @@ void ioc_ioprio_changed(struct io_contexvoid ioc_cgroup_changed (struct io_context *ioc);#else struct io_context ; -static inline void put_io_context (struct io_context *ioc) { } +static inline void put_io_context (struct io_context *ioc, +struct request_queue *locked_q) { }static inline void exit_io_context (struct task_struct *task) { }#endif
kernel/fork.c +1 -1
@@ -890,7 +890,7 @@ static int copy_io(unsigned long clone_freturn -ENOMEM ; new_ioc->ioprio = ioc->ioprio; - put_io_context(new_ioc); + put_io_context(new_ioc, NULL); }#endif return 0 ;
include/linux/blkdev.h +3 -0
@@ -399,6 +399,9 @@ struct request_queue {/* Throttle data */ struct throtl_data *td;#endif +#ifdef CONFIG_LOCKDEP +int ioc_release_depth; +#endif };#define QUEUE_FLAG_QUEUED 1/* uses generic tag queueing */
Re: [PATCH] block: strip out locking optimization in put_io_context()
2012/2/10 Tejun Heo <[email protected]>:
Quoted 10 lines
Hello, Shaohua.
Can you please test the following one? It's probably the simplest version w/o RCU and wq deferring. RCUfying isn't too bad but I'm still a bit hesitant because RCU coverage needs to be extended to
request_queuevia conditionalsynchronize_rcu()in queue exit path (can't enforce delayed RCU free onrequest_queuesand unconditionalsynchronize_rcu()may cause excessive delay during boot for certain configurations). It now can be done in the block core layer proper so it shouldn't be as bad tho. If this too flops, I'll get to that.
doesn't work.
Re: [PATCH] block: strip out locking optimization in put_io_context()
2012/2/10 Shaohua Li <[email protected]>:
Quoted 11 lines
2012/2/10 Tejun Heo <[email protected]>:
Quoted 10 lines
Hello, Shaohua.
Can you please test the following one? It's probably the simplest version w/o RCU and wq deferring. RCUfying isn't too bad but I'm still a bit hesitant because RCU coverage needs to be extended to
request_queuevia conditionalsynchronize_rcu()in queue exit path (can't enforce delayed RCU free onrequest_queuesand unconditionalsynchronize_rcu()may cause excessive delay during boot for certain configurations). It now can be done in the block core layer proper so it shouldn't be as bad tho. If this too flops, I'll get to that.
Quoted 1 line
doesn't work.
I added trace in the schedule_work code path of put_io_context, which runs very rare. So it's not lock contention for sure. Sounds the only difference between the good/bad cases is the good case runs with rcu_lock_read/rcu_read_unlock. I also checked slab info, the cfq related slab doesn't use too many memory, unlikely because rcu latency uses too many memory.
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Hello,
Quoted 8 lines · Shaohua Li
Quoted 8 lines
Quoted 8 lines
Can you please test the following one? It's probably the simplest version w/o RCU and wq deferring. RCUfying isn't too bad but I'm still a bit hesitant because RCU coverage needs to be extended to
request_queuevia conditionalsynchronize_rcu()in queue exit path (can't enforce delayed RCU free onrequest_queuesand unconditionalsynchronize_rcu()may cause excessive delay during boot for certain configurations). It now can be done in the block core layer proper so it shouldn't be as bad tho. If this too flops, I'll get to that.
Quoted 7 lines
Quoted 1 line
doesn't work.
I added trace in the
schedule_workcode path ofput_io_context, which runs very rare. So it's not lock contention for sure. Sounds the only difference between the good/bad cases is the good case runs with rcu_lock_read/rcu_read_unlock. I also checked slab info, the cfq related slab doesn't use too many memory, unlikely because rcu latency uses too many memory.
Yeah, that makes much more sense. It just isn't hot enough path for this sort of micro locking changes to matter. I think the problem is that, after the change, the cfqq aren't being expired immediately on task exit. ie. While moving the cic destruction to release path, I accidentally removed exit notification to cfq. I'll come up with a fix.
Thank you!
Signature · 1 line
tejun
Jens Axboe
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Quoted 19 lines · Tejun Heo
Hello,
Quoted 8 lines · Shaohua Li
Quoted 8 lines
Quoted 8 lines
Can you please test the following one? It's probably the simplest version w/o RCU and wq deferring. RCUfying isn't too bad but I'm still a bit hesitant because RCU coverage needs to be extended to
request_queuevia conditionalsynchronize_rcu()in queue exit path (can't enforce delayed RCU free onrequest_queuesand unconditionalsynchronize_rcu()may cause excessive delay during boot for certain configurations). It now can be done in the block core layer proper so it shouldn't be as bad tho. If this too flops, I'll get to that.Quoted 7 lines
Quoted 1 line
doesn't work.
I added trace in the
schedule_workcode path ofput_io_context, which runs very rare. So it's not lock contention for sure. Sounds the only difference between the good/bad cases is the good case runs with rcu_lock_read/rcu_read_unlock. I also checked slab info, the cfq related slab doesn't use too many memory, unlikely because rcu latency uses too many memory.
Quoted 6 lines
Yeah, that makes much more sense. It just isn't hot enough path for this sort of micro locking changes to matter. I think the problem is that, after the change, the cfqq aren't being expired immediately on task exit. ie. While moving the cic destruction to release path, I accidentally removed exit notification to cfq. I'll come up with a fix.
Was just thinking about that last night, the missing slice expire on task exit makes a LOT more sense than changed locking.
I'm pushing off what I have to Linus today, since I'll be gone skiing next week. I will check email regularly and be able to apply patches and so forth, just a heads up on availability.
Signature · 1 line
Jens Axboe
Re: [PATCH] block: strip out locking optimization in put_io_context()
In reply to Tejun Heo2012/2/11 Tejun Heo <[email protected]>:
Quoted 19 lines
Hello,
Quoted 8 lines · Shaohua Li
Quoted 8 lines
Quoted 8 lines
Can you please test the following one? It's probably the simplest version w/o RCU and wq deferring. RCUfying isn't too bad but I'm still a bit hesitant because RCU coverage needs to be extended to
request_queuevia conditionalsynchronize_rcu()in queue exit path (can't enforce delayed RCU free onrequest_queuesand unconditionalsynchronize_rcu()may cause excessive delay during boot for certain configurations). It now can be done in the block core layer proper so it shouldn't be as bad tho. If this too flops, I'll get to that.Quoted 7 lines
Quoted 1 line
doesn't work.
I added trace in the
schedule_workcode path ofput_io_context, which runs very rare. So it's not lock contention for sure. Sounds the only difference between the good/bad cases is the good case runs with rcu_lock_read/rcu_read_unlock. I also checked slab info, the cfq related slab doesn't use too many memory, unlikely because rcu latency uses too many memory.
Quoted 6 lines
Yeah, that makes much more sense. It just isn't hot enough path for this sort of micro locking changes to matter. I think the problem is that, after the change, the cfqq aren't being expired immediately on task exit. ie. While moving the cic destruction to release path, I accidentally removed exit notification to cfq. I'll come up with a fix.
ah, I felt a little strange looking at exit_io_context, but didn't realize the exit notification is removed (confused by put_io_context). This makes a lot of sense. Good catch!
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Hello, Shaohua.
Can you please test the following patch? It's combination of three patches which invokes elevator icq exit from exit_io_context(). This unfortunately ends up adding another reverse locking loop and using RCU could be better; unfortunately, the change isn't trivial due to q->queue_lock modification during blk_cleanup_queue() and ioc cleanup being called after that from blk_release_queue() - IOW, while holding RCU, we might end up grabbing the wrong q lock (I don't think this is a new problem).
Now that we have proper request draining on queue exit, we can probably move ioc clearing and other operations to blk_cleanup_queue() and then apply RCU, but that's for another merge window.
Thanks.
block/blk-ioc.c +116 -88
@@ -29,6 +29,21 @@ void get_io_context(struct io_context *i }EXPORT_SYMBOL (get_io_context); +/* +* Exiting ioc may nest into another put_io_context() leading to nested +* fast path release. As the ioc's can't be the same, this is okay but +* makes lockdep whine. Keep track of nesting and use it as subclass. +*/ +#ifdef CONFIG_LOCKDEP +#define ioc_exit_depth (q) ((q) ? (q)->ioc_exit_depth : 0) +#define ioc_exit_depth_inc (q) (q)->ioc_exit_depth++ +#define ioc_exit_depth_dec (q) (q)->ioc_exit_depth-- +#else +#define ioc_exit_depth (q) 0 +#define ioc_exit_depth_inc (q) do { } while (0) +#define ioc_exit_depth_dec (q) do { } while (0) +#endif +static void icq_free_icq_rcu (struct rcu_head *head) { struct io_cq *icq = container_of(head, struct io_cq, __rcu_head);
@@ -36,17 +51,31 @@ static void icq_free_icq_rcu(struct rcu_kmem_cache_free (icq->__rcu_icq_cache, icq); } -/* -* Exit and free an icq. Called with both ioc and q locked. -*/ +/* Exit an icq. Called with both ioc and q locked. */ static void ioc_exit_icq (struct io_cq *icq) { +struct elevator_type *et = icq->q->elevator->type; + +if (icq->flags &ICQ_EXITED ) +return ; + +if (et->ops.elevator_exit_icq_fn) { +ioc_exit_depth_inc (icq->q); + et->ops.elevator_exit_icq_fn (icq); +ioc_exit_depth_dec (icq->q); + } + + icq->flags |=ICQ_EXITED ; +} + +/* Release an icq. Called with both ioc and q locked. */ +static void ioc_destroy_icq (struct io_cq *icq) +{struct io_context *ioc = icq->ioc; -struct request_queue *q = icq->q; -struct elevator_type *et = q->elevator->type; +struct elevator_type *et = icq->q->elevator->type;lockdep_assert_held (&ioc->lock); -lockdep_assert_held (q->queue_lock); +lockdep_assert_held (icq->q->queue_lock);radix_tree_delete (&ioc->icq_tree, icq->q->id);hlist_del_init (&icq->ioc_node);
@@ -60,8 +89,7 @@ static void ioc_exit_icq(struct io_cq *iif (rcu_dereference_raw (ioc->icq_hint) == icq)rcu_assign_pointer (ioc->icq_hint,NULL ); -if (et->ops.elevator_exit_icq_fn) - et->ops.elevator_exit_icq_fn (icq); +ioc_exit_icq (icq);/* * @icq->q might have gone away by the time RCU callback runs
@@ -71,70 +99,6 @@ static void ioc_exit_icq(struct io_cq *icall_rcu (&icq->__rcu_head, icq_free_icq_rcu); } -/* -* Slow path for ioc release in put_io_context(). Performs double-lock -* dancing to unlink all icq's and then frees ioc. -*/ -static void ioc_release_fn (struct work_struct *work) -{ - struct io_context *ioc = container_of(work, struct io_context, - release_work); -struct request_queue *last_q =NULL ; -unsigned long flags; - -/* -* Exiting icq may call into put_io_context() through elevator -* which will trigger lockdep warning. The ioc's are guaranteed to -* be different, use a different locking subclass here. Use -* irqsave variant as there's no spin_lock_irq_nested(). -*/ -spin_lock_irqsave_nested (&ioc->lock, flags,1 ); - -while (!hlist_empty (&ioc->icq_list)) { -struct io_cq *icq =hlist_entry (ioc->icq_list.first, - struct io_cq, ioc_node); -struct request_queue *this_q = icq->q; - -if (this_q != last_q) { -/* -* Need to switch to @this_q. Once we release -* @ioc->lock, it can go away along with @cic. -* Hold on to it. -*/ -__blk_get_queue (this_q); - -/* -* blk_put_queue() might sleep thanks to kobject -* idiocy. Always release both locks, put and -* restart. -*/ -if (last_q) { -spin_unlock (last_q->queue_lock); -spin_unlock_irqrestore (&ioc->lock, flags); -blk_put_queue (last_q); - }else { -spin_unlock_irqrestore (&ioc->lock, flags); - } - - last_q = this_q; -spin_lock_irqsave (this_q->queue_lock, flags); -spin_lock_nested (&ioc->lock,1 ); -continue ; - } -ioc_exit_icq (icq); - } - -if (last_q) { -spin_unlock (last_q->queue_lock); -spin_unlock_irqrestore (&ioc->lock, flags); -blk_put_queue (last_q); - }else { -spin_unlock_irqrestore (&ioc->lock, flags); - } - -kmem_cache_free (iocontext_cachep, ioc); -} -/** * put_io_context - put a reference of io_context * @ioc: io_context to put
@@ -142,7 +106,7 @@ static void ioc_release_fn(struct work_s* Decrement reference count of @ioc and release it if the count reaches * zero. */ -void put_io_context (struct io_context *ioc) +void put_io_context (struct io_context *ioc,struct request_queue *locked_q) {unsigned long flags;
@@ -151,16 +115,33 @@ void put_io_context(struct io_context *iBUG_ON (atomic_long_read (&ioc->refcount) <=0 ); +if (!atomic_long_dec_and_test (&ioc->refcount)) +return ; +/* -* Releasing ioc requires reverse order double locking and we may -* already be holding a queue_lock. Do it asynchronously from wq. +* Need to grab both q and ioc locks from ioc side to release all +* icqs. Perform reverse double locking. */ -if (atomic_long_dec_and_test (&ioc->refcount)) { -spin_lock_irqsave (&ioc->lock, flags); -if (!hlist_empty (&ioc->icq_list)) -schedule_work (&ioc->release_work); -spin_unlock_irqrestore (&ioc->lock, flags); +spin_lock_irqsave_nested (&ioc->lock, flags,ioc_exit_depth (locked_q)); + +while (!hlist_empty (&ioc->icq_list)) { +struct io_cq *icq =hlist_entry (ioc->icq_list.first, + struct io_cq, ioc_node); +struct request_queue *q = icq->q; + +if (q == locked_q ||spin_trylock (q->queue_lock)) { +ioc_destroy_icq (icq); +if (q != locked_q) +spin_unlock (q->queue_lock); + }else { +spin_unlock_irqrestore (&ioc->lock, flags); +cpu_relax (); +spin_lock_irqsave_nested (&ioc->lock, flags, +ioc_exit_depth (locked_q)); + } } + +spin_unlock_irqrestore (&ioc->lock, flags); }EXPORT_SYMBOL (put_io_context);
@@ -168,14 +149,40 @@ EXPORT_SYMBOL(put_io_context);void exit_io_context (struct task_struct *task) {struct io_context *ioc; +struct io_cq *icq; +struct hlist_node *n;task_lock (task); ioc = task->io_context; task->io_context =NULL ;task_unlock (task); -atomic_dec (&ioc->nr_tasks); -put_io_context (ioc); +if (!atomic_dec_and_test (&ioc->nr_tasks)) { +put_io_context (ioc,NULL ); +return ; + } + +/* +* Need ioc lock to walk icq_list and q lock to exit icq. Perform +* reverse double locking. +*/ +retry: +spin_lock_irq (&ioc->lock); +hlist_for_each_entry (icq, n, &ioc->icq_list, ioc_node) { +if (icq->flags &ICQ_EXITED ) +continue ; +if (spin_trylock (icq->q->queue_lock)) { +ioc_exit_icq (icq); +spin_unlock (icq->q->queue_lock); + }else { +spin_unlock_irq (&ioc->lock); +cpu_relax (); + goto retry; + } + } +spin_unlock_irq (&ioc->lock); + +put_io_context (ioc,NULL ); }/**
@@ -194,7 +201,7 @@ void ioc_clear_queue(struct request_queustruct io_context *ioc = icq->ioc;spin_lock (&ioc->lock); -ioc_exit_icq (icq); +ioc_destroy_icq (icq);spin_unlock (&ioc->lock); } }
@@ -215,7 +222,6 @@ void create_io_context_slowpath(struct tspin_lock_init (&ioc->lock);INIT_RADIX_TREE (&ioc->icq_tree,GFP_ATOMIC | __GFP_HIGH);INIT_HLIST_HEAD (&ioc->icq_list); -INIT_WORK (&ioc->release_work, ioc_release_fn);/* * Try to install. ioc shouldn't be installed if someone else
@@ -363,13 +369,13 @@ struct io_cq *ioc_create_icq(struct requreturn icq; } -void ioc_set_changed (struct io_context *ioc,int which) +void ioc_set_icq_flags (struct io_context *ioc,unsigned int flags) {struct io_cq *icq;struct hlist_node *n;hlist_for_each_entry (icq, n, &ioc->icq_list, ioc_node) -set_bit (which, &icq->changed); + icq->flags |= flags; }/**
@@ -387,7 +393,7 @@ void ioc_ioprio_changed(struct io_contexspin_lock_irqsave (&ioc->lock, flags); ioc->ioprio = ioprio; -ioc_set_changed (ioc,ICQ_IOPRIO_CHANGED ); +ioc_set_icq_flags (ioc,ICQ_IOPRIO_CHANGED );spin_unlock_irqrestore (&ioc->lock, flags); }
@@ -404,11 +410,33 @@ void ioc_cgroup_changed(struct io_contexunsigned long flags;spin_lock_irqsave (&ioc->lock, flags); -ioc_set_changed (ioc,ICQ_CGROUP_CHANGED ); +ioc_set_icq_flags (ioc,ICQ_CGROUP_CHANGED );spin_unlock_irqrestore (&ioc->lock, flags); }EXPORT_SYMBOL (ioc_cgroup_changed); +/** +* icq_get_changed - fetch and clear icq changed mask +* @icq: icq of interest +* +* Fetch and clear ICQ_*_CHANGED bits from @icq. Grabs and releases +* @icq->ioc->lock. +*/ +unsigned icq_get_changed (struct io_cq *icq) +{ +unsigned int changed =0 ; +unsigned long flags; + +if (unlikely (icq->flags &ICQ_CHANGED_MASK )) { +spin_lock_irqsave (&icq->ioc->lock, flags); + changed = icq->flags &ICQ_CHANGED_MASK ; + icq->flags &= ~ICQ_CHANGED_MASK ; +spin_unlock_irqrestore (&icq->ioc->lock, flags); + } +return changed; +} +EXPORT_SYMBOL (icq_get_changed); +static int __init blk_ioc_init (void ) { iocontext_cachep = kmem_cache_create("blkdev_ioc",
block/cfq-iosched.c +7 -7
@@ -1787,7 +1787,7 @@ __cfq_slice_expired(struct cfq_data *cfq cfqd->active_queue =NULL ;if (cfqd->active_cic) { -put_io_context (cfqd->active_cic->icq.ioc); +put_io_context (cfqd->active_cic->icq.ioc, cfqd->queue); cfqd->active_cic =NULL ; } }
@@ -3470,20 +3470,20 @@ cfq_set_request(struct request_queue *q,const int rw =rq_data_dir (rq);const bool is_sync =rq_is_sync (rq);struct cfq_queue *cfqq; +unsigned int changed;might_sleep_if (gfp_mask & __GFP_WAIT);spin_lock_irq (q->queue_lock);/* handle changed notifications */ -if (unlikely (cic->icq.changed)) { -if (test_and_clear_bit (ICQ_IOPRIO_CHANGED , &cic->icq.changed)) -changed_ioprio (cic); + changed =icq_get_changed (&cic->icq); +if (unlikely (changed &ICQ_IOPRIO_CHANGED )) +changed_ioprio (cic);#ifdef CONFIG_CFQ_GROUP_IOSCHED -if (test_and_clear_bit (ICQ_CGROUP_CHANGED , &cic->icq.changed)) -changed_cgroup (cic); +if (unlikely (changed &ICQ_CGROUP_CHANGED )) +changed_cgroup (cic);#endif - } new_queue: cfqq =cic_to_cfqq (cic, is_sync);
include/linux/iocontext.h +11 -8
@@ -3,11 +3,13 @@#include <linux/radix-tree.h> #include <linux/rcupdate.h> -#include <linux/workqueue.h> enum { -ICQ_IOPRIO_CHANGED , -ICQ_CGROUP_CHANGED , +ICQ_IOPRIO_CHANGED =1 <<0 , +ICQ_CGROUP_CHANGED =1 <<1 , +ICQ_EXITED =1 <<2 , + +ICQ_CHANGED_MASK =ICQ_IOPRIO_CHANGED |ICQ_CGROUP_CHANGED , };/*
@@ -88,7 +90,8 @@ struct io_cq {struct rcu_head __rcu_head; }; -unsigned long changed; +/* ICQ_*, use atomic bitops */ +unsigned long flags; };/*
@@ -113,8 +116,6 @@ struct io_context {struct radix_tree_root icq_tree;struct io_cq __rcu *icq_hint;struct hlist_head icq_list; - -struct work_struct release_work; };static inline struct io_context *ioc_task_link (struct io_context *ioc)
@@ -133,15 +134,17 @@ static inline struct io_context *ioc_tasstruct task_struct ;#ifdef CONFIG_BLOCK -void put_io_context (struct io_context *ioc); +void put_io_context (struct io_context *ioc,struct request_queue *locked_q);void exit_io_context (struct task_struct *task);struct io_context *get_task_io_context (struct task_struct *task,gfp_t gfp_flags,int node);void ioc_ioprio_changed (struct io_context *ioc,int ioprio);void ioc_cgroup_changed (struct io_context *ioc); +unsigned int icq_get_changed (struct io_cq *icq);#else struct io_context ; -static inline void put_io_context (struct io_context *ioc) { } +static inline void put_io_context (struct io_context *ioc, +struct request_queue *locked_q) { }static inline void exit_io_context (struct task_struct *task) { }#endif
block/blk-cgroup.c +1 -1
@@ -1659,7 +1659,7 @@ static void blkiocg_attach(struct cgroup ioc =get_task_io_context (task,GFP_ATOMIC ,NUMA_NO_NODE );if (ioc) {ioc_cgroup_changed (ioc); -put_io_context (ioc); +put_io_context (ioc,NULL ); } } }
block/blk-core.c +1 -1
@@ -642,7 +642,7 @@ static inline void blk_free_request(struif (rq->cmd_flags &REQ_ELVPRIV ) {elv_put_request (q, rq);if (rq->elv.icq) -put_io_context (rq->elv.icq->ioc); +put_io_context (rq->elv.icq->ioc, q); }mempool_free (rq, q->rq.rq_pool);
fs/ioprio.c +1 -1
@@ -51,7 +51,7 @@ int set_task_ioprio(struct task_struct * ioc =get_task_io_context (task,GFP_ATOMIC ,NUMA_NO_NODE );if (ioc) {ioc_ioprio_changed (ioc, ioprio); -put_io_context (ioc); +put_io_context (ioc,NULL ); }return err;
kernel/fork.c +1 -1
@@ -910,7 +910,7 @@ static int copy_io(unsigned long clone_freturn -ENOMEM ; new_ioc->ioprio = ioc->ioprio; - put_io_context(new_ioc); + put_io_context(new_ioc, NULL); }#endif return 0 ;
include/linux/blkdev.h +3 -0
@@ -399,6 +399,9 @@ struct request_queue {/* Throttle data */ struct throtl_data *td;#endif +#ifdef CONFIG_LOCKDEP +int ioc_exit_depth; +#endif };#define QUEUE_FLAG_QUEUED 1/* uses generic tag queueing */
Re: [PATCH] block: strip out locking optimization in put_io_context()
2012/2/14 Tejun Heo <[email protected]>:
Quoted 11 lines
Hello, Shaohua.
Can you please test the following patch? It's combination of three patches which invokes elevator icq exit from
exit_io_context(). This unfortunately ends up adding another reverse locking loop and using RCU could be better; unfortunately, the change isn't trivial due toq->queue_lockmodification duringblk_cleanup_queue()and ioc cleanup being called after that fromblk_release_queue()- IOW, while holding RCU, we might end up grabbing the wrong q lock (I don't think this is a new problem).
Quoted 3 lines
Now that we have proper request draining on queue exit, we can probably move ioc clearing and other operations to
blk_cleanup_queue()and then apply RCU, but that's for another merge window.
This fixed the regression. Thanks!
Tejun Heo
maintainerRe: [PATCH] block: strip out locking optimization in put_io_context()
Hello,
Quoted 6 lines · Shaohua Li
2012/2/14 Tejun Heo <[email protected]>:
Quoted 3 lines
Now that we have proper request draining on queue exit, we can probably move ioc clearing and other operations to
blk_cleanup_queue()and then apply RCU, but that's for another merge window.This fixed the regression. Thanks!
Ah... that's a good news. I'll split up the patches and post them.
Thank you very much for the bisection and testing all the different patches!
Signature · 1 line
tejun
Re: [PATCH] block: strip out locking optimization in put_io_context()
In reply to Linus Torvalds2012/2/10 Linus Torvalds <[email protected]>:
Quoted 14 lines · Tejun Heo
Quoted 4 lines
* What exactly is the test and what do you measure? What does "12% regression" mean? Is it wallclock time or CPU time? If it's CPU time, does systime increase dominate the regression?
Shaohua, it might be interesting to see a profile of the bad case.
Now, quite often these kinds of things don't show anything at all - it's just due to cache issues and there's no obvious "we hold spinlock X for 15 seconds total". But if it's actual lock contention rather than just "more scheduling of worker threads", it should show up in the profile quite clearly.
Quoted 3 lines
That said, I do think the RCU approach is the right one. The whole delayed deallocation (and the replacement patch with rwlocks) really smells like "badly done RCU-like behavior" to me.
Appears not a lock contention issue. The system is quite idle, about 20% busy. And top shows no cpu is very busy. Before test runs, the system has about 2G free memory (from vmstat)
system and user time isn't changed. only real time becomes longer. This suggests IO is slower or there is more IO. But vmstat and iostat data doesn't show significant difference between the good and bad cases. There might be some access pattern changed which makes swap no efficient or working set is wrongly swaped out.
Tejun Heo
maintainerPATCH 1/1 block: fix lockdep warning on io_context release put_io_context()
In reply to Jens Axboe11a3122f6c "block: strip out locking optimization in put_io_context()" removed ioc_lock depth lockdep annoation along with locking optimization; however, while recursing from put_io_context() is no longer possible, ioc_release_fn() may still end up putting the last reference of another ioc through elevator, which wlil grab ioc->lock triggering spurious (as the ioc is always different one) A-A deadlock warning.
As this can only happen one time from ioc_release_fn(), using non-zero subclass from ioc_release_fn() is enough. Use subclass 1.
---
Jens, this one should go with the previous patch to avoid triggering spurious lockdep warning.
Thanks.
block/blk-ioc.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
block/blk-ioc.c +14 -7
@@ -80,8 +80,15 @@ static void ioc_release_fn(struct work_s struct io_context *ioc = container_of(work, struct io_context, release_work);struct request_queue *last_q =NULL ; +unsigned long flags; -spin_lock_irq (&ioc->lock); +/* +* Exiting icq may call into put_io_context() through elevator +* which will trigger lockdep warning. The ioc's are guaranteed to +* be different, use a different locking subclass here. Use +* irqsave variant as there's no spin_lock_irq_nested(). +*/ +spin_lock_irqsave_nested (&ioc->lock, flags,1 );while (!hlist_empty (&ioc->icq_list)) { struct io_cq *icq = hlist_entry(ioc->icq_list.first,
@@ -103,15 +110,15 @@ static void ioc_release_fn(struct work_s*/ if (last_q) {spin_unlock (last_q->queue_lock); -spin_unlock_irq (&ioc->lock); +spin_unlock_irqrestore (&ioc->lock, flags);blk_put_queue (last_q); }else { -spin_unlock_irq (&ioc->lock); +spin_unlock_irqrestore (&ioc->lock, flags); } last_q = this_q; -spin_lock_irq (this_q->queue_lock); -spin_lock (&ioc->lock); +spin_lock_irqsave (this_q->queue_lock, flags); +spin_lock_nested (&ioc->lock,1 );continue ; }ioc_exit_icq (icq);
@@ -119,10 +126,10 @@ static void ioc_release_fn(struct work_sif (last_q) {spin_unlock (last_q->queue_lock); -spin_unlock_irq (&ioc->lock); +spin_unlock_irqrestore (&ioc->lock, flags);blk_put_queue (last_q); }else { -spin_unlock_irq (&ioc->lock); +spin_unlock_irqrestore (&ioc->lock, flags); }kmem_cache_free (iocontext_cachep, ioc);
Tejun Heo
maintainerQuoted 4 lines · Tejun Heo
It's one wq scheduling on exit for any task which has issued an IO. I don't think it would matter except for task fork/exit microbenchs (or workloads which approximate to that). I'll get some measurements and strip the optimization if it doesn't really show up.
I'm still playing with test methods and getting numbers but the following is the simplified one of the three setups I'm playing with - the current one, simplified and no optimization. There *seems* to be appreciable performance degradation on fork/exit w/ ioc microbenchs so I'm likely to go with the following. I'll post when I know more.
Thanks.
--- block/blk-ioc.c | 50 +++++++++++++------------------------------------- 1 file changed, 13 insertions(+), 37 deletions(-)
block/blk-ioc.c +13 -37
@@ -158,7 +158,6 @@ static void ioc_release_fn(struct work_s*/ void put_io_context (struct io_context *ioc,struct request_queue *locked_q) { -struct request_queue *last_q = locked_q;unsigned long flags;if (ioc ==NULL )
@@ -171,50 +170,27 @@ void put_io_context(struct io_context *iif (!atomic_long_dec_and_test (&ioc->refcount))return ; -/* -* Destroy @ioc. This is a bit messy because icq's are chained -* from both ioc and queue, and ioc->lock nests inside queue_lock. -* The inner ioc->lock should be held to walk our icq_list and then -* for each icq the outer matching queue_lock should be grabbed. -* ie. We need to do reverse-order double lock dancing. -* -* Another twist is that we are often called with one of the -* matching queue_locks held as indicated by @locked_q, which -* prevents performing double-lock dance for other queues. -* -* So, we do it in two stages. The fast path uses the queue_lock -* the caller is holding and, if other queues need to be accessed, -* uses trylock to avoid introducing locking dependency. This can -* handle most cases, especially if @ioc was performing IO on only -* single device. -* -* If trylock doesn't cut it, we defer to @ioc->release_work which -* can do all the double-locking dancing. -*/ spin_lock_irqsave_nested (&ioc->lock, flags,ioc_release_depth (locked_q)); -while (!hlist_empty (&ioc->icq_list)) { +/* +* Due to locking order between queue_lock and ioc lock, proper icq +* shoot down requires reverse double locking dance from another +* context. As there usually is no or one icq, try to handle those +* cases synchronously. +*/ +if (!hlist_empty (&ioc->icq_list)) {struct io_cq *icq =hlist_entry (ioc->icq_list.first, struct io_cq, ioc_node); -struct request_queue *this_q = icq->q; - -if (this_q != last_q) { -if (last_q && last_q != locked_q) -spin_unlock (last_q->queue_lock); - last_q =NULL ; - -if (!spin_trylock (this_q->queue_lock)) -break ; - last_q = this_q; -continue ; +if (locked_q) { +if (locked_q == icq->q) +ioc_exit_icq (icq); + }else if (spin_trylock (icq->q->queue_lock)) { +ioc_exit_icq (icq); +spin_unlock (icq->q->queue_lock); } -ioc_exit_icq (icq); } -if (last_q && last_q != locked_q) -spin_unlock (last_q->queue_lock); -spin_unlock_irqrestore (&ioc->lock, flags);/* if no icq is left, we're done; otherwise, kick release_work */
2012/2/7 Tejun Heo <[email protected]>:
Quoted 11 lines · Tejun Heo
Quoted 4 lines
It's one wq scheduling on exit for any task which has issued an IO. I don't think it would matter except for task fork/exit microbenchs (or workloads which approximate to that). I'll get some measurements and strip the optimization if it doesn't really show up.
I'm still playing with test methods and getting numbers but the following is the simplified one of the three setups I'm playing with - the current one, simplified and no optimization. There *seems* to be appreciable performance degradation on fork/exit w/ ioc microbenchs so I'm likely to go with the following. I'll post when I know more.
Hi,
Since you are talking about performance, one of our microbenchmark (swap) shows a regression. Alex bisect it to be b2efa05265d62bc2, which is related to the ioc change. A little strange to me, don't expect such change can cause performance issue. I haven't double check the issue, but if you have ideas, please
let me know.
Thanks,
Shaohua
Tejun Heo
maintainerHello,
Quoted 5 lines · Shaohua Li
Since you are talking about performance, one of our microbenchmark (swap) shows a regression. Alex bisect it to be
b2efa05265d62bc2, which is related to the ioc change. A little strange to me, don't expect such change can cause performance issue. I haven't double check the issue, but if you have ideas, please let me know.
Hmmm... I think the only thing which can be made slower by that patch is ioc release (ie. task exits). What does the microbench do?
Thanks.
Signature · 1 line
tejun
Quoted 11 lines · Tejun Heo
Hello,
Quoted 5 lines · Shaohua Li
Since you are talking about performance, one of our microbenchmark (swap) shows a regression. Alex bisect it to be
b2efa05265d62bc2, which is related to the ioc change. A little strange to me, don't expect such change can cause performance issue. I haven't double check the issue, but if you have ideas, please let me know.Hmmm... I think the only thing which can be made slower by that patch is ioc release (ie. task exits). What does the microbench do?
Quite simple, copy kernel source to tmpfs in tight memory environment. This will trigger swap. The bisection result appears stable.
Thanks,
Shaohua
Tejun Heo
maintainerQuoted 2 lines · Shaohua Li
Quite simple, copy kernel source to tmpfs in tight memory environment. This will trigger swap. The bisection result appears stable.
Looked through the commit again but it really doesn't change fast paths. The only paths which can be slower are ioc and q exits. Can you please do the followings?
* Share the script used for microbench. Maybe it somehow exposes slower exit path? How much regression are we talking about?
* Test with the this_q != locked_q trylock patch.
Thanks.
Signature · 1 line
tejun
Quoted 10 lines · Tejun Heo
Quoted 2 lines · Shaohua Li
Quite simple, copy kernel source to tmpfs in tight memory environment. This will trigger swap. The bisection result appears stable.
Looked through the commit again but it really doesn't change fast paths. The only paths which can be slower are ioc and q exits. Can you please do the followings?
* Share the script used for microbench. Maybe it somehow exposes slower exit path? How much regression are we talking about?
I need strip the script out from our test framework, but it essentially is a 'time copy -a tmpfs/kernelsource tmpfs/copy'. The time increased ~20% with 3.3-rc1.
Quoted 1 line
* Test with the
this_q!=locked_qtrylock patch.
sure.
Thanks,
Shaohua
Quoted 9 lines · Shaohua Li
Quoted 8 lines · Tejun Heo
Quoted 2 lines · Shaohua Li
Quite simple, copy kernel source to tmpfs in tight memory environment. This will trigger swap. The bisection result appears stable.
Looked through the commit again but it really doesn't change fast paths. The only paths which can be slower are ioc and q exits. Can you please do the followings?
Quoted 5 lines
Quoted 2 lines
* Share the script used for microbench. Maybe it somehow exposes slower exit path? How much regression are we talking about?
I need strip the script out from our test framework, but it essentially is a 'time copy -a tmpfs/kernelsource tmpfs/copy'. The time increased ~20% with 3.3-rc1.
oops, I get confused about our swap and swap-cp test. The workload with regression is swap. which does 'time kernelbuild tmpfs/kernelsource'. we run several such kernelbuild in the meantime to trigger swap. Very sorry to give your wrong info before.
Quoted 1 line · Tejun Heo
* Test with the
this_q!=locked_qtrylock patch.
this one slightly reduces the regression. without it, the regression is about 17%, with it, the regression is about 12%. Test result is stable.
Quoted 4 lines · Shaohua Li
Quoted 1 line · Tejun Heo
* Test with the
this_q!=locked_qtrylock patch.this one slightly reduces the regression. without it, the regression is about 17%, with it, the regression is about 12%. Test result is stable.
What happens when you remove the fast-case entirely?
See the other patch by Tejun with the subject "[PATCH] block: strip out locking optimization in put_io_context()"
Linus
Tejun Heo
maintainerHello,
block/blk-ioc.c void put_io_context(struct io_context *ioc, struct request_queue *locked_q) · line 209
Quoted 12 lines · Shaohua Li
block/blk-ioc.c +3
-1@@ -204,7 +204,9 @@ void put_io_context(struct io_context *ioc, struct request_queue *locked_q)spin_unlock (last_q->queue_lock); last_q =NULL ; -if (!spin_trylock (this_q->queue_lock)) +/* spin_trylock() always successes in UP case */ +if (this_q != locked_q && + !spin_trylock (this_q->queue_lock))
Yeah, this is the right thing to do. I think the comment is slightly misleading. I'll prep a patch.
Thanks.
Signature · 1 line
tejun
Quoted 5 lines · Shaohua Li
block/blk-ioc.c +0
-0
block/blk-ioc.c void put_io_context(struct io_context *ioc, struct request_queue *locked_q) · line 212
Quoted 10 lines
block/blk-ioc.c +3
-1@@ -204,7 +204,9 @@ void put_io_context(struct io_context *ioc, struct request_queue *locked_q)spin_unlock (last_q->queue_lock); last_q =NULL ; -if (!spin_trylock (this_q->queue_lock)) +/* spin_trylock() always successes in UP case */ +if (this_q != locked_q&& + !spin_trylock (this_q->queue_lock))break ; last_q = this_q;continue ;
I can see from the rest of this thread that your patch fixes a multitude of different backtraces, including (most importantly :) mine, thanks: