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
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.
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)
* Arjan van de Ven <[email protected]> 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
Ingo Molnar wrote:
> * Arjan van de Ven <[email protected]> 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...
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