23 second kernel compile / pagemap_lru_lock improvement

Your suggestion seems to improve the pagemap_lru_lock contention a little:
(make bzImage , 16 way NUMA-Q, 5 runs each)

before:

lockstat.1: 20.6% 59.2% 5.2us( 81us) 104us( 11ms)(15.2%) 994575 40.8% 59.2% 0% pagemap_lru_lock
lockstat.2: 19.0% 54.8% 4.8us( 84us) 102us( 10ms)(13.7%) 1007488 45.2% 54.8% 0% pagemap_lru_lock
lockstat.3: 19.5% 55.6% 4.9us( 78us) 105us( 11ms)(14.4%) 1000062 44.4% 55.6% 0% pagemap_lru_lock
lockstat.4: 17.8% 51.6% 5.2us( 105us) 90us( 12ms)(10.0%) 1008310 48.4% 51.6% 0% pagemap_lru_lock
lockstat.5: 20.2% 57.1% 5.4us( 86us) 111us( 16ms)(14.7%) 1014988 42.9% 57.1% 0% pagemap_lru_lock

after:

lockstat.1: 18.0% 54.3% 4.7us( 155us) 92us( 14ms)(11.9%) 999140 45.7% 54.3% 0% pagemap_lru_lock
lockstat.2: 17.7% 52.8% 4.6us( 151us) 94us( 14ms)(12.0%) 997999 47.2% 52.8% 0% pagemap_lru_lock
lockstat.3: 18.3% 54.4% 5.2us(1581us) 111us( 10ms)(13.2%) 1005149 45.6% 54.4% 0% pagemap_lru_lock
lockstat.4: 20.2% 57.8% 5.3us( 210us) 108us( 12ms)(14.9%) 998813 42.2% 57.8% 0% pagemap_lru_lock
lockstat.5: 14.4% 44.2% 3.8us( 167us) 81us( 11ms)( 8.5%) 1004476 55.8% 44.2% 0% pagemap_lru_lock

It's a little difficult to see, as there's quite some variablity between runs.

If we look at activate_page_nolock, it's easier to understand the context of the patch:

------------------

static inline void activate_page_nolock(struct page * page)

{
        if (PageLRU(page) && !PageActive(page)) {
                del_page_from_inactive_list(page);
                add_page_to_active_list(page);
        }
}

void activate_page(struct page * page)

{
        spin_lock(&pagemap_lru_lock);
        activate_page_nolock(page);
        spin_unlock(&pagemap_lru_lock);
}

---------------------

Patch is below - any comments?

@@ -46,9 +46,15 @@
 
 void activate_page(struct page * page)
 {
-	spin_lock(&pagemap_lru_lock);
-	activate_page_nolock(page);
-	spin_unlock(&pagemap_lru_lock);
+	if (PageLRU(page) && !PageActive(page)) {
+		/* 
+		 * no point in grabbing the lock if we don't have to do
+		 * anything with it, but check twice in case of a race 
+		 */
+		spin_lock(&pagemap_lru_lock);
+		activate_page_nolock(page);
+		spin_unlock(&pagemap_lru_lock);
+	}
 }
 
 /**

I'm surprised it made any difference at all, I think the patch mainly adds more tests: activate_page is only called from mark_page_accessed (after testing !PageActive) and from fail_writepage (where usually !PageActive). I don't think many !PageLRU pages can get there. Or is activate_page being called from other places in your tree?

Hugh

There's quite a bit of variablility between runs, so it's a little hard to pin down. The number of locks taken doesn't seem to be affected much, and that's probably the best indicator of whether this is really working.

No, I don't think I'm doing other stranger things to activate_page.

It does seem distinctly odd that we take the lock, *then* test whether we actually need to do anything. Is the test just a sanity check that should never fail?

M.

It's quite normal to have to recheck flags after taking the relevant lock. Here I think the two flags have different needs. I've not checked rigorously, but I believe that the PageLRU flag cannot change beneath us (but does need to be checked either outside or inside the lock); whereas it's easy to find races where PageActive is set outside but found clear once inside the lock, or vice versa.

Now it doesn't matter if we make a wrong activity decision occasionally, but we do need to keep internal consistency. If PageActive were not rechecked inside pagemap_lru_lock, nr_active_pages and nr_inactive_pages would become approximate instead of exact counts; then there's a danger they would tend to drift in one direction, unbalancing shrink_caches.

Hugh