vmscan: balance local_irq_disable() and local_irq_enable()
in [Kernel]
|
Prev: [PATCH 1/2] syslog: distinguish between /proc/kmsg and syscalls
Next: vmscan: balance local_irq_disable() and local_irq_enable()
From: Steven Rostedt on 3 Feb 2010 15:10 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. -- Steve -- 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: KOSAKI Motohiro on 3 Feb 2010 19:30
> 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. -- 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/ |