Re: [PATCH] x86: create array based interface to change page attribute

[Arjan van de Ven <arjan@linux.intel.com>]

Thomas Hellström wrote:
> Dave Airlie wrote:
>> When cpa was refactored to the new set_memory_ interfaces, it removed
>> a special case fast path for AGP systems, which did a lot of page by page
>> attribute changing and held the flushes until they were finished. The
>> DRM memory manager also required this to get useable speeds.
>>
>> This introduces a new interface, which accepts an array of memory 
>> addresses
>> to have attributes changed on and to flush once finished.
>>
>> Further changes to the AGP stack to actually use this interface will be
>> published later.
>>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>> ---
>>  arch/x86/mm/pageattr-test.c  |   12 ++-
>>  arch/x86/mm/pageattr.c       |  164 
>> +++++++++++++++++++++++++++++++-----------
>>  include/asm-x86/cacheflush.h |    3 +
>>  3 files changed, 132 insertions(+), 47 deletions(-)
>>   
> ...
> Dave,
> Nice work, but how do we handle highmem pages? 

Cache attributes fundamentally work on a mapping and not on physical memory.
(MTRR's are special there, they do work on physical memory, but that's a special case and not relevant here)

So to be honest, your question doesn't make sense;  because all I can do is ask "which mapping of these pages".

Even the old interfaces prior to 2.6.24 only managed to deal with SOME of the mappings of a page.
And if/when future CPUs don't require all mappings to be in sync, clearly the kernel will only change
the mapping that is requested as well.



> Since this is an AGPgart and DRM fastpath, the interface should ideally 
> be adapted to match the data structures used by those callers. 

Actually, the interface has to make sense conceptually convey the right information,
the implementation should not have to second guess internals of AGP/DRM because that
would just be a recipe for disaster.
>The 
> AGPgart module uses an array of addresses, which effectively stops it 
> from using pages beyond the DMA32 limit. The DRM memory manager uses an 
> array of struct page pointers, but using that was, If I understand 
> things correctly, rejected.

yes because simply put, if you pass a struct page to such a function, you're not telling it which
mapping or mappings you want changed....
(And if you say "only the 1:1 mapping, so why doesn't the other side calculate that"... there's no speed gain in doing
the calculation for that on the other side of an interface, and that makes it zero reason to misdesign the interface
to only have the "which mapping" information implicit)