s390's PageSwapCache test

s390's PageSwapCache test

[Hugh Dickins]

Hi Martin,

I'm slightly bothered by that PageSwapCache() test you've just added
in page_remove_rmap(), before s390's page_test_dirty():

		if ((!PageAnon(page) || PageSwapCache(page)) &&
		    page_test_dirty(page)) {
			page_clear_dirty(page);
			set_page_dirty(page);
		}

It's not wrong; but if it's necessary, then I need to understand why;
and if it's unnecessary, then we'd do better to remove it (optimizing
your optimization a little).

I believe it's unnecessary: it is possible, yes, to arrive here and
find the anon page dirty with respect to what's on swap disk; but
because anon pages are COWed, never sharing modification with other
users, that will only be so if we're the only user of that page, and
about to free it, in which case no point in doing the set_page_dirty().

For a very similar case, see the PageAnon() test in zap_pte_range(),
where we also skip the set_page_dirty().

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: s390's PageSwapCache test

[Martin Schwidefsky]

Hi Hugh,

On Sat, 2008-08-02 at 10:05 +0100, Hugh Dickins wrote:
> I'm slightly bothered by that PageSwapCache() test you've just added
> in page_remove_rmap(), before s390's page_test_dirty():
> 
> 		if ((!PageAnon(page) || PageSwapCache(page)) &&
> 		    page_test_dirty(page)) {
> 			page_clear_dirty(page);
> 			set_page_dirty(page);
> 		}
> 
> It's not wrong; but if it's necessary, then I need to understand why;
> and if it's unnecessary, then we'd do better to remove it (optimizing
> your optimization a little).

I want to play safe. I can conclude that the page dirty bit is of no
interest if the page is a purely anonymous page without a backing. That
is what the test checks for.

> I believe it's unnecessary: it is possible, yes, to arrive here and
> find the anon page dirty with respect to what's on swap disk; but
> because anon pages are COWed, never sharing modification with other
> users, that will only be so if we're the only user of that page, and
> about to free it, in which case no point in doing the set_page_dirty().

Hmm, what about the following sequence:
1) a page is added to the swap
2) the page is dirtied again
3) the process forks
4) the first process does an mlock
5) vmscan succeeds in replacing the pte of the second process with a
swap entry but fails for the pte of the first process.
6) the first process exists.

If the PageSwapCache() check is missing zap_pte_range() will drop the
last pte for the page but won't transfer the dirty bit.
Wouldn't that break?

> For a very similar case, see the PageAnon() test in zap_pte_range(),
> where we also skip the set_page_dirty().

-- 
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: s390's PageSwapCache test

[Hugh Dickins]

On Sat, 2 Aug 2008, Martin Schwidefsky wrote:
> On Sat, 2008-08-02 at 10:05 +0100, Hugh Dickins wrote:
> > I'm slightly bothered by that PageSwapCache() test you've just added
> > in page_remove_rmap(), before s390's page_test_dirty():
> > 
> > 		if ((!PageAnon(page) || PageSwapCache(page)) &&
> > 		    page_test_dirty(page)) {
> > 			page_clear_dirty(page);
> > 			set_page_dirty(page);
> > 		}
> > 
> > It's not wrong; but if it's necessary, then I need to understand why;
> > and if it's unnecessary, then we'd do better to remove it (optimizing
> > your optimization a little).
> ... 
> Hmm, what about the following sequence:
> 1) a page is added to the swap
> 2) the page is dirtied again
> 3) the process forks
> 4) the first process does an mlock
> 5) vmscan succeeds in replacing the pte of the second process with a
> swap entry but fails for the pte of the first process.
> 6) the first process exists.
                       exits
> 
> If the PageSwapCache() check is missing zap_pte_range() will drop the
> last pte for the page but won't transfer the dirty bit.
> Wouldn't that break?

Yes, it took me a while to understand, but you are right.  I was
blinkered, thinking always of page_remove_rmap called by zap_pte_range,
forgetting page_remove_rmap called by try_to_unmap_one i.e. vmscan.

Your example is dealt with on the non-s390 arches by try_to_unmap_one's
	if (pte_dirty(pteval))
		set_page_dirty(page);
but s390 needs something else: and since you can't do it until the
last mapping is removed, you're stuck with detecting the possibility
of this case by testing PageSwapCache in there.

It's a pity that so often it's irrelevant, but I can't offhand think
of a better answer, and it would only be a small optimization to an
already non-optimal path.

Good thinking, Martin, and thank you for enlightening me:
when I'm next in patch mode I'll add a comment there on it.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>