[patch 2/2] reduce tlb/cache flush times of agpgart memory allocation

[patch 2/2] reduce tlb/cache flush times of agpgart memory allocation

[Shaohua Li]

To reduce tlb/cache flush, makes agp memory allocation do one flush
after all pages in a region are changed to uc.

All agp drivers except agp-sgi uses agp_generic_alloc_page()
for .agp_alloc_page, so the patch should work for them. agp-sgi is only
for ia64, so not a problem too. 

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
 drivers/char/agp/agp.h     |    4 ++++
 drivers/char/agp/generic.c |    4 +++-
 include/asm-x86/agp.h      |    3 +++
 3 files changed, 10 insertions(+), 1 deletion(-)

Index: linux/drivers/char/agp/generic.c
===================================================================
--- linux.orig/drivers/char/agp/generic.c	2008-08-04 12:03:37.000000000 +0800
+++ linux/drivers/char/agp/generic.c	2008-08-04 12:06:31.000000000 +0800
@@ -274,6 +274,7 @@ struct agp_memory *agp_allocate_memory(s
 		new->memory[i] = virt_to_gart(addr);
 		new->page_count++;
 	}
+	map_page_into_agp_global_flush();
 	new->bridge = bridge;
 
 	return new;
@@ -1186,7 +1187,8 @@ void *agp_generic_alloc_page(struct agp_
 	if (page == NULL)
 		return NULL;
 
-	map_page_into_agp(page);
+	/* agp_allocate_memory will do flush */
+	map_page_into_agp_noflush(page);
 
 	get_page(page);
 	atomic_inc(&agp_bridge->current_memory_agp);
Index: linux/include/asm-x86/agp.h
===================================================================
--- linux.orig/include/asm-x86/agp.h	2008-08-04 12:03:37.000000000 +0800
+++ linux/include/asm-x86/agp.h	2008-08-04 12:06:31.000000000 +0800
@@ -15,6 +15,9 @@
 #define map_page_into_agp(page) set_pages_uc(page, 1)
 #define unmap_page_from_agp(page) set_pages_wb(page, 1)
 
+#define map_page_into_agp_noflush(page) set_pages_uc_noflush(page, 1)
+#define map_page_into_agp_global_flush() set_memory_flush_all()
+
 /*
  * Could use CLFLUSH here if the cpu supports it. But then it would
  * need to be called for each cacheline of the whole page so it may
Index: linux/drivers/char/agp/agp.h
===================================================================
--- linux.orig/drivers/char/agp/agp.h	2008-08-04 12:03:37.000000000 +0800
+++ linux/drivers/char/agp/agp.h	2008-08-04 12:06:31.000000000 +0800
@@ -30,6 +30,10 @@
 #define _AGP_BACKEND_PRIV_H 1
 
 #include <asm/agp.h>	/* for flush_agp_cache() */
+#ifndef map_page_into_agp_noflush
+#define map_page_into_agp_noflush(page) map_page_into_agp(page)
+#define map_page_into_agp_global_flush()
+#endif
 
 #define PFX "agpgart: "
 

Re: [patch 2/2] reduce tlb/cache flush times of agpgart memory allocation

[Ingo Molnar]

* Shaohua Li <shaohua.li@intel.com> wrote:

> To reduce tlb/cache flush, makes agp memory allocation do one flush 
> after all pages in a region are changed to uc.
> 
> All agp drivers except agp-sgi uses agp_generic_alloc_page() for 
> .agp_alloc_page, so the patch should work for them. agp-sgi is only 
> for ia64, so not a problem too.

applied to tip/x86/pat - thanks!

I've Cc:-ed more PAT folks - any objections?

	Ingo

Re: [patch 2/2] reduce tlb/cache flush times of agpgart memory allocation

[Arjan van de Ven]

On Fri, 15 Aug 2008 16:31:31 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Shaohua Li <shaohua.li@intel.com> wrote:
> 
> > To reduce tlb/cache flush, makes agp memory allocation do one flush 
> > after all pages in a region are changed to uc.
> > 
> > All agp drivers except agp-sgi uses agp_generic_alloc_page() for 
> > .agp_alloc_page, so the patch should work for them. agp-sgi is only 
> > for ia64, so not a problem too.
> 
> applied to tip/x86/pat - thanks!
> 
> I've Cc:-ed more PAT folks - any objections?
> 

it really needs something else instead; it needs airlied's array
allocator
otherwise you hit the second wall as well (the pat checks per page)

in reality an array version of ioremap (or set_pages_X) is what is
needed

Re: [patch 2/2] reduce tlb/cache flush times of agpgart memory allocation

[Ingo Molnar]

* Arjan van de Ven <arjan@infradead.org> wrote:

> On Fri, 15 Aug 2008 16:31:31 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > * Shaohua Li <shaohua.li@intel.com> wrote:
> > 
> > > To reduce tlb/cache flush, makes agp memory allocation do one flush 
> > > after all pages in a region are changed to uc.
> > > 
> > > All agp drivers except agp-sgi uses agp_generic_alloc_page() for 
> > > .agp_alloc_page, so the patch should work for them. agp-sgi is only 
> > > for ia64, so not a problem too.
> > 
> > applied to tip/x86/pat - thanks!
> > 
> > I've Cc:-ed more PAT folks - any objections?
> > 
> 
> it really needs something else instead; it needs airlied's array 
> allocator otherwise you hit the second wall as well (the pat checks 
> per page)
> 
> in reality an array version of ioremap (or set_pages_X) is what is 
> needed

ok, agreed.

	Ingo

RE: [patch 2/2] reduce tlb/cache flush times of agpgart memory allocation

[Li, Shaohua]

>-----Original Message-----
>From: Arjan van de Ven [mailto:arjan@infradead.org]
>Sent: Friday, August 15, 2008 10:41 PM
>To: Ingo Molnar
>Cc: Li, Shaohua; lkml; airlied@linux.ie; Andrew Morton; Ingo Molnar; Siddha,
>Suresh B; Pallipadi, Venkatesh; Thomas Gleixner; H. Peter Anvin
>Subject: Re: [patch 2/2] reduce tlb/cache flush times of agpgart memory
>allocation
>
>On Fri, 15 Aug 2008 16:31:31 +0200
>Ingo Molnar <mingo@elte.hu> wrote:
>
>>
>> * Shaohua Li <shaohua.li@intel.com> wrote:
>>
>> > To reduce tlb/cache flush, makes agp memory allocation do one flush
>> > after all pages in a region are changed to uc.
>> >
>> > All agp drivers except agp-sgi uses agp_generic_alloc_page() for
>> > .agp_alloc_page, so the patch should work for them. agp-sgi is only
>> > for ia64, so not a problem too.
>>
>> applied to tip/x86/pat - thanks!
>>
>> I've Cc:-ed more PAT folks - any objections?
>>
>
>it really needs something else instead; it needs airlied's array
>allocator
>otherwise you hit the second wall as well (the pat checks per page)
Somebody should have a measurement. In my test, the real bottleneck is the cache flush. It appears flush cache page is slow if there are a lot of pages, In my patch, I use a wbinvd. This can be optimized to do wbinvd with a threshold. Maybe airlied can change his patch with this way.

Thanks,
Shaohua

Re: [patch 2/2] reduce tlb/cache flush times of agpgart memory allocation

[Arjan van de Ven]

On Mon, 18 Aug 2008 09:21:12 +0800
"Li, Shaohua" <shaohua.li@intel.com> wrote:

> 
> >
> >it really needs something else instead; it needs airlied's array
> >allocator
> >otherwise you hit the second wall as well (the pat checks per page)
> Somebody should have a measurement. In my test, the real bottleneck
> is the cache flush. It appears flush cache page is slow if there are
> a lot of pages, In my patch, I use a wbinvd. This can be optimized to
> do wbinvd with a threshold. Maybe airlied can change his patch with
> this way.


it would be great if you had time to update his patch and this to
it...

and the logic probably should be "if there's more than X pags in the
the array, just use wbinvd".
Although wbinvd is very painful if you have 12Mb of cache and you wipe
it for all cores in the system ;-(



-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

RE: [patch 2/2] reduce tlb/cache flush times of agpgart memory allocation

[Li, Shaohua]

>-----Original Message-----
>From: Arjan van de Ven [mailto:arjan@infradead.org]
>Sent: Monday, August 18, 2008 11:57 AM
>To: Li, Shaohua
>Cc: Ingo Molnar; lkml; airlied@linux.ie; Andrew Morton; Ingo Molnar; Siddha,
>Suresh B; Pallipadi, Venkatesh; Thomas Gleixner; H. Peter Anvin
>Subject: Re: [patch 2/2] reduce tlb/cache flush times of agpgart memory
>allocation
>
>On Mon, 18 Aug 2008 09:21:12 +0800
>"Li, Shaohua" <shaohua.li@intel.com> wrote:
>
>>
>> >
>> >it really needs something else instead; it needs airlied's array
>> >allocator
>> >otherwise you hit the second wall as well (the pat checks per page)
>> Somebody should have a measurement. In my test, the real bottleneck
>> is the cache flush. It appears flush cache page is slow if there are
>> a lot of pages, In my patch, I use a wbinvd. This can be optimized to
>> do wbinvd with a threshold. Maybe airlied can change his patch with
>> this way.
>
>
>it would be great if you had time to update his patch and this to
>it...
I'll do it soon.

>and the logic probably should be "if there's more than X pags in the
>the array, just use wbinvd".
>Although wbinvd is very painful if you have 12Mb of cache and you wipe
>it for all cores in the system ;-(
This might not be that bad, changing attribute isn't frequently used.

Thanks,
Shaohua

Re: [patch 2/2] reduce tlb/cache flush times of agpgart memory allocation

[Ingo Molnar]

* Li, Shaohua <shaohua.li@intel.com> wrote:

> > it would be great if you had time to update his patch and this to 
> > it...
>
> I'll do it soon.

great! Please do it as a delta patch against tip/master:

   http://people.redhat.com/mingo/tip.git/README

as your first two patches are being tested already:

 466ae83: reduce tlb/cache flush times of agpgart memory allocation
 1ac2f7d: introduce two APIs for page attribute

[ but it can all only go upstream once the observations from Arjan have
  been addressed. ]

> > and the logic probably should be "if there's more than X pags in the 
> > the array, just use wbinvd". Although wbinvd is very painful if you 
> > have 12Mb of cache and you wipe it for all cores in the system ;-(
>
> This might not be that bad, changing attribute isn't frequently used.

dont some X/DRM drivers do it for a wide range of GX ops?

	Ingo

Re: [patch 2/2] reduce tlb/cache flush times of agpgart memory allocation

[H. Peter Anvin]

Ingo Molnar wrote:
> * Li, Shaohua <shaohua.li@intel.com> wrote:
> 
>>> it would be great if you had time to update his patch and this to 
>>> it...
>> I'll do it soon.
> 
> great! Please do it as a delta patch against tip/master:
> 
>    http://people.redhat.com/mingo/tip.git/README
> 
> as your first two patches are being tested already:
> 
>  466ae83: reduce tlb/cache flush times of agpgart memory allocation
>  1ac2f7d: introduce two APIs for page attribute
> 
> [ but it can all only go upstream once the observations from Arjan have
>   been addressed. ]
> 
>>> and the logic probably should be "if there's more than X pags in the 
>>> the array, just use wbinvd". Although wbinvd is very painful if you 
>>> have 12Mb of cache and you wipe it for all cores in the system ;-(
>> This might not be that bad, changing attribute isn't frequently used.
> 
> dont some X/DRM drivers do it for a wide range of GX ops?
> 

I think so... at least it's likely to become more common. 
Realistically, it means WBINVD, being uninterruptible, is probably 
unsafe even for very large amounts.

	-hpa