Re: another new version of pageattr caching conflict fix for 2.4

[Andi Kleen]

On Sun, Jun 16, 2002 at 04:08:49AM -0600, Eric W. Biederman wrote:
> Andi Kleen <[e-mail]> writes:
> 
> > On Fri, Jun 14, 2002 at 12:31:33PM -0500, Andrea Arcangeli wrote:
> > > On Fri, Jun 14, 2002 at 06:13:28PM +0200, Andi Kleen wrote:
> > > > +#ifdef CONFIG_HIGHMEM
> > > > +	/* Hopefully not be mapped anywhere else. */
> > > > +	if (page >= highmem_start_page) 
> > > > +		return 0;
> > > > +#endif
> > > 
> > > there's no hope here. If you don't want to code it right because nobody
> > > is exercising such path and flush both any per-cpu kmap-atomic slot and
> > > the page->virtual, please place a BUG() there or any more graceful
> > > failure notification.
> > 
> > Ok done. 
> > 
> > I also removed the DRM change because it was buggy. I'm not 100% yet
> > it is even a problem. Needs more investigation.
> > 
> > BTW there is another corner case that was overlooked:  
> > mmap of /dev/kmem, /proc/kcore, /dev/mem. To handle this correctly 
> > the mmap functions would need to always walk the kernel page table
> > and force the correct caching attribute in the user mapping.
> > But what to do when there is already an existing mapping to user space?
> 
> Don't allow the change_page_attr if page->count > 1 is an easy solution,
> and it probably suffices.  Beyond that anything mmaped can be found

Erm, what should it do then? Fail the AGP map ?

> by walking into the backing address space, and then through the
> vm_area_structs found with i_mmap && i_mmap_shared.  Of course the
> vm_area_structs may possibly need to break because of the multiple
> page protections.

I know that, but it seems rather racy expensive and complicated.

> 
> > > > +int change_page_attr(struct page *page, int numpages, pgprot_t prot)
> > > > +{
> > > 
> > > this API not the best, again I would recommend something on these lines:
> > 
> > Hmm: i would really prefer to do the allocation in the caller.
> > If you want your API you could do  just do it as a simple wrapper to
> > the existing function (with the new numpages it would be even 
> > efficient) 
> 
> Using pgprot_t to convey the cacheablity of a page appears to be an
> abuse.  At the very least we need a PAGE_CACHE_MASK, to find just
> the cacheability attributes.

change_page_attr is more than just cachability. You can use it e.g.
to write protect kernel pages not (if you wanted to do that for some
reason) 

The current one is fine with PAGE_KERNEL_NOCACHE, you could eventually
define PAGE_KERNEL_WRITECOMBINING too if you wanted.

> And we should really consider using the other cacheability page
> attributes on x86, not just cache enable/disable.  Using just mtrr's
> is limiting in that you don't always have enough of them, and that
> sometimes valid ram is mapped uncacheable, because of the mtrr
> alignment restrictions. 

Exposing it to user space in a more general way is tricky because
you need to avoid aliases with different caching attributes and also
change the kernel map as needed (would be surely an interesting 
project, but definitely requires more work) 

You can already use WT. DRM does that already in fact. 
Newer Intel/AMD CPUs allow to set up a more general PAT table with some
more modis, but to be honest I don't see the point in most of them,
except perhaps WC. Unfortunately there is no 'weak ordering like alpha/sparc64'
mode that could be used in the kernel :-)

-Andi