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

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

[William Lee Irwin III]

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
--- a/include/linux/page-flags.h	Sun Jun  2 15:42:49 2002
+++ b/include/linux/page-flags.h	Sun Jun  2 15:42:49 2002
@@ -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
--- a/mm/page_alloc.c	Sun Jun  2 15:42:49 2002
+++ b/mm/page_alloc.c	Sun Jun  2 15:42:49 2002
@@ -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;

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

[Hugh Dickins]

On Sun, 2 Jun 2002, William Lee Irwin III wrote:
> 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.
> 
>  	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?

Hugh

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

[David S. Miller]

   From: Hugh Dickins <hugh@veritas.com>
   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.

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

[William Lee Irwin III]

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

On Mon, Jun 03, 2002 at 11:14:43AM +0100, Hugh Dickins wrote:
> 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?
> Hugh

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


Cheers,
Bill

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

[David S. Miller]

   From: William Lee Irwin III <wli@holomorphy.com>
   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 :-)

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

[William Lee Irwin III]

From: William Lee Irwin III <wli@holomorphy.com>
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...

On Mon, Jun 03, 2002 at 02:27:39AM -0700, David S. Miller wrote:
> Such as the fact that none of these operations need to
> be atomic :-)

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 =====
--- 1.63/mm/page_alloc.c	Tue May 28 16:57:49 2002
+++ edited/mm/page_alloc.c	Mon Jun  3 03:58:59 2002
@@ -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;

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

[David S. Miller]

   From: William Lee Irwin III <wli@holomorphy.com>
   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.

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

[William Lee Irwin III]

>    From: William Lee Irwin III <wli@holomorphy.com>
>    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.

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


Cheers,
Bill