LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* issue with patch "x86: no CPA on iounmap"
@ 2008-02-05  1:13 Siddha, Suresh B
  2008-02-05  4:41 ` Arjan van de Ven
  2008-02-05  4:48 ` Arjan van de Ven
  0 siblings, 2 replies; 6+ messages in thread
From: Siddha, Suresh B @ 2008-02-05  1:13 UTC (permalink / raw)
  To: tglx, mingo; +Cc: linux-kernel, arjan

This is wrt to x86 git commit f56d005d30342a45d8af2b75ecccc82200f09600
	"x86: no CPA on iounmap"

This can use performance issue. When a GART driver unmaps a RAM page,
which was mapped as UC, this commit will still retain UC attribute
on the kernel identity mapping. This can cause mysterious performance issue
if this freed page gets used by kernel later.

For now we should change the attribute during iounmap and in future PAT
infrastructure will have necessary hooks to avoid the aliasing issues.

thanks,
suresh

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: issue with patch "x86: no CPA on iounmap"
  2008-02-05  1:13 issue with patch "x86: no CPA on iounmap" Siddha, Suresh B
@ 2008-02-05  4:41 ` Arjan van de Ven
  2008-02-05  4:48 ` Arjan van de Ven
  1 sibling, 0 replies; 6+ messages in thread
From: Arjan van de Ven @ 2008-02-05  4:41 UTC (permalink / raw)
  To: Siddha, Suresh B; +Cc: tglx, mingo, linux-kernel

Siddha, Suresh B wrote:
> This is wrt to x86 git commit f56d005d30342a45d8af2b75ecccc82200f09600
> 	"x86: no CPA on iounmap"
> 
> This can use performance issue. When a GART driver unmaps a RAM page,
> which was mapped as UC, this commit will still retain UC attribute
> on the kernel identity mapping. This can cause mysterious performance issue
> if this freed page gets used by kernel later.
> 
> For now we should change the attribute during iounmap and in future PAT
> infrastructure will have necessary hooks to avoid the aliasing issues.
> 

