Messages in this thread | | | From | KOSAKI Motohiro <> | Subject | Re: [RFC][PATCH] vmscan: balance local_irq_disable() and local_irq_enable() | Date | Thu, 4 Feb 2010 09:22:54 +0900 (JST) |
| |
> 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 from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |