setup_per_zone_pages_min(): zone->lock vs. zone->lru_lock

Hi,

is zone->lru_lock really the right lock to take in setup_per_zone_pages_min()? All other functions in mm/page_alloc.c take zone->lock instead, for working with page->lru free-list or PageBuddy().

setup_per_zone_pages_min() eventually calls move_freepages(), which is also manipulating the page->lru free-list and checking for PageBuddy(). Both should be protected by zone->lock instead of zone->lru_lock, if I understood that right, or else there could be a race with the other functions in mm/page_alloc.c.

We ran into a list corruption bug in free_pages_bulk() once, during memory hotplug stress test, but cannot reproduce it easily. So I cannot verify if using zone->lock instead of zone->lru_lock would fix it, but to me it looks like this may be the problem.

Any thoughts?

BTW, I also wonder if a spin_lock_irq() would be enough, instead of spin_lock_irqsave(), because this function should never be called from interrupt context, right?

The allocator protects it freelists using zone->lock (as we can see in rmqueue_bulk), so anything which manipulates those should also be using that lock. move_freepages() is scanning the cmap and picking up free pages directly off the free lists, it is expecting those lists to be stable; it would appear to need zone->lock. It does look like setup_per_zone_pages_min() is holding the wrong thing at first look.

Quoted 6 lines · Andy Whitcroft

The allocator protects it freelists using zone->lock (as we can see in rmqueue_bulk), so anything which manipulates those should also be using that lock. move_freepages() is scanning the cmap and picking up free pages directly off the free lists, it is expecting those lists to be stable; it would appear to need zone->lock. It does look like setup_per_zone_pages_min() is holding the wrong thing at first look.

I just noticed that the spin_lock in that function is much older than the call to setup_zone_migrate_reserve(), which then calls move_freepages(). So it seems that the zone->lru_lock there does (did?) have another purpose, maybe protecting zone->present_pages/pages_min/etc.

Looks like the need for a zone->lock (if any) was added later, but I'm not sure if makes sense to take both locks together, or if the lru_lock is still needed at all.

Quoted 8 lines · On Mon, 29 Sep 2008 23:20:05 +0200 Gerald Schaefer…

Quoted 6 lines · Andy Whitcroft

The allocator protects it freelists using zone->lock (as we can see in rmqueue_bulk), so anything which manipulates those should also be using that lock. move_freepages() is scanning the cmap and picking up free pages directly off the free lists, it is expecting those lists to be stable; it would appear to need zone->lock. It does look like setup_per_zone_pages_min() is holding the wrong thing at first look.

Quoted 5 lines

I just noticed that the spin_lock in that function is much older than the call to setup_zone_migrate_reserve(), which then calls move_freepages(). So it seems that the zone->lru_lock there does (did?) have another purpose, maybe protecting zone->present_pages/pages_min/etc.

Maybe.

Quoted 5 lines

Looks like the need for a zone->lock (if any) was added later, but I'm not sure if makes sense to take both locks together, or if the lru_lock is still needed at all.

At first look, replacing zone->lru_lock with zone->lock is enough... This function is an only one function which use zone->lru_lock in page_alloc.c And zone_watermark_ok() which access zone->pages_min/low/high is not under any locks. So, taking zone->lru_lock here doesn't seem to be necessary...

From: Gerald Schaefer <[email protected]>

This replaces zone->lru_lock in setup_per_zone_pages_min() with zone->lock. There seems to be no need for the lru_lock anymore, but there is a need for zone->lock instead, because that function may call move_freepages() via setup_zone_migrate_reserve().

Signed-off-by Gerald Schaefer <[email protected]>

---
 mm/page_alloc.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/page_alloc.c
===================================================================

mm/page_alloc.c +2 -3

@@ -4207,7 +4207,7 @@ void setup_per_zone_pages_min(void)
 	for_each_zone(zone) {
 		u64 tmp;
 
-		spin_lock_irqsave(&zone->lru_lock, flags);
+		spin_lock_irqsave(&zone->lock, flags);
 		tmp = (u64)pages_min * zone->present_pages;
 		do_div(tmp, lowmem_pages);
 		if (is_highmem(zone)) {

@@ -4239,7 +4239,7 @@ void setup_per_zone_pages_min(void)
 		zone->pages_low   = zone->pages_min + (tmp >> 2);
 		zone->pages_high  = zone->pages_min + (tmp >> 1);
 		setup_zone_migrate_reserve(zone);
-		spin_unlock_irqrestore(&zone->lru_lock, flags);
+		spin_unlock_irqrestore(&zone->lock, flags);
 	}
 
 	/* update totalreserve_pages */
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Gerald Schaefer <[email protected]>

This replaces zone->lru_lock in setup_per_zone_pages_min() with zone->lock. There seems to be no need for the lru_lock anymore, but there is a need for zone->lock instead, because that function may call move_freepages() via setup_zone_migrate_reserve().

Signed-off-by Gerald Schaefer <[email protected]>

---
 mm/page_alloc.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/page_alloc.c
===================================================================

mm/page_alloc.c +2 -2

@@ -4207,7 +4207,7 @@ void setup_per_zone_pages_min(void)
 	for_each_zone(zone) {
 		u64 tmp;
 
-		spin_lock_irqsave(&zone->lru_lock, flags);
+		spin_lock_irqsave(&zone->lock, flags);
 		tmp = (u64)pages_min * zone->present_pages;
 		do_div(tmp, lowmem_pages);
 		if (is_highmem(zone)) {

@@ -4239,7 +4239,7 @@ void setup_per_zone_pages_min(void)
 		zone->pages_low   = zone->pages_min + (tmp >> 2);
 		zone->pages_high  = zone->pages_min + (tmp >> 1);
 		setup_zone_migrate_reserve(zone);
-		spin_unlock_irqrestore(&zone->lru_lock, flags);
+		spin_unlock_irqrestore(&zone->lock, flags);
 	}
 
 	/* update totalreserve_pages */