Re: PAT support

[Terence Ripperda <tripperda@nvidia.com>]

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


Thanks for the great feedback (as always), Andi.

I've gotten started on this, mainly focusing on modifying change_page_attr to use unsigned long instead of struct page*. hit a few snags on the way, but got things figured out. it should be working now, but may still have a bug or two I need to work out. I also moved cmap_request_range into change_page_attr as suggested.

what I wanted to bring up is some of the issues I'm running into that will require some slight adjustments. I wanted to make sure to get feedback on this.

the first is that change_page_attr is basically a one-way operation that just sets the caching for a range of pages. in contrast, the cmap_ api is more of a push/pop approach, with cmap_request_range and cmap_release_range. all callers of change_page_attr generally use it to set a new attribute, then assume they need to restore the attribute back to PAGE_KERNEL (WB). additionally, when splitting/merging large pages, change_page_attr has similar assumptions (that we flip between a default PAGE_KERNEL caching and some other caching).

I haven't thought through all the details yet, but would it be acceptable to consider modifying change_page_attr to also use a push/pop approach? it may also make sense to remove the assumptions about initial state and actually save off what the initial state is, so it can be restored to that state.

the second is in iounmap. this function only takes an address, we don't get the size until we've retrieved the struct vm_struct* by freeing the mapping via remove_vm_area. once we have this size, we can call change_page_attr. but by then, the virtual mapping was already freed in remove_vm_area, so playing with the page tables in change_page_attr causes problems:

remap_area_pte: page already exists
------------[ cut here ]------------
kernel BUG at arch/i386/mm/ioremap.c:37!
invalid operand: 0000 [#1]
...
Call Trace:
 [<c011d9c4>] __ioremap+0xbc/0xec
 [<c0228012>] acpi_os_map_memory+0x2a/0x44
 [<c023c11a>] acpi_tb_get_table_header+0x52/0xdc
 [<c023c076>] acpi_tb_get_table+0x16/0x68
 [<c023fb87>] acpi_ut_allocate_owner_id+0x8f/0x9c
...

in this case, I think change_page_attr filled the pte back in after it had been freed by remove_vm_area.

this could be fixed multiple ways: change_page_attr could not touch the page tables at all for i/o regions (would ioremap cover everything? are 4M ptes used on i/o regions?), we could check for the PAGE_PRESENT bit (seems like a hack workaround), I could add an api to retrieve the struct vm_struct* so we could call change_page_attr before calling remove_vm_area. I wanted to check and see what the preferred approach was.

(aside: we only call change_page_attr if we're calling a non-standard ioremap, like ioremap_nocache. I need to move this logic into ioremap, so we get a cmap_ for all ioremaps, regardless of caching type. note that iounmap currently has no idea what caching ioremap requested.)

also, I'm attaching my current progress. note that this is a debug-work-in-progress, so has lots of extra printouts and perhaps some code that would be cleaned up (for example, I duplicated lookup_address -> lookup_phys_address to make some things easier, but that might be cleaned up before long). I also haven't had time to get to all the suggestions, but will get to them.

Thanks,
Terence

[-- Attachment #2: cachemap-1.10-pre1-2.6.4.bz2 --]
[-- Type: application/octet-stream, Size: 13324 bytes --]