* [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).