[RFC] Making compound pages mandatory

[RFC] Making compound pages mandatory

[David Howells]

Hi Linus,

Do you have any objection to compound pages being made mandatory, even without
HUGETLB support?

David

Re: [RFC] Making compound pages mandatory

[Andrew Morton]

David Howells <dhowells@redhat.com> wrote:
>
> 
> Hi Linus,
> 
> Do you have any objection to compound pages being made mandatory, even without
> HUGETLB support?
> 

Andrea wants to do that, purely from a code coverage point of view.  But it
does add a little extra overhead.

For what reason do you propose this?

Re: [RFC] Making compound pages mandatory

[David Howells]

> Andrea wants to do that, purely from a code coverage point of view.  But it
> does add a little extra overhead.

It definitely does.

> For what reason do you propose this?

You and Christoph, amongst others, seemed dead in favour of it. Other than
that, I have no real need for it, except that it makes kobjsize() simpler.

David

Re: [RFC] Making compound pages mandatory

[Linus Torvalds]

On Tue, 16 Nov 2004, David Howells wrote:
> 
> Do you have any objection to compound pages being made mandatory, even without
> HUGETLB support?

I haven't really looked into it, so I cannot make an informed decision.  
How big is the overhead? And what's the _point_, since we don't seem to 
need them normally, but the code is there for people who _do_ need them? 

I don't generally like two paths, but quite frankly, I consider the
non-compound case the regular case right now. How expensive does the
compound pages end up being? Is it just one more pointer chase on every
get_page/put_page, or what? Does anybody have numbers for before/after 
that are otherwise comparable?

		Linus

Re: [RFC] Making compound pages mandatory

[Andrew Morton]

Linus Torvalds <torvalds@osdl.org> wrote:
>
> 
> 
> On Tue, 16 Nov 2004, David Howells wrote:
> > 
> > Do you have any objection to compound pages being made mandatory, even without
> > HUGETLB support?
> 
> I haven't really looked into it, so I cannot make an informed decision.  
> How big is the overhead? And what's the _point_, since we don't seem to 
> need them normally, but the code is there for people who _do_ need them? 

Yes, it's just the single pointer chase.  Probably that's the common case
now, because everyone will be enabling hugepages on lots of architectures.

But still, the non-compound code is well tested too, and leaving it in
place does make a little microoptimisation available to those who want it,
so I don't see a reason yet to make compound pages compulsory.

So I'd suggest that we make compound pages conditional on a new
CONFIG_COMPOUND_PAGE and make that equal to HUGETLB_PAGE || !MMU.

Re: [RFC] Making compound pages mandatory

[Nick Piggin]

