remove mixture of non-atomic operations with page->flags which requires atomic operations to access

page->flags is effectively a lock word as its various bits are updatable and accessible only by atomic operations. This patch removes the update of page->flags in __free_pages_ok() with non-atomic operations in favor of using atomic bit operations to update the bits to be cleared.

Against 2.5.19.

Cheers,
Bill

diff -Nru a/include/linux/page-flags.h b/include/linux/page-flags.h
@@ -169,6 +169,7 @@
 
 #define PageChecked(page)	test_bit(PG_checked, &(page)->flags)
 #define SetPageChecked(page)	set_bit(PG_checked, &(page)->flags)
+#define ClearPageChecked(page)	clear_bit(PG_checked, &(page)->flags)
 
 #define PageReserved(page)	test_bit(PG_reserved, &(page)->flags)
 #define SetPageReserved(page)	set_bit(PG_reserved, &(page)->flags)

diff -Nru a/mm/page_alloc.c b/mm/page_alloc.c
@@ -91,8 +91,12 @@
 	BUG_ON(PageLRU(page));
 	BUG_ON(PageActive(page));
 	BUG_ON(PageWriteback(page));
+
 	ClearPageDirty(page);
-	page->flags &= ~(1<<PG_referenced);
+	ClearPageUptodate(page);
+	ClearPageSlab(page);
+	ClearPageNosave(page);
+	ClearPageChecked(page);
 
 	if (current->flags & PF_FREE_PAGES)
 		goto local_freelist;

From: Hugh Dickins <[email protected]>
Date: Mon, 3 Jun 2002 11:14:43 +0100 (BST)

   On Sun, 2 Jun 2002, William Lee Irwin III wrote:
   > 
   >  	ClearPageDirty(page);
   > -	page->flags &= ~(1<<PG_referenced);
   > +	ClearPageUptodate(page);
   > +	ClearPageSlab(page);
   > +	ClearPageNosave(page);
   > +	ClearPageChecked(page);

Don't all those atomic volatile bitops slow down a hotpath for no real gain? I'm all for clearing all possible flag bits at that point, but would prefer just one mask myself. But given all the preceding tests, and the ClearPageDirty, perhaps I'm foolish to question your additions.

And wasn't it originally clearing the referenced bit, now leaving it?

Furthermore, when this code runs there should be no way for any part of the kernel to reference the page in a way that cares about these flag bits.

So if anything, we should be doing _ALL_ of the flag bitsops non-atomically.

It should be clearing it, I'd retransmit if there weren't other objections to address...

Cheers,
Bill

From: William Lee Irwin III <[email protected]>
Date: Mon, 3 Jun 2002 03:28:09 -0700

It should be clearing it, I'd retransmit if there weren't other objections to address...

Such as the fact that none of these operations need to be atomic :-)

Ugh, even if it isn't I don't care to deal with it, rusty, chuck this one.

Cheers,
Bill

From: William Lee Irwin III <[email protected]>
Date: Mon, 3 Jun 2002 03:28:09 -0700

After looking around a little bit the unique reference guarantee plus various memory barriers surrounding it appear to suffice. But I'm still left quite uneasy by the usage of non-atomic operations on an atomically updated lock word. At the very least making the operand shifted an unsigned long is needed. So this ensues:

Cheers,
Bill

===== mm/page_alloc.c 1.63 vs edited =====

@@ -110,8 +110,8 @@
 		BUG();
 	if (PageWriteback(page))
 		BUG();
-	ClearPageDirty(page);
-	page->flags &= ~(1<<PG_referenced);
+
+	page->flags &= ~((1UL << PG_referenced) | (1UL << PG_dirty));
 
 	if (current->flags & PF_FREE_PAGES)
 		goto local_freelist;

From: William Lee Irwin III <[email protected]>
Date: Mon, 3 Jun 2002 04:00:55 -0700

    	if (PageWriteback(page))
    		BUG();

- ClearPageDirty(page);
- page->flags &= ~(1<<PG_referenced);
+
+ page->flags &= ~((1UL << PG_referenced) | (1UL << PG_dirty));

Umm, nevermind. Look at ClearPageDirty, it does
"other stuff" so you can't remove it wholesale.

In the end, the code is as it should be right now.