* [RFC][PATCH] vmscan: Unbalanced local_irq_disable and enable
@ 2010-02-03 19:53 John Kacur
2010-02-03 19:53 ` [RFC][PATCH] vmscan: balance local_irq_disable() and local_irq_enable() John Kacur
0 siblings, 1 reply; 6+ messages in thread
From: John Kacur @ 2010-02-03 19:53 UTC (permalink / raw)
To: lkml
Cc: Thomas Gleixner, Ingo Molnar, Andrew Morton, KOSAKI Motohiro,
Rik van Riel, Johannes Weiner, Minchan Kim, linux-mm, Steven,
"Rostedt <rostedt", John Kacur
I was inspecting this code as I was trying to port some -rt patches to
2.6.33-rcX and it looks quite unusual. It is possible that it is working as
designed and there is nothing wrong with it, so I would like your comments.
Normally a call to local_irq_disable() would be balanced by a call to
local_irq_enable(). Furthermore a call to spin_lock() would be balanced by
a call to spin_unlock() and not to spin_unlock_irq()
However, the call to spin_unlock_irq() will call local_irq_enable()
so that will take care of the unbalanced local_irq_disable.
Still it seems strange.
The patch that I am providing here, is what I think the code SHOULD look like
just based on inspection, it is not at all well testing, I'm just providing
it to illustrate what at least looks wrong with the current code.
Thanks
John Kacur (1):
vmscan: balance local_irq_disable() and local_irq_enable()
mm/vmscan.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC][PATCH] vmscan: balance local_irq_disable() and local_irq_enable()
2010-02-03 19:53 [RFC][PATCH] vmscan: Unbalanced local_irq_disable and enable John Kacur
@ 2010-02-03 19:53 ` John Kacur
2010-02-03 20:09 ` Steven Rostedt
0 siblings, 1 reply; 6+ messages in thread
From: John Kacur @ 2010-02-03 19:53 UTC (permalink / raw)
To: lkml
Cc: Thomas Gleixner, Ingo Molnar, Andrew Morton, KOSAKI Motohiro,
Rik van Riel, Johannes Weiner, Minchan Kim, linux-mm, Steven,
"Rostedt <rostedt", John Kacur
Balance local_irq_disable() and local_irq_enable() as well as
spin_lock_irq() and spin_lock_unlock_irq
Signed-off-by: John Kacur <jkacur@redhat.com>
---
mm/vmscan.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index c26986c..b895025 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1200,8 +1200,9 @@ static unsigned long shrink_inactive_list(unsigned long max_scan,
if (current_is_kswapd())
__count_vm_events(KSWAPD_STEAL, nr_freed);
__count_zone_vm_events(PGSTEAL, zone, nr_freed);
+ local_irq_enable();
- spin_lock(&zone->lru_lock);
+ spin_lock_irq(&zone->lru_lock);
/*
* Put back any unfreeable pages.
*/
--
1.6.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH] vmscan: balance local_irq_disable() and local_irq_enable()
2010-02-03 19:53 ` [RFC][PATCH] vmscan: balance local_irq_disable() and local_irq_enable() John Kacur
@ 2010-02-03 20:09 ` Steven Rostedt
2010-02-03 20:12 ` John Kacur
0 siblings, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2010-02-03 20:09 UTC (permalink / raw)
To: John Kacur
Cc: lkml, Thomas Gleixner, Ingo Molnar, Andrew Morton,
KOSAKI Motohiro, Rik van Riel, Johannes Weiner, Minchan Kim,
linux-mm
t On Wed, 2010-02-03 at 20:53 +0100, John Kacur wrote:
> Balance local_irq_disable() and local_irq_enable() as well as
> spin_lock_irq() and spin_lock_unlock_irq
>
> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---
> mm/vmscan.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index c26986c..b895025 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1200,8 +1200,9 @@ static unsigned long shrink_inactive_list(unsigned long max_scan,
> if (current_is_kswapd())
> __count_vm_events(KSWAPD_STEAL, nr_freed);
> __count_zone_vm_events(PGSTEAL, zone, nr_freed);
> + local_irq_enable();
>
> - spin_lock(&zone->lru_lock);
> + spin_lock_irq(&zone->lru_lock);
> /*
> * Put back any unfreeable pages.
> */
The above looks wrong. I don't know the code, but just by looking at
where the locking and interrupts are, I can take a guess.
Lets add a little more of the code:
local_irq_disable();
if (current_is_kswapd())
__count_vm_events(KSWAPD_STEAL, nr_freed);
__count_zone_vm_events(PGSTEAL, zone, nr_freed);
spin_lock(&zone->lru_lock);
/*
I'm guessing the __count_zone_vm_events and friends need interrupts
disabled here, probably due to per cpu stuff. But if you enable
interrupts before the spin_lock() you may let an interrupt come in and
invalidate what was done above it.
So no, I do not think enabling interrupts here is a good thing.
-- Steve
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH] vmscan: balance local_irq_disable() and local_irq_enable()
2010-02-03 20:09 ` Steven Rostedt
@ 2010-02-03 20:12 ` John Kacur
2010-02-04 0:22 ` KOSAKI Motohiro
0 siblings, 1 reply; 6+ messages in thread
From: John Kacur @ 2010-02-03 20:12 UTC (permalink / raw)
To: rostedt
Cc: lkml, Thomas Gleixner, Ingo Molnar, Andrew Morton,
KOSAKI Motohiro, Rik van Riel, Johannes Weiner, Minchan Kim,
linux-mm
On Wed, Feb 3, 2010 at 9:09 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> t On Wed, 2010-02-03 at 20:53 +0100, John Kacur wrote:
>> Balance local_irq_disable() and local_irq_enable() as well as
>> spin_lock_irq() and spin_lock_unlock_irq
>>
>> Signed-off-by: John Kacur <jkacur@redhat.com>
>> ---
>> mm/vmscan.c | 3 ++-
>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index c26986c..b895025 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1200,8 +1200,9 @@ static unsigned long shrink_inactive_list(unsigned long max_scan,
>> if (current_is_kswapd())
>> __count_vm_events(KSWAPD_STEAL, nr_freed);
>> __count_zone_vm_events(PGSTEAL, zone, nr_freed);
>> + local_irq_enable();
>>
>> - spin_lock(&zone->lru_lock);
>> + spin_lock_irq(&zone->lru_lock);
>> /*
>> * Put back any unfreeable pages.
>> */
>
>
> The above looks wrong. I don't know the code, but just by looking at
> where the locking and interrupts are, I can take a guess.
>
> Lets add a little more of the code:
>
> local_irq_disable();
> if (current_is_kswapd())
> __count_vm_events(KSWAPD_STEAL, nr_freed);
> __count_zone_vm_events(PGSTEAL, zone, nr_freed);
>
> spin_lock(&zone->lru_lock);
> /*
>
> I'm guessing the __count_zone_vm_events and friends need interrupts
> disabled here, probably due to per cpu stuff. But if you enable
> interrupts before the spin_lock() you may let an interrupt come in and
> invalidate what was done above it.
>
> So no, I do not think enabling interrupts here is a good thing.
>
okay, and since we have already done local_irq_disable(), then that is
why we only need the spin_lock() and not the spin_lock_irq() flavour?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH] vmscan: balance local_irq_disable() and local_irq_enable()
2010-02-03 20:12 ` John Kacur
@ 2010-02-04 0:22 ` KOSAKI Motohiro
2010-02-05 16:05 ` John Kacur
0 siblings, 1 reply; 6+ messages in thread
From: KOSAKI Motohiro @ 2010-02-04 0:22 UTC (permalink / raw)
To: John Kacur
Cc: kosaki.motohiro, rostedt, lkml, Thomas Gleixner, Ingo Molnar,
Andrew Morton, Rik van Riel, Johannes Weiner, Minchan Kim,
linux-mm
> On Wed, Feb 3, 2010 at 9:09 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > t On Wed, 2010-02-03 at 20:53 +0100, John Kacur wrote:
> >> Balance local_irq_disable() and local_irq_enable() as well as
> >> spin_lock_irq() and spin_lock_unlock_irq
> >>
> >> Signed-off-by: John Kacur <jkacur@redhat.com>
> >> ---
> >> mm/vmscan.c | 3 ++-
> >> 1 files changed, 2 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/mm/vmscan.c b/mm/vmscan.c
> >> index c26986c..b895025 100644
> >> --- a/mm/vmscan.c
> >> +++ b/mm/vmscan.c
> >> @@ -1200,8 +1200,9 @@ static unsigned long shrink_inactive_list(unsigned long max_scan,
> >> if (current_is_kswapd())
> >> __count_vm_events(KSWAPD_STEAL, nr_freed);
> >> __count_zone_vm_events(PGSTEAL, zone, nr_freed);
> >> + local_irq_enable();
> >>
> >> - spin_lock(&zone->lru_lock);
> >> + spin_lock_irq(&zone->lru_lock);
> >> /*
> >> * Put back any unfreeable pages.
> >> */
> >
> >
> > The above looks wrong. I don't know the code, but just by looking at
> > where the locking and interrupts are, I can take a guess.
> >
> > Lets add a little more of the code:
> >
> > local_irq_disable();
> > if (current_is_kswapd())
> > __count_vm_events(KSWAPD_STEAL, nr_freed);
> > __count_zone_vm_events(PGSTEAL, zone, nr_freed);
> >
> > spin_lock(&zone->lru_lock);
> > /*
> >
> > I'm guessing the __count_zone_vm_events and friends need interrupts
> > disabled here, probably due to per cpu stuff. But if you enable
> > interrupts before the spin_lock() you may let an interrupt come in and
> > invalidate what was done above it.
> >
> > So no, I do not think enabling interrupts here is a good thing.
> >
>
> okay, and since we have already done local_irq_disable(), then that is
> why we only need the spin_lock() and not the spin_lock_irq() flavour?
Yes, spin_lock_irq() is equivalent to spin_lock() + irq_disable().
Now, we already disabled irq. then, we only need spin_lock().
So, I don't think shrink_inactive_list need any fix.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH] vmscan: balance local_irq_disable() and local_irq_enable()
2010-02-04 0:22 ` KOSAKI Motohiro
@ 2010-02-05 16:05 ` John Kacur
0 siblings, 0 replies; 6+ messages in thread
From: John Kacur @ 2010-02-05 16:05 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: rostedt, lkml, Thomas Gleixner, Ingo Molnar, Andrew Morton,
Rik van Riel, Johannes Weiner, Minchan Kim, linux-mm
On Thu, Feb 4, 2010 at 1:22 AM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> On Wed, Feb 3, 2010 at 9:09 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> > t On Wed, 2010-02-03 at 20:53 +0100, John Kacur wrote:
>> >> Balance local_irq_disable() and local_irq_enable() as well as
>> >> spin_lock_irq() and spin_lock_unlock_irq
>> >>
>> >> Signed-off-by: John Kacur <jkacur@redhat.com>
>> >> ---
>> >> mm/vmscan.c | 3 ++-
>> >> 1 files changed, 2 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> >> index c26986c..b895025 100644
>> >> --- a/mm/vmscan.c
>> >> +++ b/mm/vmscan.c
>> >> @@ -1200,8 +1200,9 @@ static unsigned long shrink_inactive_list(unsigned long max_scan,
>> >> if (current_is_kswapd())
>> >> __count_vm_events(KSWAPD_STEAL, nr_freed);
>> >> __count_zone_vm_events(PGSTEAL, zone, nr_freed);
>> >> + local_irq_enable();
>> >>
>> >> - spin_lock(&zone->lru_lock);
>> >> + spin_lock_irq(&zone->lru_lock);
>> >> /*
>> >> * Put back any unfreeable pages.
>> >> */
>> >
>> >
>> > The above looks wrong. I don't know the code, but just by looking at
>> > where the locking and interrupts are, I can take a guess.
>> >
>> > Lets add a little more of the code:
>> >
>> > local_irq_disable();
>> > if (current_is_kswapd())
>> > __count_vm_events(KSWAPD_STEAL, nr_freed);
>> > __count_zone_vm_events(PGSTEAL, zone, nr_freed);
>> >
>> > spin_lock(&zone->lru_lock);
>> > /*
>> >
>> > I'm guessing the __count_zone_vm_events and friends need interrupts
>> > disabled here, probably due to per cpu stuff. But if you enable
>> > interrupts before the spin_lock() you may let an interrupt come in and
>> > invalidate what was done above it.
>> >
>> > So no, I do not think enabling interrupts here is a good thing.
>> >
>>
>> okay, and since we have already done local_irq_disable(), then that is
>> why we only need the spin_lock() and not the spin_lock_irq() flavour?
>
> Yes, spin_lock_irq() is equivalent to spin_lock() + irq_disable().
> Now, we already disabled irq. then, we only need spin_lock().
>
> So, I don't think shrink_inactive_list need any fix.
>
Thanks for the explanation!
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-02-05 16:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-03 19:53 [RFC][PATCH] vmscan: Unbalanced local_irq_disable and enable John Kacur
2010-02-03 19:53 ` [RFC][PATCH] vmscan: balance local_irq_disable() and local_irq_enable() John Kacur
2010-02-03 20:09 ` Steven Rostedt
2010-02-03 20:12 ` John Kacur
2010-02-04 0:22 ` KOSAKI Motohiro
2010-02-05 16:05 ` John Kacur
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).