this is a hard one; because the flipside is someone ioremapping the same page twice
(which is not impossible if there's 2 1k bars)... and then one of them gets iounmapped,
which would turn the page cachable again.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: issue with patch "x86: no CPA on iounmap"
  2008-02-05  1:13 issue with patch "x86: no CPA on iounmap" Siddha, Suresh B
  2008-02-05  4:41 ` Arjan van de Ven
@ 2008-02-05  4:48 ` Arjan van de Ven
  2008-02-05  7:05   ` Ingo Molnar
  1 sibling, 1 reply; 6+ messages in thread
From: Arjan van de Ven @ 2008-02-05  4:48 UTC (permalink / raw)
  To: Siddha, Suresh B; +Cc: tglx, mingo, linux-kernel

Siddha, Suresh B wrote:
> This is wrt to x86 git commit f56d005d30342a45d8af2b75ecccc82200f09600
> 	"x86: no CPA on iounmap"
> 
> This can use performance issue. When a GART driver unmaps a RAM page,

thinking about this some more...

afaik the gart driver doesn't use ioremap....

(and it does caching control explicitly, and sets its pages back to cached)


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: issue with patch "x86: no CPA on iounmap"
  2008-02-05  4:48 ` Arjan van de Ven
@ 2008-02-05  7:05   ` Ingo Molnar
  2008-02-05  7:14     ` Arjan van de Ven
  2008-02-07 20:02     ` Siddha, Suresh B
  0 siblings, 2 replies; 6+ messages in thread
From: Ingo Molnar @ 2008-02-05  7:05 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: Siddha, Suresh B, tglx, linux-kernel


* Arjan van de Ven <arjan@linux.intel.com> wrote:

> Siddha, Suresh B wrote:
>> This is wrt to x86 git commit f56d005d30342a45d8af2b75ecccc82200f09600
>> 	"x86: no CPA on iounmap"
>>
>> This can use performance issue. When a GART driver unmaps a RAM page,
>
> thinking about this some more...
>
> afaik the gart driver doesn't use ioremap....
>
> (and it does caching control explicitly, and sets its pages back to 
> cached)

there are many GART drivers, and the method used depends on the GART 
driver. The following GART drivers still use ioremap in one way or 
another:

 drivers/char/agp/amd-k7-agp.c
 drivers/char/agp/ati-agp.c
 drivers/char/agp/generic.c
 drivers/char/agp/sworks-agp.c
 drivers/char/drm/radeon_cp.c

the method use is in all cases the same: they use __get_free_page() to 
pick up a general RAM page, they do SetPageReserved() and then they use 
ioremap_nocache() to map it non-cached, and then they also program the 
GART to access those pages.

when the GART code deinits, it does an iounmap() on those pages, unmaps 
it from the GART hardware itself, does a ClearPageReserved() and does 
__free_page() to put the page into the general page pool again. So 
Suresh is right: these pages are currently marked UC at this point and 
we need to mark them cacheable.

we could do this automatically in iounmap() upon seeing a page_is_ram() 
that has PageReserved set. Or we could stick in a set_memory_wb() into 
the deinit [and ioremap_nocache()-failure] sequence.

Since we treat PageReserved pages specially in ioremap() already [we 
allow them, despite them being listed in the e820 map], i think the more 
robust solution is to recognize them in iounmap() as well - this way it 
cannot be forgotten accidentally. (and UC pages in the buddy are _hard_ 
to notice after the fact) There is no aliasing danger i believe: IO bars 
should never be marked as general RAM in the e820.

	Ingo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: issue with patch "x86: no CPA on iounmap"
  2008-02-05  7:05   ` Ingo Molnar
@ 2008-02-05  7:14     ` Arjan van de Ven
  2008-02-07 20:02     ` Siddha, Suresh B
  1 sibling, 0 replies; 6+ messages in thread
From: Arjan van de Ven @ 2008-02-05  7:14 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: Siddha, Suresh B, tglx, linux-kernel

Ingo Molnar wrote:
> * Arjan van de Ven <arjan@linux.intel.com> wrote:
> 
>> Siddha, Suresh B wrote:
>>> This is wrt to x86 git commit f56d005d30342a45d8af2b75ecccc82200f09600
>>> 	"x86: no CPA on iounmap"
>>>
>>> This can use performance issue. When a GART driver unmaps a RAM page,
>> thinking about this some more...
>>
>> afaik the gart driver doesn't use ioremap....
>>
>> (and it does caching control explicitly, and sets its pages back to 
>> cached)
> 
> there are many GART drivers, and the method used depends on the GART 
> driver. The following GART drivers still use ioremap in one way or 
> another:
> 
>  drivers/char/agp/amd-k7-agp.c
>  drivers/char/agp/ati-agp.c
>  drivers/char/agp/generic.c
>  drivers/char/agp/sworks-agp.c
>  drivers/char/drm/radeon_cp.c
> 
> the method use is in all cases the same: they use __get_free_page() to 
> pick up a general RAM page, they do SetPageReserved() and then they use 
> ioremap_nocache() to map it non-cached, and then they also program the 
> GART to access those pages.
> 
> when the GART code deinits, it does an iounmap() on those pages, unmaps 
> it from the GART hardware itself, does a ClearPageReserved() and does 
> __free_page() to put the page into the general page pool again. So 
> Suresh is right: these pages are currently marked UC at this point and 
> we need to mark them cacheable.
> 
> we could do this automatically in iounmap() upon seeing a page_is_ram() 
> that has PageReserved set. Or we could stick in a set_memory_wb() into 
> the deinit [and ioremap_nocache()-failure] sequence.
> 
> Since we treat PageReserved pages specially in ioremap() already [we 
> allow them, despite them being listed in the e820 map], i think the more 
> robust solution is to recognize them in iounmap() as well - this way it 
> cannot be forgotten accidentally. (and UC pages in the buddy are _hard_ 
> to notice after the fact) There is no aliasing danger i believe: IO bars 
> should never be marked as general RAM in the e820.
> 

agreed, esp for .25

it's sort of a weird case of ioremap() use; I wonder if longer term we need
to have a different sort of interface for this kind of use...

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: issue with patch "x86: no CPA on iounmap"
  2008-02-05  7:05   ` Ingo Molnar
  2008-02-05  7:14     ` Arjan van de Ven
@ 2008-02-07 20:02     ` Siddha, Suresh B
  1 sibling, 0 replies; 6+ messages in thread
From: Siddha, Suresh B @ 2008-02-07 20:02 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: Arjan van de Ven, Siddha, Suresh B, tglx, linux-kernel

On Tue, Feb 05, 2008 at 08:05:35AM +0100, Ingo Molnar wrote:
> there are many GART drivers, and the method used depends on the GART 
> driver. The following GART drivers still use ioremap in one way or 
> another:

There is another issue I see in the recent pageattr changes, again in the GART
driver context.

GART drivers use map_page_into_agp() and unmap_page_from_agp() during
runtime (and not just during load and unload of the driver module). In the
recent pageattr changes, we seem to have dropped the concept of reverting
to the large page(for the kernel identity mapping) while changing the attribute
back to WB. In this GART driver context, over the time, potentially
kernel identity mappings might be backed by small pages, if we don't
revert to large page again during set_memory_wb() which gets called during
unmap_page_from_agp() for a RAM page. And thus loosing the advantage of large
page mapping for kernel identity mappings.

thanks,
suresh

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2008-02-07 20:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-05  1:13 issue with patch "x86: no CPA on iounmap" Siddha, Suresh B
2008-02-05  4:41 ` Arjan van de Ven
2008-02-05  4:48 ` Arjan van de Ven
2008-02-05  7:05   ` Ingo Molnar
2008-02-05  7:14     ` Arjan van de Ven
2008-02-07 20:02     ` Siddha, Suresh B

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).