mm: memcontrol: correct the comment in mem_cgroup_swapout()
Subsystems (2)
PATCH 1/1 mm: memcontrol: correct the comment in mem_cgroup_swapout()
Clark stumbled over a VM_BUG_ON() in -RT which was then was removed by Johannes in commit f371763a79d ("mm: memcontrol: fix false-positive VM_BUG_ON() on -rt"). The comment before that patch was a tiny bit better than it is now. While the patch claimed to fix a false-postive on -RT this was not the case. None of the -RT folks ACKed it and it was not a false positive report. That was a *real* problem.
This patch updates the comment that is improper because it refers to "disabled preemption" as a consequence of that lock being taken. A spin_lock() disables preemption, true, but in this case the code relies on the fact that the lock _also_ disables interrupts once it is acquired. And this is the important detail (which was checked the VM_BUG_ON()) which needs to be pointed out. This is the hint one needs while looking at the code. It was explained by Johannes on the list that the per-CPU variables are protected by local_irq_save(). The BUG_ON() was helpful. This code has been workarounded in -RT in the meantime. I wouldn't mind running into more of those if the code in question uses *special* kind of locking since now there is no no verification (in terms of lockdep or BUG_ON()).
The two functions after the comment could also have a "local_irq_save()" dance around them in order to serialize access to the per-CPU variables. This has been avoided because the interrupts should be off.
--- mm/memcontrol.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
mm/memcontrol.c +6 -1
@@ -5835,7 +5835,12 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)if (!mem_cgroup_is_root (memcg))page_counter_uncharge (&memcg->memory,1 ); -/* Caller disabled preemption with mapping->tree_lock */ +/* +* Interrupts should be disabled here because the caller holds the +* mapping->tree_lock lock which is taken with interrupts-off. It is +* important here to have the interrupts disabled because it is the +* only synchronisation we have for udpating the per-CPU variables. +*/ mem_cgroup_charge_statistics (memcg, page,-1 );memcg_check_events (memcg, page); }
Signature · 7 lines
2.1.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Johannes Weiner
maintainerQuoted 6 lines · Sebastian Andrzej Siewior
Clark stumbled over a
VM_BUG_ON()in -RT which was then was removed by Johannes in commitf371763a79d("mm: memcontrol: fix false-positiveVM_BUG_ON()on -rt"). The comment before that patch was a tiny bit better than it is now. While the patch claimed to fix a false-postive on -RT this was not the case. None of the -RT folks ACKed it and it was not a false positive report. That was a *real* problem.
The real problem is that irqs_disabled() on -rt is returning false negatives. Having it return false within a spin_lock_irq() section is broken.
Quoted 11 lines
This patch updates the comment that is improper because it refers to "disabled preemption" as a consequence of that lock being taken. A
spin_lock()disables preemption, true, but in this case the code relies on the fact that the lock _also_ disables interrupts once it is acquired. And this is the important detail (which was checked theVM_BUG_ON()) which needs to be pointed out. This is the hint one needs while looking at the code. It was explained by Johannes on the list that the per-CPU variables are protected bylocal_irq_save(). TheBUG_ON()was helpful. This code has been workarounded in -RT in the meantime. I wouldn't mind running into more of those if the code in question uses *special* kind of locking since now there is no no verification (in terms of lockdep orBUG_ON()).
I'd be happy to re-instate the VM_BUG_ON that checks for disabled interrupts as before, that was the most obvious documentation.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 11 lines · Johannes Weiner
Quoted 6 lines · Sebastian Andrzej Siewior
Clark stumbled over a
VM_BUG_ON()in -RT which was then was removed by Johannes in commitf371763a79d("mm: memcontrol: fix false-positiveVM_BUG_ON()on -rt"). The comment before that patch was a tiny bit better than it is now. While the patch claimed to fix a false-postive on -RT this was not the case. None of the -RT folks ACKed it and it was not a false positive report. That was a *real* problem.The real problem is that
irqs_disabled()on -rt is returning false negatives. Having it return false within aspin_lock_irq()section is broken.
As I explained it in
http://www.spinics.net/lists/linux-rt-users/msg13499.html
it is not.
Quoted 12 lines
Quoted 11 lines
This patch updates the comment that is improper because it refers to "disabled preemption" as a consequence of that lock being taken. A
spin_lock()disables preemption, true, but in this case the code relies on the fact that the lock _also_ disables interrupts once it is acquired. And this is the important detail (which was checked theVM_BUG_ON()) which needs to be pointed out. This is the hint one needs while looking at the code. It was explained by Johannes on the list that the per-CPU variables are protected bylocal_irq_save(). TheBUG_ON()was helpful. This code has been workarounded in -RT in the meantime. I wouldn't mind running into more of those if the code in question uses *special* kind of locking since now there is no no verification (in terms of lockdep orBUG_ON()).
Quoted 2 lines
I'd be happy to re-instate the
VM_BUG_ONthat checks for disabled interrupts as before, that was the most obvious documentation.
sure thing, patch follows in a jiffy or two.
Sebastian
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Johannes Weiner
maintainerQuoted 9 lines · Sebastian Andrzej Siewior
Quoted 8 lines · Johannes Weiner
Quoted 6 lines · Sebastian Andrzej Siewior
Clark stumbled over a
VM_BUG_ON()in -RT which was then was removed by Johannes in commitf371763a79d("mm: memcontrol: fix false-positiveVM_BUG_ON()on -rt"). The comment before that patch was a tiny bit better than it is now. While the patch claimed to fix a false-postive on -RT this was not the case. None of the -RT folks ACKed it and it was not a false positive report. That was a *real* problem.
Quoted 7 lines
Quoted 3 lines
The real problem is that
irqs_disabled()on -rt is returning false negatives. Having it return false within aspin_lock_irq()section is broken.As I explained it in
http://www.spinics.net/lists/linux-rt-users/msg13499.htmlit is not.
I missed this email, sorry about that.
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
This patch updates the comment that is improper because it refers to "disabled preemption" as a consequence of that lock being taken. A
spin_lock()disables preemption, true, but in this case the code relies on the fact that the lock _also_ disables interrupts once it is acquired. And this is the important detail (which was checked theVM_BUG_ON()) which needs to be pointed out. This is the hint one needs while looking at the code. It was explained by Johannes on the list that the per-CPU variables are protected bylocal_irq_save(). TheBUG_ON()was helpful. This code has been workarounded in -RT in the meantime. I wouldn't mind running into more of those if the code in question uses *special* kind of locking since now there is no no verification (in terms of lockdep orBUG_ON()).
Quoted 4 lines
Quoted 2 lines
I'd be happy to re-instate the
VM_BUG_ONthat checks for disabled interrupts as before, that was the most obvious documentation.sure thing, patch follows in a jiffy or two.
Thanks.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH v2 1/1 mm: memcontrol: bring back the VM_BUG_ON() in mem_cgroup_swapout()
Clark stumbled over a VM_BUG_ON() in -RT which was then was removed by Johannes in commit f371763a79d ("mm: memcontrol: fix false-positive VM_BUG_ON() on -rt"). The comment before that patch was a tiny bit better than it is now. While the patch claimed to fix a false-postive on -RT this was not the case. None of the -RT folks ACKed it and it was not a false positive report. That was a *real* problem.
This patch updates the comment that is improper because it refers to "disabled preemption" as a consequence of that lock being taken. A spin_lock() disables preemption, true, but in this case the code relies on the fact that the lock _also_ disables interrupts once it is acquired. And this is the important detail (which was checked the VM_BUG_ON()) which needs to be pointed out. This is the hint one needs while looking at the code. It was explained by Johannes on the list that the per-CPU variables are protected by local_irq_save(). The BUG_ON() was helpful. This code has been workarounded in -RT in the meantime. I wouldn't mind running into more of those if the code in question uses *special* kind of locking since now there is no verification (in terms of lockdep or BUG_ON()) and therefore I bring the VM_BUG_ON() check back in.
The two functions after the comment could also have a "local_irq_save()" dance around them in order to serialize access to the per-CPU variables. This has been avoided because the interrupts should be off.
---
v1…v2: bring back VM_BUG_ON()
mm/memcontrol.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
mm/memcontrol.c +7 -1
@@ -5835,7 +5835,13 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)if (!mem_cgroup_is_root (memcg))page_counter_uncharge (&memcg->memory,1 ); -/* Caller disabled preemption with mapping->tree_lock */ +/* +* Interrupts should be disabled here because the caller holds the +* mapping->tree_lock lock which is taken with interrupts-off. It is +* important here to have the interrupts disabled because it is the +* only synchronisation we have for udpating the per-CPU variables. +*/ +VM_BUG_ON (!irqs_disabled ());mem_cgroup_charge_statistics (memcg, page,-1 );memcg_check_events (memcg, page); }
Signature · 7 lines
2.1.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH v2] mm: memcontrol: bring back the VM_BUG_ON() in mem_cgroup_swapout()
Quoted 20 lines · Sebastian Andrzej Siewior
Clark stumbled over a
VM_BUG_ON()in -RT which was then was removed by Johannes in commitf371763a79d("mm: memcontrol: fix false-positiveVM_BUG_ON()on -rt"). The comment before that patch was a tiny bit better than it is now. While the patch claimed to fix a false-postive on -RT this was not the case. None of the -RT folks ACKed it and it was not a false positive report. That was a *real* problem.This patch updates the comment that is improper because it refers to "disabled preemption" as a consequence of that lock being taken. A
spin_lock()disables preemption, true, but in this case the code relies on the fact that the lock _also_ disables interrupts once it is acquired. And this is the important detail (which was checked theVM_BUG_ON()) which needs to be pointed out. This is the hint one needs while looking at the code. It was explained by Johannes on the list that the per-CPU variables are protected bylocal_irq_save(). TheBUG_ON()was helpful. This code has been workarounded in -RT in the meantime. I wouldn't mind running into more of those if the code in question uses *special* kind of locking since now there is no verification (in terms of lockdep orBUG_ON()) and therefore I bring theVM_BUG_ON()check back in.
Quoted 5 lines
The two functions after the comment could also have a "
local_irq_save()" dance around them in order to serialize access to the per-CPU variables. This has been avoided because the interrupts should be off.Signed-off-by Sebastian Andrzej Siewior <[email protected]>
Much better, thanks.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>