[-- Attachment #1: Type: text/plain, Size: 1211 bytes --]

Andrew Morton wrote:
> Linus Torvalds <torvalds@osdl.org> wrote:
> 
>>
>>
>>On Tue, 16 Nov 2004, David Howells wrote:
>>
>>>Do you have any objection to compound pages being made mandatory, even without
>>>HUGETLB support?
>>
>>I haven't really looked into it, so I cannot make an informed decision.  
>>How big is the overhead? And what's the _point_, since we don't seem to 
>>need them normally, but the code is there for people who _do_ need them? 
> 
> 
> Yes, it's just the single pointer chase.  Probably that's the common case
> now, because everyone will be enabling hugepages on lots of architectures.
> 
> But still, the non-compound code is well tested too, and leaving it in
> place does make a little microoptimisation available to those who want it,
> so I don't see a reason yet to make compound pages compulsory.
> 
> So I'd suggest that we make compound pages conditional on a new
> CONFIG_COMPOUND_PAGE and make that equal to HUGETLB_PAGE || !MMU.

Good idea. BTW, any reason why the following (very)micro optimisation
shouldn't go in?

It currently only picks up a couple of things under fs/, but might help
reduce other ifdefery around the place. For example mm.h: page_count and
get_page.

[-- Attachment #2: mm-page_compound-microopt.patch --]
[-- Type: text/x-patch, Size: 880 bytes --]




---

 linux-2.6-npiggin/include/linux/page-flags.h |    4 ++++
 1 files changed, 4 insertions(+)

diff -puN include/linux/page-flags.h~mm-page_compound-microopt include/linux/page-flags.h
--- linux-2.6/include/linux/page-flags.h~mm-page_compound-microopt	2004-11-17 14:02:44.000000000 +1100
+++ linux-2.6-npiggin/include/linux/page-flags.h	2004-11-17 14:09:07.000000000 +1100
@@ -286,7 +286,11 @@ extern unsigned long __read_page_state(u
 #define ClearPageReclaim(page)	clear_bit(PG_reclaim, &(page)->flags)
 #define TestClearPageReclaim(page) test_and_clear_bit(PG_reclaim, &(page)->flags)
 
+#ifdef CONFIG_HUGETLB_PAGE
 #define PageCompound(page)	test_bit(PG_compound, &(page)->flags)
+#else
+#define PageCompound(page)	0
+#endif
 #define SetPageCompound(page)	set_bit(PG_compound, &(page)->flags)
 #define ClearPageCompound(page)	clear_bit(PG_compound, &(page)->flags)
 

_

Re: [RFC] Making compound pages mandatory

[Nick Piggin]

Nick Piggin wrote:

> 
> Good idea. BTW, any reason why the following (very)micro optimisation
> shouldn't go in?
> 
> It currently only picks up a couple of things under fs/, but might help
> reduce other ifdefery around the place. For example mm.h: page_count and
> get_page.
> 

Like this, perhaps? It does actually introduce a change in the object
code. Namely hugetlb's put_page will now also be done inline for non
compound pages - maybe this change is unacceptable though, but it does
cut down the ifdefs.

Re: [RFC] Making compound pages mandatory

[Nick Piggin]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

Nick Piggin wrote:
> Nick Piggin wrote:
> 
>>
>> Good idea. BTW, any reason why the following (very)micro optimisation
>> shouldn't go in?
>>
>> It currently only picks up a couple of things under fs/, but might help
>> reduce other ifdefery around the place. For example mm.h: page_count and
>> get_page.
>>
> 
> Like this, perhaps? It does actually introduce a change in the object
> code. Namely hugetlb's put_page will now also be done inline for non
> compound pages - maybe this change is unacceptable though, but it does
> cut down the ifdefs.

Err... attached.

[-- Attachment #2: mm-less-ifdefs.patch --]
[-- Type: text/x-patch, Size: 2391 bytes --]




---

 linux-2.6-npiggin/include/linux/mm.h |   20 ++++----------------
 linux-2.6-npiggin/mm/swap.c          |   20 +++++++-------------
 2 files changed, 11 insertions(+), 29 deletions(-)

diff -puN include/linux/mm.h~mm-less-ifdefs include/linux/mm.h
--- linux-2.6/include/linux/mm.h~mm-less-ifdefs	2004-11-17 14:14:12.000000000 +1100
+++ linux-2.6-npiggin/include/linux/mm.h	2004-11-17 14:17:24.000000000 +1100
@@ -288,11 +288,9 @@ struct page {
 
 extern void FASTCALL(__page_cache_release(struct page *));
 
-#ifdef CONFIG_HUGETLB_PAGE
-
 static inline int page_count(struct page *p)
 {
-	if (PageCompound(p))
+	if (unlikely(PageCompound(p)))
 		p = (struct page *)p->private;
 	return atomic_read(&(p)->_count) + 1;
 }
@@ -304,25 +302,15 @@ static inline void get_page(struct page 
 	atomic_inc(&page->_count);
 }
 
-void put_page(struct page *page);
-
-#else		/* CONFIG_HUGETLB_PAGE */
-
-#define page_count(p)		(atomic_read(&(p)->_count) + 1)
-
-static inline void get_page(struct page *page)
-{
-	atomic_inc(&page->_count);
-}
-
+void put_compound_page(struct page *page);
 static inline void put_page(struct page *page)
 {
+	if (unlikely(PageCompound(page)))
+		put_compound_page(page);
 	if (!PageReserved(page) && put_page_testzero(page))
 		__page_cache_release(page);
 }
 
-#endif		/* CONFIG_HUGETLB_PAGE */
-
 /*
  * Multiple processes may "see" the same page. E.g. for untouched
  * mappings of /dev/null, all processes see the same page full of
diff -puN mm/swap.c~mm-less-ifdefs mm/swap.c
--- linux-2.6/mm/swap.c~mm-less-ifdefs	2004-11-17 14:15:20.000000000 +1100
+++ linux-2.6-npiggin/mm/swap.c	2004-11-17 14:15:48.000000000 +1100
@@ -35,23 +35,17 @@
 int page_cluster;
 
 #ifdef CONFIG_HUGETLB_PAGE
-
-void put_page(struct page *page)
+void put_compound_page(struct page *page)
 {
-	if (unlikely(PageCompound(page))) {
-		page = (struct page *)page->private;
-		if (put_page_testzero(page)) {
-			void (*dtor)(struct page *page);
+	page = (struct page *)page->private;
+	if (put_page_testzero(page)) {
+		void (*dtor)(struct page *page);
 
-			dtor = (void (*)(struct page *))page[1].mapping;
-			(*dtor)(page);
-		}
-		return;
+		dtor = (void (*)(struct page *))page[1].mapping;
+		(*dtor)(page);
 	}
-	if (!PageReserved(page) && put_page_testzero(page))
-		__page_cache_release(page);
 }
-EXPORT_SYMBOL(put_page);
+EXPORT_SYMBOL(put_compound_page);
 #endif
 
 /*

_

Re: [RFC] Making compound pages mandatory

[Nick Piggin]

Nick Piggin wrote:

> -static inline void get_page(struct page *page)
> -{
> -	atomic_inc(&page->_count);
> -}
> -
> +void put_compound_page(struct page *page);
>  static inline void put_page(struct page *page)
>  {
> +	if (unlikely(PageCompound(page)))
> +		put_compound_page(page);
===>		return;
===>	}
>  	if (!PageReserved(page) && put_page_testzero(page))
>  		__page_cache_release(page);
>  }

... and that's something close to what it should look like.
Sorry, not having a good day today :\

Ignoring me might be the best option.

Re: [RFC] Making compound pages mandatory

[Linus Torvalds]

On Tue, 16 Nov 2004, Andrew Morton wrote:
> 
> So I'd suggest that we make compound pages conditional on a new
> CONFIG_COMPOUND_PAGE and make that equal to HUGETLB_PAGE || !MMU.

That sounds sane, and seems easily done in init/Kconfig. David?

[ There's too damn many Davids around. DavidH? Mr Howells? Dude? ]

	Linus

Re: [RFC] Making compound pages mandatory

[David Howells]

> > So I'd suggest that we make compound pages conditional on a new
> > CONFIG_COMPOUND_PAGE and make that equal to HUGETLB_PAGE || !MMU.
> 
> That sounds sane, and seems easily done in init/Kconfig. David?

That looks reasonable, I think.

Would you object to some more micro-optimisations:

 (1) Allow struct page to be marked __attribute__((align(sizeof(long)*2))) on
     archs that want it. On frv for instance this would allow the use of
     double-register load/store instructions when accessing certain adjacent
     pairs of members (such as page->lru.next and page->lru.prev). I think the
     frv compiler will emit these when it knows it won't cause a misalignment
     exception (8-byte accesses must be 8-byte aligned).

 (2) Allow put_page() to be provided by the arch if desired. This would allow
     me to do a better job of testing page flags and mucking around with the
     refcount by writing it in assembly. This is sort of prerequisite on (1)
     and the fact that accesses to page->_count are atomic and so are inline
     asm anyway.

> [ There's too damn many Davids around. DavidH? Mr Howells? Dude? ]

I generally refer to all other Davids as Dave, apart from David Woodhouse who
should be referred to as "Fish":-P

DavidH would be okay.

David

Re: [RFC] Making compound pages mandatory

[David Howells]

> Yes, it's just the single pointer chase.  Probably that's the common case
> now, because everyone will be enabling hugepages on lots of architectures.

No, it's not just the single pointer chase. See my email to Linus.

I would, however, be willing to endorse the use of PG_compound, especially
under !MMU conditions for two reasons:

 (1) Safety. The current mechanism of managing intermediate pages is
     potentially fragile under uClinux.

 (2) put_page() and get_page() aren't actually called all that often compared
     to other things, especially under uClinux.

David

Re: [RFC] Making compound pages mandatory

[David Howells]

> > Do you have any objection to compound pages being made mandatory, even
> > without HUGETLB support?
> 
> I haven't really looked into it, so I cannot make an informed decision.  
> How big is the overhead? And what's the _point_, since we don't seem to 
> need them normally, but the code is there for people who _do_ need them? 

The reason they might be useful under uClinux is that access_process_vm()
mucks around with pages in the middle of a high-order allocation.

Technically, with what was integrated to support uClinux and with the patch I
added, it's not actually necessary to use compound pages, I think, if only
because access_process_vm() provides some protections against munmap() and
exit().

However, that doesn't mean there isn't something I and the uClinux community
haven't noticed, or that situations won't arise in the future where the
current scheme is going to cause the kernel to explode.

> I don't generally like two paths, but quite frankly, I consider the
> non-compound case the regular case right now. How expensive does the
> compound pages end up being? Is it just one more pointer chase on every
> get_page/put_page, or what?

No, it's not. In put_page() it's _always_ going to be at least one more
pointer chase, and sometimes it's going to be two more; though the dcache may
offset this. put_page() refers not only to data in the first real page of a
compound page, but the second real page too.

Furthermore, put_page() becomes out of line. This means you've got register
clobberage and the mechanisms of function calls to deal with. Also you've got
more conditional instructions. I would propose also that the condition
checking for PG_compound be marked unlikely().

> Does anybody have numbers for before/after that are otherwise comparable?

I don't. However, the people hassling me about it might (hch for one).

David