vmscan: balance local_irq_disable() and local_irq_enable()
in [Kernel]
|
Prev: vmscan: balance local_irq_disable() and local_irq_enable()
Next: patch net-restore-ip-source-validation.patch added to 2.6.32-stable tree
From: John Kacur on 3 Feb 2010 15:20 On Wed, Feb 3, 2010 at 9:09 PM, Steven Rostedt <rostedt(a)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(a)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 from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: John Kacur on 5 Feb 2010 11:10
On Thu, Feb 4, 2010 at 1:22 AM, KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com> wrote: >> On Wed, Feb 3, 2010 at 9:09 PM, Steven Rostedt <rostedt(a)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(a)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 from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |