kvm deadlock
Hello,
I am struggling with repeatable full hardware locks when running 8-12 KVM vms. At some point before the hard lock I get a inconsistent lock state warning. An example of this can be found here:
After that the server continues to run for a while and then starts its death spiral. When it reaches that point it fails to log anything further to the disk, but by attaching a console I have been able to get a stack trace documenting the final implosion:
All of the cores end up hung and the server stops responding to all input, including SysRq commands.
I have seen this behavior on two machines (dual E5606 running Fedora 16) both passed cpuburnin testing and memtest86 scans without error.
I have reproduced the crash and stack traces from a Fedora debugging kernel - 3.1.2-1 and with a vanilla 3.1.4 kernel.
Nate Custer
QA Analyst
cPanel Inc
Quoted 8 lines · Nate Custer
Hello,
I am struggling with repeatable full hardware locks when running 8-12 KVM vms. At some point before the hard lock I get a inconsistent lock state warning. An example of this can be found here:
After that the server continues to run for a while and then starts its death spiral. When it reaches that point it fails to log anything further to the disk, but by attaching a console I have been able to get a stack trace documenting the final implosion:
Quoted 7 lines
All of the cores end up hung and the server stops responding to all input, including SysRq commands.
I have seen this behavior on two machines (dual E5606 running Fedora 16) both passed cpuburnin testing and memtest86 scans without error.
I have reproduced the crash and stack traces from a Fedora debugging kernel - 3.1.2-1 and with a vanilla 3.1.4 kernel.
Busted hardware, apparently. Can you reproduce these issues with the same workload on different hardware?
Quoted 11 lines · Marcelo Tosatti
Quoted 10 lines · Nate Custer
Hello,
I am struggling with repeatable full hardware locks when running 8-12 KVM vms. At some point before the hard lock I get a inconsistent lock state warning. An example of this can be found here:
After that the server continues to run for a while and then starts its death spiral. When it reaches that point it fails to log anything further to the disk, but by attaching a console I have been able to get a stack trace documenting the final implosion:
Quoted 8 lines
Quoted 5 lines
All of the cores end up hung and the server stops responding to all input, including SysRq commands.
I have seen this behavior on two machines (dual E5606 running Fedora 16) both passed cpuburnin testing and memtest86 scans without error.
I have reproduced the crash and stack traces from a Fedora debugging kernel - 3.1.2-1 and with a vanilla 3.1.4 kernel.
Busted hardware, apparently. Can you reproduce these issues with the same workload on different hardware?
I don't think it's hardware related. The second trace (in the first paste) is called during swap, so GFP_FS is set. The first one is not, so GFP_FS is clear. Lockdep is worried about the following scenario:
acpi_early_init() is called
calls pcpu_alloc(), which takes pcpu_alloc_mutex eventually, calls kmalloc(), or some other allocation function no memory, so swap
call try_to_free_pages()
submit_bio()
blk_throtl_bio()
blkio_alloc_blkg_stats()
alloc_percpu()
pcpu_alloc(), which takes pcpu_alloc_mutex
deadlock
It's a little unlikely that acpi_early_init() will OOM, but lockdep doesn't know that. Other callers of pcpu_alloc() could trigger the same thing.
When lockdep says
[ 5839.924953] other info that might help us debug this: [ 5839.925396] Possible unsafe locking scenario: [ 5839.925397] [ 5839.925840] CPU0 [ 5839.926063] ---- [ 5839.926287] lock(pcpu_alloc_mutex); [ 5839.926533] <Interrupt> [ 5839.926756] lock(pcpu_alloc_mutex); [ 5839.926986]
It really means
<swap, set GFP_FS>
GFP_FS simply marks the beginning of a nested, unrelated context that uses the same thread, just like an interrupt. Kudos to lockdep for catching that.
I think the allocation in blkio_alloc_blkg_stats() should be moved out of the I/O path into some init function. Copying Jens.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 55 lines · Avi Kivity
Quoted 11 lines · Marcelo Tosatti
Quoted 10 lines · Nate Custer
Hello,
I am struggling with repeatable full hardware locks when running 8-12 KVM vms. At some point before the hard lock I get a inconsistent lock state warning. An example of this can be found here:
After that the server continues to run for a while and then starts its death spiral. When it reaches that point it fails to log anything further to the disk, but by attaching a console I have been able to get a stack trace documenting the final implosion:
Quoted 8 lines
Quoted 5 lines
All of the cores end up hung and the server stops responding to all input, including SysRq commands.
I have seen this behavior on two machines (dual E5606 running Fedora 16) both passed cpuburnin testing and memtest86 scans without error.
I have reproduced the crash and stack traces from a Fedora debugging kernel - 3.1.2-1 and with a vanilla 3.1.4 kernel.
Busted hardware, apparently. Can you reproduce these issues with the same workload on different hardware?
I don't think it's hardware related. The second trace (in the first paste) is called during swap, so
GFP_FSis set. The first one is not, soGFP_FSis clear. Lockdep is worried about the following scenario:
acpi_early_init()is called
callspcpu_alloc(), which takespcpu_alloc_mutexeventually, callskmalloc(), or some other allocation function no memory, so swap
calltry_to_free_pages()
submit_bio()
blk_throtl_bio()
blkio_alloc_blkg_stats()
alloc_percpu()
pcpu_alloc(), which takespcpu_alloc_mutex
deadlockIt's a little unlikely that
acpi_early_init()will OOM, but lockdep doesn't know that. Other callers ofpcpu_alloc()could trigger the same thing.When lockdep says
[ 5839.924953] other info that might help us debug this: [ 5839.925396] Possible unsafe locking scenario: [ 5839.925397] [ 5839.925840] CPU0 [ 5839.926063] ---- [ 5839.926287] lock(pcpu_alloc_mutex); [ 5839.926533] <Interrupt> [ 5839.926756] lock(pcpu_alloc_mutex); [ 5839.926986]It really means
Quoted 8 lines
<swap, set
GFP_FS>
GFP_FSsimply marks the beginning of a nested, unrelated context that uses the same thread, just like an interrupt. Kudos to lockdep for catching that.I think the allocation in
blkio_alloc_blkg_stats()should be moved out of the I/O path into some init function. Copying Jens.
The other traces have apparently bogus NMI interrupts, but it might be a software bug, OK.
Quoted 2 lines · Marcelo Tosatti
The other traces have apparently bogus NMI interrupts, but it might be a software bug, OK.
These are from sysrq-blah, no? I'm looking at them now.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 5 lines · Avi Kivity
Quoted 2 lines · Marcelo Tosatti
The other traces have apparently bogus NMI interrupts, but it might be a software bug, OK.
These are from sysrq-blah, no? I'm looking at them now.
I don't know. Its a hang ? It could be memory corruption (of the timer olist) instead of a bogus NMI actually, the second.
Nate?
Quoted 2 lines · Marcelo Tosatti
I don't know. Its a hang ? It could be memory corruption (of the timer olist) instead of a bogus NMI actually, the second.
What is pasted in the second paste is what came scrolling across the console right before the end of all responsiveness. It came from a dmesg dump, the next dmesg command was not accepted via ssh and the console attached showed the same stack trace. At that point the system refused to respond to any direct keyboard input, including the SysRq commands that I expected to work after a core dump.
The issue happened with two servers (same hardware, same build group so there is a chance of a bad hardware batch). Switching to an older kernel/kvm setup in RHEL 6.2 has corrected the issue, which suggests a software issue to me.
Nate Custer
Quoted 9 lines · Nate Custer
Quoted 2 lines · Marcelo Tosatti
I don't know. Its a hang ? It could be memory corruption (of the timer olist) instead of a bogus NMI actually, the second.
What is pasted in the second paste is what came scrolling across the console right before the end of all responsiveness. It came from a dmesg dump, the next dmesg command was not accepted via ssh and the console attached showed the same stack trace. At that point the system refused to respond to any direct keyboard input, including the SysRq commands that I expected to work after a core dump.
The issue happened with two servers (same hardware, same build group so there is a chance of a bad hardware batch). Switching to an older kernel/kvm setup in RHEL 6.2 has corrected the issue, which suggests a software issue to me.
Right. Perhaps try an older upstream kernel to find a culprit then.
Quoted 9 lines · Nate Custer
Quoted 2 lines · Marcelo Tosatti
I don't know. Its a hang ? It could be memory corruption (of the timer olist) instead of a bogus NMI actually, the second.
What is pasted in the second paste is what came scrolling across the console right before the end of all responsiveness. It came from a dmesg dump, the next dmesg command was not accepted via ssh and the console attached showed the same stack trace. At that point the system refused to respond to any direct keyboard input, including the SysRq commands that I expected to work after a core dump.
The issue happened with two servers (same hardware, same build group so there is a chance of a bad hardware batch). Switching to an older kernel/kvm setup in RHEL 6.2 has corrected the issue, which suggests a software issue to me.
If you can, please set up netconsole, which should give us complete dmesg captures.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 9 lines · Marcelo Tosatti
Quoted 5 lines · Avi Kivity
Quoted 2 lines · Marcelo Tosatti
The other traces have apparently bogus NMI interrupts, but it might be a software bug, OK.
These are from sysrq-blah, no? I'm looking at them now.
I don't know. Its a hang ? It could be memory corruption (of the timer olist) instead of a bogus NMI actually, the second.
Looks like lots of cpus are waiting on the smp_call_function_single() lock. Looks like rcu is complaining:
[ 4959.814010] [<ffffffff81252e47>] __const_udelay+0x2c/0x2e [ 4959.814017] [<ffffffff81027449>] native_safe_apic_wait_icr_idle+0x31/0x3d [ 4959.814024] [<ffffffff81027f11>] __default_send_IPI_dest_field.constprop.0+0x23/0x5d [ 4959.814032] [<ffffffff81027f93>] default_send_IPI_mask_sequence_phys+0x48/0x97 [ 4959.814039] [<ffffffff81089848>] ? tick_nohz_handler+0xdf/0xdf [ 4959.814044] [<ffffffff8102b561>] physflat_send_IPI_all+0x17/0x19 [ 4959.814052] [<ffffffff81028102>] arch_trigger_all_cpu_backtrace+0x57/0x89 [ 4959.814057] [<ffffffff810c7711>] __rcu_pending+0x89/0x328 [ 4959.814063] [<ffffffff81089848>] ? tick_nohz_handler+0xdf/0xdf [ 4959.814067] [<ffffffff810c7e0a>] rcu_check_callbacks+0x88/0xb9 [ 4959.814071] [<ffffffff8106aeba>] update_process_times+0x3f/0x75
Maybe the core issue is that CPU 3 is spinning in do_insn_fetch() and denying rcu grace periods. Nate, can you provide a few more dumps (this is looking at the second paste, so more of the same)?
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 55 lines · Avi Kivity
Quoted 11 lines · Marcelo Tosatti
Quoted 10 lines · Nate Custer
Hello,
I am struggling with repeatable full hardware locks when running 8-12 KVM vms. At some point before the hard lock I get a inconsistent lock state warning. An example of this can be found here:
After that the server continues to run for a while and then starts its death spiral. When it reaches that point it fails to log anything further to the disk, but by attaching a console I have been able to get a stack trace documenting the final implosion:
Quoted 8 lines
Quoted 5 lines
All of the cores end up hung and the server stops responding to all input, including SysRq commands.
I have seen this behavior on two machines (dual E5606 running Fedora 16) both passed cpuburnin testing and memtest86 scans without error.
I have reproduced the crash and stack traces from a Fedora debugging kernel - 3.1.2-1 and with a vanilla 3.1.4 kernel.
Busted hardware, apparently. Can you reproduce these issues with the same workload on different hardware?
I don't think it's hardware related. The second trace (in the first paste) is called during swap, so
GFP_FSis set. The first one is not, soGFP_FSis clear. Lockdep is worried about the following scenario:
acpi_early_init()is called
callspcpu_alloc(), which takespcpu_alloc_mutexeventually, callskmalloc(), or some other allocation function no memory, so swap
calltry_to_free_pages()
submit_bio()
blk_throtl_bio()
blkio_alloc_blkg_stats()
alloc_percpu()
pcpu_alloc(), which takespcpu_alloc_mutex
deadlockIt's a little unlikely that
acpi_early_init()will OOM, but lockdep doesn't know that. Other callers ofpcpu_alloc()could trigger the same thing.When lockdep says
[ 5839.924953] other info that might help us debug this: [ 5839.925396] Possible unsafe locking scenario: [ 5839.925397] [ 5839.925840] CPU0 [ 5839.926063] ---- [ 5839.926287] lock(pcpu_alloc_mutex); [ 5839.926533] <Interrupt> [ 5839.926756] lock(pcpu_alloc_mutex); [ 5839.926986]It really means
Quoted 8 lines
<swap, set
GFP_FS>
GFP_FSsimply marks the beginning of a nested, unrelated context that uses the same thread, just like an interrupt. Kudos to lockdep for catching that.I think the allocation in
blkio_alloc_blkg_stats()should be moved out of the I/O path into some init function. Copying Jens.
That's completely buggy, basically you end up with a GFP_KERNEL allocation from the IO submit path. Vivek, per_cpu data needs to be set up at init time. You can't allocate it dynamically off the IO path.
Signature · 1 line
Jens Axboe
Quoted 62 lines · Jens Axboe
Quoted 61 lines · Avi Kivity
Quoted 11 lines · Marcelo Tosatti
Quoted 10 lines · Nate Custer
Hello,
I am struggling with repeatable full hardware locks when running 8-12 KVM vms. At some point before the hard lock I get a inconsistent lock state warning. An example of this can be found here:
After that the server continues to run for a while and then starts its death spiral. When it reaches that point it fails to log anything further to the disk, but by attaching a console I have been able to get a stack trace documenting the final implosion:
Quoted 8 lines
Quoted 5 lines
All of the cores end up hung and the server stops responding to all input, including SysRq commands.
I have seen this behavior on two machines (dual E5606 running Fedora 16) both passed cpuburnin testing and memtest86 scans without error.
I have reproduced the crash and stack traces from a Fedora debugging kernel - 3.1.2-1 and with a vanilla 3.1.4 kernel.
Busted hardware, apparently. Can you reproduce these issues with the same workload on different hardware?
I don't think it's hardware related. The second trace (in the first paste) is called during swap, so
GFP_FSis set. The first one is not, soGFP_FSis clear. Lockdep is worried about the following scenario:
acpi_early_init()is called
callspcpu_alloc(), which takespcpu_alloc_mutexeventually, callskmalloc(), or some other allocation function no memory, so swap
calltry_to_free_pages()
submit_bio()
blk_throtl_bio()
blkio_alloc_blkg_stats()
alloc_percpu()
pcpu_alloc(), which takespcpu_alloc_mutex
deadlockIt's a little unlikely that
acpi_early_init()will OOM, but lockdep doesn't know that. Other callers ofpcpu_alloc()could trigger the same thing.When lockdep says
[ 5839.924953] other info that might help us debug this: [ 5839.925396] Possible unsafe locking scenario: [ 5839.925397] [ 5839.925840] CPU0 [ 5839.926063] ---- [ 5839.926287] lock(pcpu_alloc_mutex); [ 5839.926533] <Interrupt> [ 5839.926756] lock(pcpu_alloc_mutex); [ 5839.926986]It really means
<swap, set
GFP_FS>
GFP_FSsimply marks the beginning of a nested, unrelated context that uses the same thread, just like an interrupt. Kudos to lockdep for catching that.
Quoted 6 lines
Quoted 2 lines
I think the allocation in
blkio_alloc_blkg_stats()should be moved out of the I/O path into some init function. Copying Jens.That's completely buggy, basically you end up with a
GFP_KERNELallocation from the IO submit path. Vivek,per_cpudata needs to be set up at init time. You can't allocate it dynamically off the IO path.
Hi Jens,
I am wondering how does CFQ get away with blocking cfqq allocation in IO submit path. I see that blk_queue_bio() will do following.
blk_queue_bio()
get_request_wait()
get_request(..,..,GFP_NOIO)
blk_alloc_request()
elv_set_request()
cfq_set_request()
---> Can sleep and do memory allocation in IO submit path as
GFP_NOIO has __GFP_WAIT.
So that means sleeping allocation from IO submit path is not necessarily a problem?
But in case of per cpu data allocation, we might be holding pcpu_alloc_mutex() already at the time of calling pcpu allocation again and that might lead to deadlock. (As Avi mentioned). If yes, then it is a problem.
Right now allocation of root group and associated stats happens at queue initialization time. For non-root cgroups, group allocation and associated per cpu stats allocation happens dynamically when the IO is submitted. So in this case may be we are creating a new blkio cgroup and then doing IO which leads to this warning.
I am not sure how to move this allocation to init path. These stats are per group and groups are created dynamically as IO happens in them. Only init path seems to be cgroup creation time. blkg is an object which is contained in a parent object and at that time parent object is not available. It is created dynamically at the IO time (cfq_group, blkio_group etc).
Though it is little hackish but can we just delay the allocation of stats if pcpu_alloc_mutex is held. We shall have to make pcpu_alloc_mutex non static though. Delaying will just not capture the stats for some time and sooner or later we will get regular IO with pcpu_alloc_mutex not held and we can do per cpu allocation at that time. I will write a a test patch.
Or may be there is a safer version of pcpu alloc which will return without allocation if pcpu_alloc_mutex is already locked.
CCing Tejun too.
Thanks
Vivek
Quoted 21 lines · Vivek Goyal
Quoted 6 lines
Quoted 2 lines
I think the allocation in
blkio_alloc_blkg_stats()should be moved out of the I/O path into some init function. Copying Jens.That's completely buggy, basically you end up with a
GFP_KERNELallocation from the IO submit path. Vivek,per_cpudata needs to be set up at init time. You can't allocate it dynamically off the IO path.Hi Jens,
I am wondering how does CFQ get away with blocking cfqq allocation in IO submit path. I see that
blk_queue_bio()will do following.
blk_queue_bio()
get_request_wait()
get_request(..,..,GFP_NOIO)
blk_alloc_request()
elv_set_request()
cfq_set_request()
---> Can sleep and do memory allocation in IO submit path as
GFP_NOIOhas__GFP_WAIT.
Quoted 2 lines
So that means sleeping allocation from IO submit path is not necessarily a problem?
__GFP_WAIT isn't the problem, you can block in the IO path. You cannot, however, recurse back into IO submission. That's why CFQ is using GFP_NOIO, implying that waiting is OK, but submitting new IO to satisfy the allocation is not.
Quoted 4 lines
But in case of per cpu data allocation, we might be holding
pcpu_alloc_mutex()already at the time of calling pcpu allocation again and that might lead to deadlock. (As Avi mentioned). If yes, then it is a problem.
It is both a problem and somewhat of a mess...
Quoted 20 lines
Right now allocation of root group and associated stats happens at queue initialization time. For non-root cgroups, group allocation and associated per cpu stats allocation happens dynamically when the IO is submitted. So in this case may be we are creating a new blkio cgroup and then doing IO which leads to this warning.
I am not sure how to move this allocation to init path. These stats are per group and groups are created dynamically as IO happens in them. Only init path seems to be cgroup creation time. blkg is an object which is contained in a parent object and at that time parent object is not available. It is created dynamically at the IO time (
cfq_group,blkio_groupetc).Though it is little hackish but can we just delay the allocation of stats if
pcpu_alloc_mutexis held. We shall have to makepcpu_alloc_mutexnon static though. Delaying will just not capture the stats for some time and sooner or later we will get regular IO withpcpu_alloc_mutexnot held and we can do per cpu allocation at that time. I will write a a test patch.
Quoted 2 lines
Or may be there is a safer version of pcpu alloc which will return without allocation if
pcpu_alloc_mutexis already locked.
Both of these proposed solutions aren't really pretty. It would be cleaner and have better runtime for the fast path if you could alloc all of these when the non-root groups are setup. Why isn't it done that way right now?
Signature · 1 line
Jens Axboe
Quoted 25 lines · Jens Axboe
Quoted 23 lines · Vivek Goyal
Quoted 6 lines
Quoted 2 lines
I think the allocation in
blkio_alloc_blkg_stats()should be moved out of the I/O path into some init function. Copying Jens.That's completely buggy, basically you end up with a
GFP_KERNELallocation from the IO submit path. Vivek,per_cpudata needs to be set up at init time. You can't allocate it dynamically off the IO path.Hi Jens,
I am wondering how does CFQ get away with blocking cfqq allocation in IO submit path. I see that
blk_queue_bio()will do following.
blk_queue_bio()
get_request_wait()
get_request(..,..,GFP_NOIO)
blk_alloc_request()
elv_set_request()
cfq_set_request()
---> Can sleep and do memory allocation in IO submit path as
GFP_NOIOhas__GFP_WAIT.So that means sleeping allocation from IO submit path is not necessarily a problem?
[..]
Quoted 4 lines
__GFP_WAITisn't the problem, you can block in the IO path. You cannot, however, recurse back into IO submission. That's why CFQ is usingGFP_NOIO, implying that waiting is OK, but submitting new IO to satisfy the allocation is not.
Ok. Got it. So even if we implement mutex_is_locked() check, it does not protect me from possiblity of per cpu allocation path recursing into IO submission. That means, there needs to be a variant of per cpu allocation which can take the allocation flags as parameter and honor these flags.
Quoted 28 lines
Quoted 5 lines
But in case of per cpu data allocation, we might be holding
pcpu_alloc_mutex()already at the time of calling pcpu allocation again and that might lead to deadlock. (As Avi mentioned). If yes, then it is a problem.It is both a problem and somewhat of a mess...
Quoted 20 lines
Right now allocation of root group and associated stats happens at queue initialization time. For non-root cgroups, group allocation and associated per cpu stats allocation happens dynamically when the IO is submitted. So in this case may be we are creating a new blkio cgroup and then doing IO which leads to this warning.
I am not sure how to move this allocation to init path. These stats are per group and groups are created dynamically as IO happens in them. Only init path seems to be cgroup creation time. blkg is an object which is contained in a parent object and at that time parent object is not available. It is created dynamically at the IO time (
cfq_group,blkio_groupetc).Though it is little hackish but can we just delay the allocation of stats if
pcpu_alloc_mutexis held. We shall have to makepcpu_alloc_mutexnon static though. Delaying will just not capture the stats for some time and sooner or later we will get regular IO withpcpu_alloc_mutexnot held and we can do per cpu allocation at that time. I will write a a test patch.
Quoted 7 lines
Quoted 2 lines
Or may be there is a safer version of pcpu alloc which will return without allocation if
pcpu_alloc_mutexis already locked.Both of these proposed solutions aren't really pretty. It would be cleaner and have better runtime for the fast path if you could alloc all of these when the non-root groups are setup. Why isn't it done that way right now?
Actually non-root group itself is setup when first IO happens in the group. When an IO is submitted, we determine which cgroup does it belong to, then we see if we have already setup the group. If not, then we go on to alloc one. (Currently I use GFP_ATOMIC for group allocation).
So group allocation itself happens in IO path (like cfqq allocation), hence per group per cpu stats are also allocated in that path.
We can't setup blkio groups at the cgroup creation time as at that time we don't even know how many request queues are threre and on how many of them cfq is being used.
Setup of root group at queue initialization time is easy. But we can't setup all other cgroups at that time as it does not take care of future cgroup creation. Also it will waste memory if cgroups are there but no IO is happening in them.
Thanks
Vivek
Hello,
Quoted 11 lines · Vivek Goyal
[..]
Quoted 4 lines
__GFP_WAITisn't the problem, you can block in the IO path. You cannot, however, recurse back into IO submission. That's why CFQ is usingGFP_NOIO, implying that waiting is OK, but submitting new IO to satisfy the allocation is not.Ok. Got it. So even if we implement
mutex_is_locked()check, it does not protect me from possiblity of per cpu allocation path recursing into IO submission. That means, there needs to be a variant of per cpu allocation which can take the allocation flags as parameter and honor these flags.
Slightly tangential but we actually have a bug here. Under high enough memory pressure, ioc or cic allocation can fail which will make request alloc fail and retry, which isn't guaranteed to make forward progress. struct request itself is mempool backed but ioc/cic aren't. It seems hitting this problem (and thus IO / memalloc deadlock) isn't too difficult w/ memcg.
An easy fix would be using INSERT_BACK instead of INSERT_SORT if elevator_set() fails. I'll soon post patches to fix the problem.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Or may be there is a safer version of pcpu alloc which will return without allocation if
pcpu_alloc_mutexis already locked.
pcpu alloc depends on vmalloc allocation, so it isn't trivial. We can try to make percpu keep cache of areas for this type of allocation but I personally think doing percpu allocation from atomic context or IO path is a bad idea. Hmmm...
Thanks.
Signature · 1 line
tejun
On Wed, Dec 14, 2011 at 10:16:23AM -0800, Tejun Heo wrote:
[..]
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Or may be there is a safer version of pcpu alloc which will return without allocation if
pcpu_alloc_mutexis already locked.pcpu alloc depends on vmalloc allocation, so it isn't trivial. We can try to make percpu keep cache of areas for this type of allocation but I personally think doing percpu allocation from atomic context or IO path is a bad idea. Hmmm...
Looks like I am running out of options here. I can't find a suitable path where I can allocate these stats out of IO path. Because devices can be plugged in dynamically (and these stats are per cgroup, per device), and cgroups can be created dynamically after device creation, I can't do any static allocation out of IO path. So that kind of makes use of per cpu memory areas for stats in this case impossible.
For a moment I thought of doing allocation from worker thread after taking a reference on the original group. Allow the IO submission to continue without blocking. Just that till per cpu areas are allocated, we will not collect any stats.
But for locking we rely on request queue lock and request queue might be gone by the time per cpu areas are allocated. That means we need a group refenrence on the request queue. Request queue referencing and life time is already full of bugs. So I don't feel comfortable adding more code there (till atleast your cleanup patches go in).
Hmm..., is revert of per cpu blkio group stats the only sane choice left now.
Thanks
Vivek
Quoted 18 lines · Vivek Goyal
On Wed, Dec 14, 2011 at 10:16:23AM -0800, Tejun Heo wrote:
[..]
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Or may be there is a safer version of pcpu alloc which will return without allocation if
pcpu_alloc_mutexis already locked.pcpu alloc depends on vmalloc allocation, so it isn't trivial. We can try to make percpu keep cache of areas for this type of allocation but I personally think doing percpu allocation from atomic context or IO path is a bad idea. Hmmm...
Looks like I am running out of options here. I can't find a suitable path where I can allocate these stats out of IO path. Because devices can be plugged in dynamically (and these stats are per cgroup, per device), and cgroups can be created dynamically after device creation, I can't do any static allocation out of IO path. So that kind of makes use of per cpu memory areas for stats in this case impossible.
Quoted 5 lines
For a moment I thought of doing allocation from worker thread after taking a reference on the original group. Allow the IO submission to continue without blocking. Just that till per cpu areas are allocated, we will not collect any stats.
Ok, even though I am not convinced about it yet, I wrote a small patch to allocate per cpu data area from worker thread context asynchronously (except for root cgroup). We continue to submit the IO without blocking for per cpu data and if per cpu data is not allocated yet, we lose the stat for that duration for that group.
Here is half baked test patch which compiles and boots. CFQ changes are not done yet. I have yet to find out if I need any additional locking or barriers. Right now I am just relying on reference counting of group objects.
I am posting it here to figure if this kind of approach is acceptable or completely frowned upon.
Thanks
Vivek
Not-signed-half-baked-test-patch
--- block/blk-cgroup.c | 29 +++++++++++++++++++++++++++ block/blk-cgroup.h | 2 + block/blk-throttle.c | 53 ++++++++++++++++++++++++++++++++++++++++++--------- 3 files changed, 75 insertions(+), 9 deletions(-)
blk-throttle.c renamed from blk-throttle.c.orig +44 -9
@@ -81,6 +81,7 @@ struct throtl_grp {int limits_changed;struct rcu_head rcu_head; +struct work_struct stat_alloc_work; };struct throtl_data
@@ -159,6 +160,7 @@ static void throtl_free_tg(struct rcu_hestruct throtl_grp *tg; tg = container_of(head, struct throtl_grp, rcu_head); +/* Will delayed allocation be visible here for sure? */ free_percpu (tg->blkg.stats_cpu);kfree (tg); }
@@ -181,6 +183,24 @@ static void throtl_put_tg(struct throtl_call_rcu (&tg->rcu_head, throtl_free_tg); } +/* No locks taken. A reference to throtl_grp taken before invocation */ +static void tg_stat_alloc_fn (struct work_struct *work) +{ + struct throtl_grp *tg = container_of(work, struct throtl_grp, + stat_alloc_work); +void *stat_ptr =NULL ; + + stat_ptr =blkio_alloc_blkg_stats_pcpu (); + +if (stat_ptr ==NULL ) { +throtl_put_tg (tg); +return ; + } + +blkio_set_blkg_stats_pcpu (&tg->blkg, stat_ptr); +throtl_put_tg (tg); +} +static void throtl_init_group (struct throtl_grp *tg) {INIT_HLIST_NODE (&tg->tg_node);
@@ -188,6 +208,7 @@ static void throtl_init_group(struct thrbio_list_init (&tg->bio_lists[0 ]);bio_list_init (&tg->bio_lists[1 ]); tg->limits_changed = false; +INIT_WORK (&tg->stat_alloc_work, tg_stat_alloc_fn);/* Practically unlimited BW */ tg->bps[0 ] = tg->bps[1 ] =-1 ;
@@ -264,7 +285,7 @@ static void throtl_init_add_tg_lists(str }/* Should be called without queue lock and outside of rcu period */ -static struct throtl_grp *throtl_alloc_tg (struct throtl_data *td) +static struct throtl_grp *throtl_alloc_tg (struct throtl_data *td,bool async) {struct throtl_grp *tg =NULL ;int ret;
@@ -273,14 +294,28 @@ static struct throtl_grp *throtl_alloc_tif (!tg)return NULL ; - ret =blkio_alloc_blkg_stats (&tg->blkg); +if (!async) { + ret =blkio_alloc_blkg_stats (&tg->blkg); -if (ret) { -kfree (tg); -return NULL ; +if (ret) { +kfree (tg); +return NULL ; + } }throtl_init_group (tg); + +if (async) { +/* +* Schedule the group per cpu stat allocation through worker +* thread +*/ +throtl_ref_get_tg (tg); +if (!schedule_work (&tg->stat_alloc_work)) +/* nobody should have scheduled this work before */ +WARN_ON (1 ); + } +return tg; }
@@ -329,14 +364,14 @@ static struct throtl_grp * throtl_get_tgrcu_read_unlock ();spin_unlock_irq (q->queue_lock); - tg =throtl_alloc_tg (td); + tg =throtl_alloc_tg (td,1 );/* Group allocated and queue is still alive. take the lock */ spin_lock_irq (q->queue_lock);/* Make sure @q is still alive */ if (unlikely (test_bit (QUEUE_FLAG_DEAD , &q->queue_flags))) { -kfree (tg); +throtl_put_tg (tg);return NULL ; }
@@ -353,7 +388,7 @@ static struct throtl_grp * throtl_get_tg __tg =throtl_find_tg (td, blkcg);if (__tg) { -kfree (tg); +throtl_put_tg (tg);rcu_read_unlock ();return __tg; }
@@ -1254,7 +1289,7 @@ int blk_throtl_init(struct request_queue/* alloc and Init root group. */ td->queue = q; - tg =throtl_alloc_tg (td); + tg =throtl_alloc_tg (td,0 );if (!tg) {kfree (td);
blk-cgroup.c renamed from blk-cgroup.c.orig +29 -0
@@ -400,6 +400,9 @@ void blkiocg_update_dispatch_stats(strucstruct blkio_group_stats_cpu *stats_cpu;unsigned long flags; +if (blkg->stats_cpu ==NULL ) +return ; +/* * Disabling interrupts to provide mutual exclusion between two * writes on same cpu. It probably is not needed for 64bit. Not
@@ -446,6 +449,9 @@ void blkiocg_update_io_merged_stats(strustruct blkio_group_stats_cpu *stats_cpu;unsigned long flags; +if (blkg->stats_cpu ==NULL ) +return ; +/* * Disabling interrupts to provide mutual exclusion between two * writes on same cpu. It probably is not needed for 64bit. Not
@@ -477,6 +483,19 @@ int blkio_alloc_blkg_stats(struct blkio_ }EXPORT_SYMBOL_GPL (blkio_alloc_blkg_stats); +void *blkio_alloc_blkg_stats_pcpu (void ) +{ +/* Allocate memory for per cpu stats */ + return alloc_percpu(struct blkio_group_stats_cpu); +} +EXPORT_SYMBOL_GPL (blkio_alloc_blkg_stats_pcpu); + +void blkio_set_blkg_stats_pcpu (struct blkio_group *blkg,void *stats_cpu) +{ + blkg->stats_cpu = stats_cpu; +} +EXPORT_SYMBOL_GPL (blkio_set_blkg_stats_pcpu); +void blkiocg_add_blkio_group (struct blkio_cgroup *blkcg,struct blkio_group *blkg,void *key,dev_t dev,enum blkio_policy_id plid)
@@ -551,6 +570,12 @@ static void blkio_reset_stats_cpu(struct {struct blkio_group_stats_cpu *stats_cpu;int i, j, k; + +/* blkg->stats_cpu can have delayed allocation */ + +if (!blkg->stats_cpu) +return ; +/* * Note: On 64 bit arch this should not be an issue. This has the * possibility of returning some inconsistent value on 32bit arch
@@ -673,6 +698,10 @@ static uint64_t blkio_read_stat_cpu(strustruct blkio_group_stats_cpu *stats_cpu;u64 val =0 , tval; +/* blkg->stats_cpu might not have been allocated yet */ +if (blkg->stats_cpu ==NULL ) +return 0 ; +for_each_possible_cpu (cpu) {unsigned int start; stats_cpu =per_cpu_ptr (blkg->stats_cpu, cpu);
blk-cgroup.h renamed from blk-cgroup.h.orig +2 -0
@@ -311,6 +311,8 @@ extern void blkiocg_add_blkio_group(stru struct blkio_group *blkg, void *key, dev_t dev, enum blkio_policy_id plid);extern int blkio_alloc_blkg_stats (struct blkio_group *blkg); +extern void *blkio_alloc_blkg_stats_pcpu (void ); +extern void blkio_set_blkg_stats_pcpu (struct blkio_group *blkg,void *stats_cpu);extern int blkiocg_del_blkio_group (struct blkio_group *blkg);extern struct blkio_group *blkiocg_lookup_group (struct blkio_cgroup *blkcg,void *key);
PATCH 1/1 blkio: alloc per cpu data from worker thread context( Re: kvm deadlock)
In reply to Jens AxboeQuoted 62 lines · Jens Axboe
Quoted 61 lines · Avi Kivity
Quoted 11 lines · Marcelo Tosatti
Quoted 10 lines · Nate Custer
Hello,
I am struggling with repeatable full hardware locks when running 8-12 KVM vms. At some point before the hard lock I get a inconsistent lock state warning. An example of this can be found here:
After that the server continues to run for a while and then starts its death spiral. When it reaches that point it fails to log anything further to the disk, but by attaching a console I have been able to get a stack trace documenting the final implosion:
Quoted 8 lines
Quoted 5 lines
All of the cores end up hung and the server stops responding to all input, including SysRq commands.
I have seen this behavior on two machines (dual E5606 running Fedora 16) both passed cpuburnin testing and memtest86 scans without error.
I have reproduced the crash and stack traces from a Fedora debugging kernel - 3.1.2-1 and with a vanilla 3.1.4 kernel.
Busted hardware, apparently. Can you reproduce these issues with the same workload on different hardware?
I don't think it's hardware related. The second trace (in the first paste) is called during swap, so
GFP_FSis set. The first one is not, soGFP_FSis clear. Lockdep is worried about the following scenario:
acpi_early_init()is called
callspcpu_alloc(), which takespcpu_alloc_mutexeventually, callskmalloc(), or some other allocation function no memory, so swap
calltry_to_free_pages()
submit_bio()
blk_throtl_bio()
blkio_alloc_blkg_stats()
alloc_percpu()
pcpu_alloc(), which takespcpu_alloc_mutex
deadlockIt's a little unlikely that
acpi_early_init()will OOM, but lockdep doesn't know that. Other callers ofpcpu_alloc()could trigger the same thing.When lockdep says
[ 5839.924953] other info that might help us debug this: [ 5839.925396] Possible unsafe locking scenario: [ 5839.925397] [ 5839.925840] CPU0 [ 5839.926063] ---- [ 5839.926287] lock(pcpu_alloc_mutex); [ 5839.926533] <Interrupt> [ 5839.926756] lock(pcpu_alloc_mutex); [ 5839.926986]It really means
<swap, set
GFP_FS>
GFP_FSsimply marks the beginning of a nested, unrelated context that uses the same thread, just like an interrupt. Kudos to lockdep for catching that.
Quoted 6 lines
Quoted 2 lines
I think the allocation in
blkio_alloc_blkg_stats()should be moved out of the I/O path into some init function. Copying Jens.That's completely buggy, basically you end up with a
GFP_KERNELallocation from the IO submit path. Vivek,per_cpudata needs to be set up at init time. You can't allocate it dynamically off the IO path.
Ok, I continued to develop on the patch which tries to allocate per cpu stats from worker thread context. Here is the patch.
Can the reporter please try out the patch and see if it helps. I am not sure if deadlock was because of mutex issue or not, but it should help get rid of lockdep warning.
This patch is on top of for-3.3/core branch of jens's linux-block tree. If it does not apply on your kernel version, do let me know the version you are testing with and I will generate another version of patch.
If testing results are good, I will break down the patch in small pieces and post as a series separately.
Thanks
Vivek
Alloc per cpu data from a worker thread context to avoid possibility of a deadlock under memory pressure.
Only side affect of delayed allocation is that we will lose the blkio cgroup stats for that group a small duration.
This patch is generated on top of "for-3.3/core" branch of linux-block tree.
--- block/blk-cgroup.c | 31 ++++++++++++- block/blk-cgroup.h | 9 +++ block/blk-throttle.c | 116 ++++++++++++++++++++++++++++++++----------------- block/cfq-iosched.c | 119 ++++++++++++++++++++++++++++++--------------------- 4 files changed, 180 insertions(+), 95 deletions(-)
blk-throttle.c renamed from blk-throttle.c.orig +75 -41
@@ -81,6 +81,7 @@ struct throtl_grp {int limits_changed;struct rcu_head rcu_head; +struct work_struct stat_alloc_work; };struct throtl_data
@@ -159,6 +160,13 @@ static void throtl_free_tg(struct rcu_hestruct throtl_grp *tg; tg = container_of(head, struct throtl_grp, rcu_head); + +/* +* Will delayed allocation be visible here for sure? I am relying +* on the fact that after blkg.stats_cpu assignment, we drop +* reference to group using atomic_dec() which should imply +* barrier +*/ free_percpu (tg->blkg.stats_cpu);kfree (tg); }
@@ -181,6 +189,48 @@ static void throtl_put_tg(struct throtl_call_rcu (&tg->rcu_head, throtl_free_tg); } +static inline void throtl_check_schedule_pcpu_stat_alloc (struct throtl_grp *tg) { +if (tg->blkg.stats_cpu !=NULL ) +return ; +/* +* Schedule the group per cpu stat allocation through worker +* thread +*/ +throtl_ref_get_tg (tg); +if (!schedule_work (&tg->stat_alloc_work)) { +/* Work is already scheduled by somebody */ +throtl_put_tg (tg); + } +} + +/* No locks taken. A reference to throtl_grp taken before invocation */ +static void tg_stat_alloc_fn (struct work_struct *work) +{ + struct throtl_grp *tg = container_of(work, struct throtl_grp, + stat_alloc_work); +void *stat_ptr =NULL ; +unsigned long ret; + + stat_ptr =blkio_alloc_blkg_stats_pcpu (); + +if (stat_ptr ==NULL ) { +/* If memory allocation failed, try again */ +throtl_check_schedule_pcpu_stat_alloc (tg); +throtl_put_tg (tg); +return ; + } + + ret =blkio_cmpxchg_blkg_stats (&tg->blkg,0 , + (unsigned long )stat_ptr); + +if (ret != (unsigned long )stat_ptr) { +/* Somebody else got to it first */ +free_percpu (tg->blkg.stats_cpu); + } + +throtl_put_tg (tg); +} +static void throtl_init_group (struct throtl_grp *tg) {INIT_HLIST_NODE (&tg->tg_node);
@@ -188,6 +238,7 @@ static void throtl_init_group(struct thrbio_list_init (&tg->bio_lists[0 ]);bio_list_init (&tg->bio_lists[1 ]); tg->limits_changed = false; +INIT_WORK (&tg->stat_alloc_work, tg_stat_alloc_fn);/* Practically unlimited BW */ tg->bps[0 ] = tg->bps[1 ] =-1 ;
@@ -263,8 +314,25 @@ static void throtl_init_add_tg_lists(strthrotl_add_group_to_td_list (td, tg); } -/* Should be called without queue lock and outside of rcu period */ -static struct throtl_grp *throtl_alloc_tg (struct throtl_data *td) +/* Allocates per cpu stats asynchronously with the help of worker thread */ +static struct throtl_grp *throtl_alloc_tg_async (struct throtl_data *td) +{ +struct throtl_grp *tg =NULL ; + + tg =kzalloc_node (sizeof (*tg),GFP_ATOMIC , td->queue->node); +if (!tg) +return NULL ; + +throtl_init_group (tg); +throtl_check_schedule_pcpu_stat_alloc (tg); +return tg; +} + +/* +* Should be called without queue lock and outside of rcu period as per +* cpu alloc will block +*/ +static struct throtl_grp *throtl_alloc_tg_sync (struct throtl_data *td) {struct throtl_grp *tg =NULL ;int ret;
@@ -273,7 +341,7 @@ static struct throtl_grp *throtl_alloc_tif (!tg)return NULL ; - ret =blkio_alloc_blkg_stats (&tg->blkg); + ret =blkio_alloc_set_blkg_stats (&tg->blkg);if (ret) {kfree (tg);
@@ -305,7 +373,7 @@ throtl_grp *throtl_find_tg(struct throtlstatic struct throtl_grp *throtl_get_tg (struct throtl_data *td) { -struct throtl_grp *tg =NULL , *__tg =NULL ; +struct throtl_grp *tg =NULL ;struct blkio_cgroup *blkcg;struct request_queue *q = td->queue;
@@ -321,46 +389,12 @@ static struct throtl_grp * throtl_get_tgreturn tg; } -/* -* Need to allocate a group. Allocation of group also needs allocation -* of per cpu stats which in-turn takes a mutex() and can block. Hence -* we need to drop rcu lock and queue_lock before we call alloc. -*/ -rcu_read_unlock (); -spin_unlock_irq (q->queue_lock); - - tg =throtl_alloc_tg (td); - -/* Group allocated and queue is still alive. take the lock */ -spin_lock_irq (q->queue_lock); - -/* Make sure @q is still alive */ -if (unlikely (test_bit (QUEUE_FLAG_DEAD , &q->queue_flags))) { -kfree (tg); -return NULL ; - } - -/* -* Initialize the new group. After sleeping, read the blkcg again. -*/ -rcu_read_lock (); - blkcg =task_blkio_cgroup (current); - -/* -* If some other thread already allocated the group while we were -* not holding queue lock, free up the group -*/ - __tg =throtl_find_tg (td, blkcg); - -if (__tg) { -kfree (tg); -rcu_read_unlock (); -return __tg; - } + tg =throtl_alloc_tg_async (td);/* Group allocation failed. Account the IO to root group */ if (!tg) { tg = td->root_tg; +rcu_read_unlock ();return tg; }
@@ -1254,7 +1288,7 @@ int blk_throtl_init(struct request_queue/* alloc and Init root group. */ td->queue = q; - tg =throtl_alloc_tg (td); + tg =throtl_alloc_tg_sync (td);if (!tg) {kfree (td);
blk-cgroup.c renamed from blk-cgroup.c.orig +28 -3
@@ -400,6 +400,9 @@ void blkiocg_update_dispatch_stats(strucstruct blkio_group_stats_cpu *stats_cpu;unsigned long flags; +if (blkg->stats_cpu ==NULL ) +return ; +/* * Disabling interrupts to provide mutual exclusion between two * writes on same cpu. It probably is not needed for 64bit. Not
@@ -446,6 +449,9 @@ void blkiocg_update_io_merged_stats(strustruct blkio_group_stats_cpu *stats_cpu;unsigned long flags; +if (blkg->stats_cpu ==NULL ) +return ; +/* * Disabling interrupts to provide mutual exclusion between two * writes on same cpu. It probably is not needed for 64bit. Not
@@ -465,9 +471,11 @@ EXPORT_SYMBOL_GPL(blkiocg_update_io_merg/* * This function allocates the per cpu stats for blkio_group. Should be called -* from sleepable context as alloc_per_cpu() requires that. +* from sleepable context as alloc_per_cpu() requires that. percpu alloc does +* not take any flags and does GFP_KERNEL allocations. Don't use it from +* IO submission path which usually might require GFP_NOIO. */ -int blkio_alloc_blkg_stats (struct blkio_group *blkg) +int blkio_alloc_set_blkg_stats (struct blkio_group *blkg) {/* Allocate memory for per cpu stats */ blkg->stats_cpu = alloc_percpu(struct blkio_group_stats_cpu);
@@ -475,7 +483,14 @@ int blkio_alloc_blkg_stats(struct blkio_return -ENOMEM ;return 0 ; } -EXPORT_SYMBOL_GPL (blkio_alloc_blkg_stats); +EXPORT_SYMBOL_GPL (blkio_alloc_set_blkg_stats); + +void *blkio_alloc_blkg_stats_pcpu (void ) +{ +/* Allocate memory for per cpu stats */ + return alloc_percpu(struct blkio_group_stats_cpu); +} +EXPORT_SYMBOL_GPL (blkio_alloc_blkg_stats_pcpu); void blkiocg_add_blkio_group(struct blkio_cgroup *blkcg, struct blkio_group *blkg, void *key, dev_t dev,
@@ -551,6 +566,12 @@ static void blkio_reset_stats_cpu(struct {struct blkio_group_stats_cpu *stats_cpu;int i, j, k; + +/* blkg->stats_cpu can have delayed allocation */ + +if (!blkg->stats_cpu) +return ; +/* * Note: On 64 bit arch this should not be an issue. This has the * possibility of returning some inconsistent value on 32bit arch
@@ -673,6 +694,10 @@ static uint64_t blkio_read_stat_cpu(strustruct blkio_group_stats_cpu *stats_cpu;u64 val =0 , tval; +/* blkg->stats_cpu might not have been allocated yet */ +if (blkg->stats_cpu ==NULL ) +return 0 ; +for_each_possible_cpu (cpu) {unsigned int start; stats_cpu =per_cpu_ptr (blkg->stats_cpu, cpu);
blk-cgroup.h renamed from blk-cgroup.h.orig +7 -2
@@ -310,7 +310,12 @@ extern struct blkio_cgroup *task_blkio_cextern void blkiocg_add_blkio_group (struct blkio_cgroup *blkcg,struct blkio_group *blkg,void *key,dev_t dev,enum blkio_policy_id plid); -extern int blkio_alloc_blkg_stats (struct blkio_group *blkg); +extern int blkio_alloc_set_blkg_stats (struct blkio_group *blkg); +extern void *blkio_alloc_blkg_stats_pcpu (void ); + +#define blkio_cmpxchg_blkg_stats (blkg, oldval, newval) \ + cmpxchg((unsigned long *)&(blkg)->stats_cpu, oldval, newval); +extern int blkiocg_del_blkio_group (struct blkio_group *blkg);extern struct blkio_group *blkiocg_lookup_group (struct blkio_cgroup *blkcg,void *key);
@@ -338,7 +343,7 @@ static inline void blkiocg_add_blkio_gro struct blkio_group *blkg, void *key, dev_t dev, enum blkio_policy_id plid) {} -static inline int blkio_alloc_blkg_stats(struct blkio_group *blkg) { return 0; } +static inline int blkio_alloc_set_blkg_stats(struct blkio_group *blkg) { return 0; }static inline int blkiocg_del_blkio_group (struct blkio_group *blkg) {return 0 ; }
cfq-iosched.c renamed from cfq-iosched.c.orig +70 -49
203 lines not on this page · raw message
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [RFT PATCH] blkio: alloc per cpu data from worker thread context( Re: kvm deadlock)
Quoted 16 lines · Nate Custer
Quoted 14 lines · Vivek Goyal
Ok, I continued to develop on the patch which tries to allocate per cpu stats from worker thread context. Here is the patch.
Can the reporter please try out the patch and see if it helps. I am not sure if deadlock was because of mutex issue or not, but it should help get rid of lockdep warning.
This patch is on top of for-3.3/core branch of jens's linux-block tree. If it does not apply on your kernel version, do let me know the version you are testing with and I will generate another version of patch.
If testing results are good, I will break down the patch in small pieces and post as a series separately.
Quoted 8 lines
Quoted 2 lines
Thanks
VivekRunning on a fedora-16 box with the patch applied to the linux-block tree I still had deadlocks. In fact it seemed to happen much faster and with ligher workloads.
I was able to get netconsole setup and a full stacktrace is posted here:
Thanks for testing it Nate. I did some debugging and found out that patch is doing double free on per cpu pointer hence the crash you are running into. I could reproduce this problem on my box. It is just a matter of doing rmdir on the blkio cgroup.
I understood the cmpxchg() semantics wrong. I have fixed it now and no crashes on directory removal. Can you please give this version a try.
Thanks
Vivek
Alloc per cpu data from a worker thread context to avoid possibility of a deadlock under memory pressure.
Only side affect of delayed allocation is that we will lose the blkio cgroup stats for that group a small duration.
This patch is generated on top of "for-3.3/core" branch of linux-block tree.
-v2:
Fixed the issue of double free on percpu pointer.
--- block/blk-cgroup.c | 31 ++++++++++++- block/blk-cgroup.h | 9 +++ block/blk-throttle.c | 116 ++++++++++++++++++++++++++++++++----------------- block/cfq-iosched.c | 119 ++++++++++++++++++++++++++++++--------------------- 4 files changed, 180 insertions(+), 95 deletions(-)
blk-throttle.c renamed from blk-throttle.c.orig +75 -41
@@ -81,6 +81,7 @@ struct throtl_grp {int limits_changed;struct rcu_head rcu_head; +struct work_struct stat_alloc_work; };struct throtl_data
@@ -159,6 +160,13 @@ static void throtl_free_tg(struct rcu_hestruct throtl_grp *tg; tg = container_of(head, struct throtl_grp, rcu_head); + +/* +* Will delayed allocation be visible here for sure? I am relying +* on the fact that after blkg.stats_cpu assignment, we drop +* reference to group using atomic_dec() which should imply +* barrier +*/ free_percpu (tg->blkg.stats_cpu);kfree (tg); }
@@ -181,6 +189,48 @@ static void throtl_put_tg(struct throtl_call_rcu (&tg->rcu_head, throtl_free_tg); } +static inline void throtl_check_schedule_pcpu_stat_alloc (struct throtl_grp *tg) { +if (tg->blkg.stats_cpu !=NULL ) +return ; +/* +* Schedule the group per cpu stat allocation through worker +* thread +*/ +throtl_ref_get_tg (tg); +if (!schedule_work (&tg->stat_alloc_work)) { +/* Work is already scheduled by somebody */ +throtl_put_tg (tg); + } +} + +/* No locks taken. A reference to throtl_grp taken before invocation */ +static void tg_stat_alloc_fn (struct work_struct *work) +{ + struct throtl_grp *tg = container_of(work, struct throtl_grp, + stat_alloc_work); +void *stat_ptr =NULL ; +unsigned long ret; + + stat_ptr =blkio_alloc_blkg_stats_pcpu (); + +if (stat_ptr ==NULL ) { +/* If memory allocation failed, try again */ +throtl_check_schedule_pcpu_stat_alloc (tg); +throtl_put_tg (tg); +return ; + } + + ret =blkio_cmpxchg_blkg_stats (&tg->blkg,0 , + (unsigned long )stat_ptr); + +if (ret !=0 ) { +/* Somebody else got to it first */ +free_percpu (stat_ptr); + } + +throtl_put_tg (tg); +} +static void throtl_init_group (struct throtl_grp *tg) {INIT_HLIST_NODE (&tg->tg_node);
@@ -188,6 +238,7 @@ static void throtl_init_group(struct thrbio_list_init (&tg->bio_lists[0 ]);bio_list_init (&tg->bio_lists[1 ]); tg->limits_changed = false; +INIT_WORK (&tg->stat_alloc_work, tg_stat_alloc_fn);/* Practically unlimited BW */ tg->bps[0 ] = tg->bps[1 ] =-1 ;
@@ -263,8 +314,25 @@ static void throtl_init_add_tg_lists(strthrotl_add_group_to_td_list (td, tg); } -/* Should be called without queue lock and outside of rcu period */ -static struct throtl_grp *throtl_alloc_tg (struct throtl_data *td) +/* Allocates per cpu stats asynchronously with the help of worker thread */ +static struct throtl_grp *throtl_alloc_tg_async (struct throtl_data *td) +{ +struct throtl_grp *tg =NULL ; + + tg =kzalloc_node (sizeof (*tg),GFP_ATOMIC , td->queue->node); +if (!tg) +return NULL ; + +throtl_init_group (tg); +throtl_check_schedule_pcpu_stat_alloc (tg); +return tg; +} + +/* +* Should be called without queue lock and outside of rcu period as per +* cpu alloc will block +*/ +static struct throtl_grp *throtl_alloc_tg_sync (struct throtl_data *td) {struct throtl_grp *tg =NULL ;int ret;
@@ -273,7 +341,7 @@ static struct throtl_grp *throtl_alloc_tif (!tg)return NULL ; - ret =blkio_alloc_blkg_stats (&tg->blkg); + ret =blkio_alloc_set_blkg_stats (&tg->blkg);if (ret) {kfree (tg);
@@ -305,7 +373,7 @@ throtl_grp *throtl_find_tg(struct throtlstatic struct throtl_grp *throtl_get_tg (struct throtl_data *td) { -struct throtl_grp *tg =NULL , *__tg =NULL ; +struct throtl_grp *tg =NULL ;struct blkio_cgroup *blkcg;struct request_queue *q = td->queue;
@@ -321,46 +389,12 @@ static struct throtl_grp * throtl_get_tgreturn tg; } -/* -* Need to allocate a group. Allocation of group also needs allocation -* of per cpu stats which in-turn takes a mutex() and can block. Hence -* we need to drop rcu lock and queue_lock before we call alloc. -*/ -rcu_read_unlock (); -spin_unlock_irq (q->queue_lock); - - tg =throtl_alloc_tg (td); - -/* Group allocated and queue is still alive. take the lock */ -spin_lock_irq (q->queue_lock); - -/* Make sure @q is still alive */ -if (unlikely (test_bit (QUEUE_FLAG_DEAD , &q->queue_flags))) { -kfree (tg); -return NULL ; - } - -/* -* Initialize the new group. After sleeping, read the blkcg again. -*/ -rcu_read_lock (); - blkcg =task_blkio_cgroup (current); - -/* -* If some other thread already allocated the group while we were -* not holding queue lock, free up the group -*/ - __tg =throtl_find_tg (td, blkcg); - -if (__tg) { -kfree (tg); -rcu_read_unlock (); -return __tg; - } + tg =throtl_alloc_tg_async (td);/* Group allocation failed. Account the IO to root group */ if (!tg) { tg = td->root_tg; +rcu_read_unlock ();return tg; }
@@ -1254,7 +1288,7 @@ int blk_throtl_init(struct request_queue/* alloc and Init root group. */ td->queue = q; - tg =throtl_alloc_tg (td); + tg =throtl_alloc_tg_sync (td);if (!tg) {kfree (td);
blk-cgroup.c renamed from blk-cgroup.c.orig +28 -3
@@ -400,6 +400,9 @@ void blkiocg_update_dispatch_stats(strucstruct blkio_group_stats_cpu *stats_cpu;unsigned long flags; +if (blkg->stats_cpu ==NULL ) +return ; +/* * Disabling interrupts to provide mutual exclusion between two * writes on same cpu. It probably is not needed for 64bit. Not
@@ -446,6 +449,9 @@ void blkiocg_update_io_merged_stats(strustruct blkio_group_stats_cpu *stats_cpu;unsigned long flags; +if (blkg->stats_cpu ==NULL ) +return ; +/* * Disabling interrupts to provide mutual exclusion between two * writes on same cpu. It probably is not needed for 64bit. Not
@@ -465,9 +471,11 @@ EXPORT_SYMBOL_GPL(blkiocg_update_io_merg/* * This function allocates the per cpu stats for blkio_group. Should be called -* from sleepable context as alloc_per_cpu() requires that. +* from sleepable context as alloc_per_cpu() requires that. percpu alloc does +* not take any flags and does GFP_KERNEL allocations. Don't use it from +* IO submission path which usually might require GFP_NOIO. */ -int blkio_alloc_blkg_stats (struct blkio_group *blkg) +int blkio_alloc_set_blkg_stats (struct blkio_group *blkg) {/* Allocate memory for per cpu stats */ blkg->stats_cpu = alloc_percpu(struct blkio_group_stats_cpu);
@@ -475,7 +483,14 @@ int blkio_alloc_blkg_stats(struct blkio_return -ENOMEM ;return 0 ; } -EXPORT_SYMBOL_GPL (blkio_alloc_blkg_stats); +EXPORT_SYMBOL_GPL (blkio_alloc_set_blkg_stats); + +void *blkio_alloc_blkg_stats_pcpu (void ) +{ +/* Allocate memory for per cpu stats */ + return alloc_percpu(struct blkio_group_stats_cpu); +} +EXPORT_SYMBOL_GPL (blkio_alloc_blkg_stats_pcpu); void blkiocg_add_blkio_group(struct blkio_cgroup *blkcg, struct blkio_group *blkg, void *key, dev_t dev,
@@ -551,6 +566,12 @@ static void blkio_reset_stats_cpu(struct {struct blkio_group_stats_cpu *stats_cpu;int i, j, k; + +/* blkg->stats_cpu can have delayed allocation */ + +if (!blkg->stats_cpu) +return ; +/* * Note: On 64 bit arch this should not be an issue. This has the * possibility of returning some inconsistent value on 32bit arch
@@ -673,6 +694,10 @@ static uint64_t blkio_read_stat_cpu(strustruct blkio_group_stats_cpu *stats_cpu;u64 val =0 , tval; +/* blkg->stats_cpu might not have been allocated yet */ +if (blkg->stats_cpu ==NULL ) +return 0 ; +for_each_possible_cpu (cpu) {unsigned int start; stats_cpu =per_cpu_ptr (blkg->stats_cpu, cpu);
blk-cgroup.h renamed from blk-cgroup.h.orig +7 -2
@@ -310,7 +310,12 @@ extern struct blkio_cgroup *task_blkio_cextern void blkiocg_add_blkio_group (struct blkio_cgroup *blkcg,struct blkio_group *blkg,void *key,dev_t dev,enum blkio_policy_id plid); -extern int blkio_alloc_blkg_stats (struct blkio_group *blkg); +extern int blkio_alloc_set_blkg_stats (struct blkio_group *blkg); +extern void *blkio_alloc_blkg_stats_pcpu (void ); + +#define blkio_cmpxchg_blkg_stats (blkg, oldval, newval) \ + cmpxchg((unsigned long *)&(blkg)->stats_cpu, oldval, newval); +extern int blkiocg_del_blkio_group (struct blkio_group *blkg);extern struct blkio_group *blkiocg_lookup_group (struct blkio_cgroup *blkcg,void *key);
@@ -338,7 +343,7 @@ static inline void blkiocg_add_blkio_gro struct blkio_group *blkg, void *key, dev_t dev, enum blkio_policy_id plid) {} -static inline int blkio_alloc_blkg_stats(struct blkio_group *blkg) { return 0; } +static inline int blkio_alloc_set_blkg_stats(struct blkio_group *blkg) { return 0; }static inline int blkiocg_del_blkio_group (struct blkio_group *blkg) {return 0 ; }
cfq-iosched.c renamed from cfq-iosched.c.orig +70 -49
203 lines not on this page · raw message
Re: [RFT PATCH] blkio: alloc per cpu data from worker thread context( Re: kvm deadlock)
Quoted 11 lines · Vivek Goyal
Thanks for testing it Nate. I did some debugging and found out that patch is doing double free on per cpu pointer hence the crash you are running into. I could reproduce this problem on my box. It is just a matter of doing rmdir on the blkio cgroup.
I understood the
cmpxchg()semantics wrong. I have fixed it now and no crashes on directory removal. Can you please give this version a try.Thanks
Vivek
After 24 hours of stress testing the machine remains up and working without issue. I will continue to test it, but am reasonably confident that this patch resolves my issue.
Nate Custer
Re: [RFT PATCH] blkio: alloc per cpu data from worker thread context( Re: kvm deadlock)
Quoted 7 lines · Nate Custer
Quoted 5 lines · Vivek Goyal
Thanks for testing it Nate. I did some debugging and found out that patch is doing double free on per cpu pointer hence the crash you are running into. I could reproduce this problem on my box. It is just a matter of doing rmdir on the blkio cgroup.
Quoted 8 lines
Quoted 6 lines
I understood the
cmpxchg()semantics wrong. I have fixed it now and no crashes on directory removal. Can you please give this version a try.Thanks
VivekAfter 24 hours of stress testing the machine remains up and working without issue. I will continue to test it, but am reasonably confident that this patch resolves my issue.
That's good to know. While you continue to stress test, I will make some minor modifications to the patch. (Thinking of delaying the retry of allocation of per cpu memory in case previous attempts failed).
Thanks
Vivek
Vivek Goyal
Tested-by Nate CusterRe: [RFT PATCH] blkio: alloc per cpu data from worker thread context( Re: kvm deadlock)
In reply to Nate CusterQuoted 7 lines · Nate Custer
Quoted 5 lines · Vivek Goyal
Thanks for testing it Nate. I did some debugging and found out that patch is doing double free on per cpu pointer hence the crash you are running into. I could reproduce this problem on my box. It is just a matter of doing rmdir on the blkio cgroup.
Quoted 9 lines
Quoted 6 lines
I understood the
cmpxchg()semantics wrong. I have fixed it now and no crashes on directory removal. Can you please give this version a try.Thanks
VivekAfter 24 hours of stress testing the machine remains up and working without issue. I will continue to test it, but am reasonably confident that this patch resolves my issue.
Hi Nate,
I have come up with final version of the patch. This time I have used non rentrant work queue to queue the stat alloc work. This also gets rid of cmpxchg code as there is only one writer at a time. There are couple of other small cleanups.
Can you please give patch also a try to make sure I have not broken something while doing changes.
This version is based on 3.2-rc6. Once you confirm the results, I will rebase it on top of "linux-block for-3.3/core" and post it to Jens for inclusion.
Thanks
Vivek
Block cgroup currently allocates percpu data for some stats. This allocation is happening in IO submission path which can recurse in IO stack.
Percpu allocation API does not take any allocation flags as input, hence to avoid the deadlock problem under memory pressure, alloc per cpu data from a worker thread context.
Only side affect of delayed allocation is that we will lose the blkio cgroup stats for that group a small duration.
In case per cpu memory allocation fails, worker thread re-arms the work with a delay of 1 second.
This patch is generated on top of 3.2-rc6
--- block/blk-cgroup.c | 31 ++++++++++++- block/blk-cgroup.h | 11 +++- block/blk-throttle.c | 115 ++++++++++++++++++++++++++++++++------------------- block/cfq-iosched.c | 111 +++++++++++++++++++++++++++---------------------- 4 files changed, 173 insertions(+), 95 deletions(-)
blk-throttle.c renamed from blk-throttle.c.orig +74 -41
@@ -81,6 +81,7 @@ struct throtl_grp {int limits_changed;struct rcu_head rcu_head; +struct delayed_work stat_alloc_work; };struct throtl_data
@@ -159,6 +160,12 @@ static void throtl_free_tg(struct rcu_hestruct throtl_grp *tg; tg = container_of(head, struct throtl_grp, rcu_head); + +/* +* delayed per cpu allocation should be visible here due to +* barrier guarantees offered by atomic_dec_and_test() called +* during dropping reference. +*/ free_percpu (tg->blkg.stats_cpu);kfree (tg); }
@@ -181,6 +188,48 @@ static void throtl_put_tg(struct throtl_call_rcu (&tg->rcu_head, throtl_free_tg); } +static inline void throtl_schedule_pcpu_stat_alloc (struct throtl_grp *tg, +unsigned long delay) { +WARN_ON (tg->blkg.stats_cpu !=NULL ); + +/* +* Schedule the group per cpu stat allocation through worker +* thread +*/ +throtl_ref_get_tg (tg); + +/* queue work on non-rentrant work queue */ +queue_delayed_work (system_nrt_wq, &tg->stat_alloc_work, delay); +} + +/* No locks taken. A reference to throtl_grp taken before invocation */ +static void tg_stat_alloc_fn (struct work_struct *work) +{ +struct delayed_work *dwork =to_delayed_work (work); + struct throtl_grp *tg = container_of(dwork, struct throtl_grp, + stat_alloc_work); +void *stat_ptr =NULL ; + + stat_ptr =blkio_alloc_blkg_stats_pcpu (); + +if (stat_ptr ==NULL ) { +/* +* If memory allocation failed, try again with delay of 1s +* FIXME: Put an upper limit on number of retries? +*/ +throtl_schedule_pcpu_stat_alloc (tg,HZ ); +throtl_put_tg (tg); +return ; + } + +/* +* There is only one writer at a time and that is worker thread. Hence +* no synchronization should be required. +*/ +blkio_set_blkg_pcpu_stats (&tg->blkg, stat_ptr); +throtl_put_tg (tg); +} +static void throtl_init_group (struct throtl_grp *tg) {INIT_HLIST_NODE (&tg->tg_node);
@@ -188,6 +237,7 @@ static void throtl_init_group(struct thrbio_list_init (&tg->bio_lists[0 ]);bio_list_init (&tg->bio_lists[1 ]); tg->limits_changed = false; +INIT_DELAYED_WORK (&tg->stat_alloc_work, tg_stat_alloc_fn);/* Practically unlimited BW */ tg->bps[0 ] = tg->bps[1 ] =-1 ;
@@ -263,8 +313,25 @@ static void throtl_init_add_tg_lists(strthrotl_add_group_to_td_list (td, tg); } -/* Should be called without queue lock and outside of rcu period */ -static struct throtl_grp *throtl_alloc_tg (struct throtl_data *td) +/* Allocates per cpu stats asynchronously with the help of worker thread */ +static struct throtl_grp *throtl_alloc_tg_async (struct throtl_data *td) +{ +struct throtl_grp *tg =NULL ; + + tg =kzalloc_node (sizeof (*tg),GFP_ATOMIC , td->queue->node); +if (!tg) +return NULL ; + +throtl_init_group (tg); +throtl_schedule_pcpu_stat_alloc (tg,0 ); +return tg; +} + +/* +* Should be called without queue lock and outside of rcu period as per +* cpu alloc will block +*/ +static struct throtl_grp *throtl_alloc_tg_sync (struct throtl_data *td) {struct throtl_grp *tg =NULL ;int ret;
@@ -273,7 +340,7 @@ static struct throtl_grp *throtl_alloc_tif (!tg)return NULL ; - ret =blkio_alloc_blkg_stats (&tg->blkg); + ret =blkio_alloc_set_blkg_stats (&tg->blkg);if (ret) {kfree (tg);
@@ -305,7 +372,7 @@ throtl_grp *throtl_find_tg(struct throtlstatic struct throtl_grp *throtl_get_tg (struct throtl_data *td) { -struct throtl_grp *tg =NULL , *__tg =NULL ; +struct throtl_grp *tg =NULL ;struct blkio_cgroup *blkcg;struct request_queue *q = td->queue;
@@ -321,46 +388,12 @@ static struct throtl_grp * throtl_get_tgreturn tg; } -/* -* Need to allocate a group. Allocation of group also needs allocation -* of per cpu stats which in-turn takes a mutex() and can block. Hence -* we need to drop rcu lock and queue_lock before we call alloc. -*/ -rcu_read_unlock (); -spin_unlock_irq (q->queue_lock); - - tg =throtl_alloc_tg (td); - -/* Group allocated and queue is still alive. take the lock */ -spin_lock_irq (q->queue_lock); - -/* Make sure @q is still alive */ -if (unlikely (test_bit (QUEUE_FLAG_DEAD , &q->queue_flags))) { -kfree (tg); -return NULL ; - } - -/* -* Initialize the new group. After sleeping, read the blkcg again. -*/ -rcu_read_lock (); - blkcg =task_blkio_cgroup (current); - -/* -* If some other thread already allocated the group while we were -* not holding queue lock, free up the group -*/ - __tg =throtl_find_tg (td, blkcg); - -if (__tg) { -kfree (tg); -rcu_read_unlock (); -return __tg; - } + tg =throtl_alloc_tg_async (td);/* Group allocation failed. Account the IO to root group */ if (!tg) { tg = td->root_tg; +rcu_read_unlock ();return tg; }
@@ -1254,7 +1287,7 @@ int blk_throtl_init(struct request_queue/* alloc and Init root group. */ td->queue = q; - tg =throtl_alloc_tg (td); + tg =throtl_alloc_tg_sync (td);if (!tg) {kfree (td);
blk-cgroup.c renamed from blk-cgroup.c.orig +28 -3
@@ -400,6 +400,9 @@ void blkiocg_update_dispatch_stats(strucstruct blkio_group_stats_cpu *stats_cpu;unsigned long flags; +if (blkg->stats_cpu ==NULL ) +return ; +/* * Disabling interrupts to provide mutual exclusion between two * writes on same cpu. It probably is not needed for 64bit. Not
@@ -446,6 +449,9 @@ void blkiocg_update_io_merged_stats(strustruct blkio_group_stats_cpu *stats_cpu;unsigned long flags; +if (blkg->stats_cpu ==NULL ) +return ; +/* * Disabling interrupts to provide mutual exclusion between two * writes on same cpu. It probably is not needed for 64bit. Not
@@ -465,9 +471,11 @@ EXPORT_SYMBOL_GPL(blkiocg_update_io_merg/* * This function allocates the per cpu stats for blkio_group. Should be called -* from sleepable context as alloc_per_cpu() requires that. +* from sleepable context as alloc_per_cpu() requires that. percpu alloc does +* not take any flags and does GFP_KERNEL allocations. Don't use it from +* IO submission path which usually might require GFP_NOIO. */ -int blkio_alloc_blkg_stats (struct blkio_group *blkg) +int blkio_alloc_set_blkg_stats (struct blkio_group *blkg) {/* Allocate memory for per cpu stats */ blkg->stats_cpu = alloc_percpu(struct blkio_group_stats_cpu);
@@ -475,7 +483,14 @@ int blkio_alloc_blkg_stats(struct blkio_return -ENOMEM ;return 0 ; } -EXPORT_SYMBOL_GPL (blkio_alloc_blkg_stats); +EXPORT_SYMBOL_GPL (blkio_alloc_set_blkg_stats); + +void *blkio_alloc_blkg_stats_pcpu (void ) +{ +/* Allocate memory for per cpu stats */ + return alloc_percpu(struct blkio_group_stats_cpu); +} +EXPORT_SYMBOL_GPL (blkio_alloc_blkg_stats_pcpu); void blkiocg_add_blkio_group(struct blkio_cgroup *blkcg, struct blkio_group *blkg, void *key, dev_t dev,
@@ -551,6 +566,12 @@ static void blkio_reset_stats_cpu(struct {struct blkio_group_stats_cpu *stats_cpu;int i, j, k; + +/* blkg->stats_cpu can have delayed allocation */ + +if (!blkg->stats_cpu) +return ; +/* * Note: On 64 bit arch this should not be an issue. This has the * possibility of returning some inconsistent value on 32bit arch
@@ -673,6 +694,10 @@ static uint64_t blkio_read_stat_cpu(strustruct blkio_group_stats_cpu *stats_cpu;u64 val =0 , tval; +/* blkg->stats_cpu might not have been allocated yet */ +if (blkg->stats_cpu ==NULL ) +return 0 ; +for_each_possible_cpu (cpu) {unsigned int start; stats_cpu =per_cpu_ptr (blkg->stats_cpu, cpu);
blk-cgroup.h renamed from blk-cgroup.h.orig +9 -2
@@ -310,7 +310,14 @@ extern struct blkio_cgroup *task_blkio_cextern void blkiocg_add_blkio_group (struct blkio_cgroup *blkcg,struct blkio_group *blkg,void *key,dev_t dev,enum blkio_policy_id plid); -extern int blkio_alloc_blkg_stats (struct blkio_group *blkg); +extern int blkio_alloc_set_blkg_stats (struct blkio_group *blkg); +extern void *blkio_alloc_blkg_stats_pcpu (void ); + +static inline void blkio_set_blkg_pcpu_stats (struct blkio_group *blkg,void *ptr) +{ + blkg->stats_cpu = ptr; +} +extern int blkiocg_del_blkio_group (struct blkio_group *blkg);extern struct blkio_group *blkiocg_lookup_group (struct blkio_cgroup *blkcg,void *key);
@@ -338,7 +345,7 @@ static inline void blkiocg_add_blkio_gro struct blkio_group *blkg, void *key, dev_t dev, enum blkio_policy_id plid) {} -static inline int blkio_alloc_blkg_stats(struct blkio_group *blkg) { return 0; } +static inline int blkio_alloc_set_blkg_stats(struct blkio_group *blkg) { return 0; }static inline int blkiocg_del_blkio_group (struct blkio_group *blkg) {return 0 ; }
cfq-iosched.c renamed from cfq-iosched.c.orig +62 -49
195 lines not on this page · raw message
Re: [RFT PATCH] blkio: alloc per cpu data from worker thread context( Re: kvm deadlock)
Quoted 45 lines · Vivek Goyal
Quoted 7 lines · Nate Custer
Quoted 5 lines · Vivek Goyal
Thanks for testing it Nate. I did some debugging and found out that patch is doing double free on per cpu pointer hence the crash you are running into. I could reproduce this problem on my box. It is just a matter of doing rmdir on the blkio cgroup.
Quoted 9 lines
Quoted 6 lines
I understood the
cmpxchg()semantics wrong. I have fixed it now and no crashes on directory removal. Can you please give this version a try.Thanks
VivekAfter 24 hours of stress testing the machine remains up and working without issue. I will continue to test it, but am reasonably confident that this patch resolves my issue.
Hi Nate,
I have come up with final version of the patch. This time I have used non rentrant work queue to queue the stat alloc work. This also gets rid of cmpxchg code as there is only one writer at a time. There are couple of other small cleanups.
Can you please give patch also a try to make sure I have not broken something while doing changes.
This version is based on 3.2-rc6. Once you confirm the results, I will rebase it on top of "linux-block for-3.3/core" and post it to Jens for inclusion.
Thanks
VivekBlock cgroup currently allocates percpu data for some stats. This allocation is happening in IO submission path which can recurse in IO stack.
Percpu allocation API does not take any allocation flags as input, hence to avoid the deadlock problem under memory pressure, alloc per cpu data from a worker thread context.
Only side affect of delayed allocation is that we will lose the blkio cgroup stats for that group a small duration.
Quoted 8 lines
In case per cpu memory allocation fails, worker thread re-arms the work with a delay of 1 second.
This patch is generated on top of 3.2-rc6
Signed-off-by Vivek Goyal <[email protected]>Reported-by Nate Custer <[email protected]>Tested-by Nate Custer <[email protected]>
Hmmm... I really don't like this approach. It's so unnecessarily complex with extra refcounting and all when about the same thing can be achieved by implementing simple mempool which is filled asynchronously. Also, the fix seems way too invasive even for -rc6, let alone -stable. If reverting isn't gonna be invasive, maybe that's a better approach for now?
I've been thinking about it and I think the use case is legit and maybe making percpu allocator support that isn't such a bad idea. I'm not completely sure yet tho. I'll give it a try later today.
Thank you.
Signature · 1 line
tejun
Re: [RFT PATCH] blkio: alloc per cpu data from worker thread context( Re: kvm deadlock)
Quoted 55 lines · Tejun Heo
Quoted 45 lines · Vivek Goyal
Quoted 7 lines · Nate Custer
Quoted 5 lines · Vivek Goyal
Thanks for testing it Nate. I did some debugging and found out that patch is doing double free on per cpu pointer hence the crash you are running into. I could reproduce this problem on my box. It is just a matter of doing rmdir on the blkio cgroup.
Quoted 9 lines
Quoted 6 lines
I understood the
cmpxchg()semantics wrong. I have fixed it now and no crashes on directory removal. Can you please give this version a try.Thanks
VivekAfter 24 hours of stress testing the machine remains up and working without issue. I will continue to test it, but am reasonably confident that this patch resolves my issue.
Hi Nate,
I have come up with final version of the patch. This time I have used non rentrant work queue to queue the stat alloc work. This also gets rid of cmpxchg code as there is only one writer at a time. There are couple of other small cleanups.
Can you please give patch also a try to make sure I have not broken something while doing changes.
This version is based on 3.2-rc6. Once you confirm the results, I will rebase it on top of "linux-block for-3.3/core" and post it to Jens for inclusion.
Thanks
VivekBlock cgroup currently allocates percpu data for some stats. This allocation is happening in IO submission path which can recurse in IO stack.
Percpu allocation API does not take any allocation flags as input, hence to avoid the deadlock problem under memory pressure, alloc per cpu data from a worker thread context.
Only side affect of delayed allocation is that we will lose the blkio cgroup stats for that group a small duration.
Quoted 8 lines
In case per cpu memory allocation fails, worker thread re-arms the work with a delay of 1 second.
This patch is generated on top of 3.2-rc6
Signed-off-by Vivek Goyal <[email protected]>Reported-by Nate Custer <[email protected]>Tested-by Nate Custer <[email protected]>
Quoted 6 lines
Hmmm... I really don't like this approach. It's so unnecessarily complex with extra refcounting and all when about the same thing can be achieved by implementing simple mempool which is filled asynchronously. Also, the fix seems way too invasive even for -rc6, let alone -stable. If reverting isn't gonna be invasive, maybe that's a better approach for now?
I think reverting the previous series is not going to be simple either. It had 13 patches.
https://lkml.org/lkml/2011/5/19/560
By making stats per cpu, I was able to reduce contention on request queue lock. Now we shall have to bring the lock back.
So to me, neither of the solution is very clean/simple. May be we can take this patch in as temporary stop gap measure for 3.2, while you cook a patch for per cpu allocator for 3.3 onwards.
This patch looks big, but is not very complex. Now allocation of group happens without dropping queue lock and it gets rid of all the trickery of dropping lock, reacquiring it and checking for queue dead etc.
Some code is related to making group refering atomic again. (Which was atomic in the past. So we just resotore that functionality).
The only core piece is taking a reference on the group and asking a worker thread to do the allocation. May be if I break the patch into small pieces, it will be more clear.
So if I had to choose between reverting the series vs taking this patch as stop gap measure, I will prefer taking this patch. It can get some soak time in Jens's tree and if everything looks good then push it for 3.2. If things look good there, then it can be pulled into -stable. So it will be a while before these changes are pulled into -stable.
Quoted 4 lines
I've been thinking about it and I think the use case is legit and maybe making percpu allocator support that isn't such a bad idea. I'm not completely sure yet tho. I'll give it a try later today.
Ok, that's good to know. If per cpu allocator can support this use case, it will be good for 3.3 onwards. This seems to be right way to go to fix the problem.
Thanks
Vivek
Re: [RFT PATCH] blkio: alloc per cpu data from worker thread context( Re: kvm deadlock)
Hello, Vivek.
Quoted 3 lines · Vivek Goyal
Ok, that's good to know. If per cpu allocator can support this use case, it will be good for 3.3 onwards. This seems to be right way to go to fix the problem.
Ummm... if we're gonna make percpu usable w/ GFP_NOIO, the right interim solution would be making a simplistic mempool so that later when percpu can do it, it can be swapped easily. I really can't see much benefit of adding refcnting on top of everything just for this.
Thanks.
Signature · 1 line
tejun
Re: [RFT PATCH] blkio: alloc per cpu data from worker thread context( Re: kvm deadlock)
Quoted 11 lines · Tejun Heo
Hello, Vivek.
Quoted 3 lines · Vivek Goyal
Ok, that's good to know. If per cpu allocator can support this use case, it will be good for 3.3 onwards. This seems to be right way to go to fix the problem.
Ummm... if we're gonna make percpu usable w/
GFP_NOIO, the right interim solution would be making a simplistic mempool so that later when percpu can do it, it can be swapped easily. I really can't see much benefit of adding refcnting on top of everything just for this.
Ok. So are you suggesting that I should write a simple mempool kind of implementation of my own for group and per cpu data allocation. Keep certain number of elements in the cache and trigger a worker thread to allocate more elements once minimum number of elements go below threshold. If pool has run out of pre-allocated elements then allocation will fail and IO will be accounted to root group?
I am looking at the mempool implementation (mempool_create()) and looks like that is not suitable for my use case. mempool_alloc() will call into alloc function provided by me and pass the flags. I can't implement an alloc function and honor that flag as per cpu alloc does not take any flags.
So IIUC, existing mempool implementation is not directly usable for my requirement and I need to write some code of my own for the caching layer which always allocates objects from reserve and fills in the pool asynchronously with the help of a worker thread.
Thanks
Vivek
Re: [RFT PATCH] blkio: alloc per cpu data from worker thread context( Re: kvm deadlock)
Hello,
Quoted 4 lines · Vivek Goyal
So IIUC, existing mempool implementation is not directly usable for my requirement and I need to write some code of my own for the caching layer which always allocates objects from reserve and fills in the pool asynchronously with the help of a worker thread.
I've been looking at it and don't think allowing percpu allocator to be called from no io path is a good idea. The on-demand area filling is tied into vmalloc area management which in turn is tied to arch page table code and I really want to avoid pre-allocating full chunk - it can be huge. I'm trying to extend mempool to cover percpu areas, so please wait a bit.
Thanks.
Signature · 1 line
tejun
Re: [RFT PATCH] blkio: alloc per cpu data from worker thread context( Re: kvm deadlock)
In reply to Vivek GoyalQuoted 7 lines · Vivek Goyal
I think reverting the previous series is not going to be simple either. It had 13 patches.
https://lkml.org/lkml/2011/5/19/560
By making stats per cpu, I was able to reduce contention on request queue lock. Now we shall have to bring the lock back.
That's not going to happen, having that lock in there was a disaster for small IO on fast devices. It's essentially the limiting factor on benchmark runs on the RHEL kernels that have it included and enabled...
Signature · 1 line
Jens Axboe