[parisc-linux] Bug in flush_dcache_page?

[parisc-linux] Bug in flush_dcache_page?

[Matthew Wilcox]

Thomas Bogendoerfer's being trying to get an L3000 to boot, and is seeing
similar problems to those seen by Bjorn on Superdome.  I think I have a lead.
Documentation/cachetlb.txt says:

  void flush_dcache_page(struct page *page)

        Any time the kernel writes to a page cache page, _OR_
        the kernel is about to read from a page cache page and
        user space shared/writable mappings of this page potentially
        exist, this routine is called.

Our implementation does:

static inline void flush_dcache_page(struct page *page)
{
        if (page->mapping && !page->mapping->i_mmap &&
                        !page->mapping->i_mmap_shared) {
                set_bit(PG_dcache_dirty, &page->flags);
        } else {
                flush_kernel_dcache_page(page_address(page));
        }
}

What we're missing is the case where the user has dirtied the page,
then the kernel calls flush_dcache_page before writing to the page.
This strikes me as a bad interface... there should be different interfaces
for `before the kernel writes' and `after the kernel writes', but let's
see if we can fix our implementation.

we have an interface to use, flush_user_dcache_range.  so we can iterate
over all the mappings and flush each of the user ranges.  btw, we're missing
an optimisation -- if page->mapping is NULL, we can set the PG_dcache_dirty
bit.  This lets us optimise a little more:

static inline void flush_dcache_page(struct page *page)
{
	if (!page->mapping || (page->mapping && !page->mapping->i_mmap &&
			!page->mapping->i_mmap_shared)) {
		set_bit(PG_dcache_dirty, &page->flags);
	} else {
		struct vm_area_struct *vma;
		for (vma = page->mapping->i_mmap; vma; vma = vma->vm_next) {
			flush_user_page(vma->vm_mm, vma->vm_start + page->index);
		}
		if (page->mapping->i_mmap_shared) {
			vma = page->mapping->i_mmap_shared;
			flush_user_page(vma->vm_mm, vma->vm_start + page->index);
		}
		flush_kernel_dcache_page(page_address(page));
	}
}

We only need to flush the first shared mapping because they're all congruent
modulo 4MB.  I suspect there are other optimisations possible (is it
really possible to have multiple _dirty_ pages on the i_mmap list?)
but this might work better.

OTOH this is adding a lot of complexity for an inline function.  maybe we
should just call flush_data_cache() and have done with it :-(

-- 
Revolutions do not require corporate support.

Re: [parisc-linux] Bug in flush_dcache_page?

[Matthew Wilcox]

On Wed, Oct 31, 2001 at 05:39:05PM +0000, Matthew Wilcox wrote:
> static inline void flush_dcache_page(struct page *page)
> {
> 	if (!page->mapping || (page->mapping && !page->mapping->i_mmap &&
> 			!page->mapping->i_mmap_shared)) {
> 		set_bit(PG_dcache_dirty, &page->flags);
> 	} else {
> 		struct vm_area_struct *vma;
> 		for (vma = page->mapping->i_mmap; vma; vma = vma->vm_next) {
> 			flush_user_page(vma->vm_mm, vma->vm_start + page->index);
> 		}
> 		if (page->mapping->i_mmap_shared) {
> 			vma = page->mapping->i_mmap_shared;
> 			flush_user_page(vma->vm_mm, vma->vm_start + page->index);
> 		}
> 		flush_kernel_dcache_page(page_address(page));
> 	}
> }

Aha.  A couple of things from talking to various VM/VFS people...
1. page->mapping is guaranteed to exist for pages in the page cache --
   this is how they're located.
2. If a page is simultaneously mapped in both user and kernel space, and
   user space touches it, either it leaves the page cache or it was mapped
   shared.

flush_dcache_page is only called on pages in the page cache, so we can
assume that page->mapping exists, and we don't need to flush any user
mappings of this page.

	if (!page->mapping->i_mmap & !page->mapping->i_mmap_shared) {
		set_bit(PG_dcache_dirty, &page->flags);
	} else {
		struct vm_area_struct *vma = page->mapping->i_mmap_shared;
		if (vma) {
			flush_user_page(vma->vm_mm, vma->vm_start + page->index);
		}
		flush_kernel_dcache_page(page_address(page));
	}

-- 
Revolutions do not require corporate support.

Re: [parisc-linux] Bug in flush_dcache_page?

[Carlos O'Donell Jr.]

> 
> Aha.  A couple of things from talking to various VM/VFS people...
> 1. page->mapping is guaranteed to exist for pages in the page cache --
>    this is how they're located.
> 2. If a page is simultaneously mapped in both user and kernel space, and
>    user space touches it, either it leaves the page cache or it was mapped
>    shared.
> 

Why would a user touch cause the page to leave the cache? 
(Maybe I'm not reading enough into it...)

The shared case I can see.

> flush_dcache_page is only called on pages in the page cache, so we can
> assume that page->mapping exists, and we don't need to flush any user
> mappings of this page.
> 
> 	if (!page->mapping->i_mmap & !page->mapping->i_mmap_shared) {
> 		set_bit(PG_dcache_dirty, &page->flags);
> 	} else {
> 		struct vm_area_struct *vma = page->mapping->i_mmap_shared;
> 		if (vma) {
> 			flush_user_page(vma->vm_mm, vma->vm_start + page->index);
> 		}
> 		flush_kernel_dcache_page(page_address(page));
> 	}
> 

Any particular reason for the bitwise and?

c.

Re: [parisc-linux] Bug in flush_dcache_page?

[Matthew Wilcox]

On Wed, Oct 31, 2001 at 08:56:06PM -0500, Carlos O'Donell Jr. wrote:
> > 2. If a page is simultaneously mapped in both user and kernel space, and
> >    user space touches it, either it leaves the page cache or it was mapped
> >    shared.
> 
> Why would a user touch cause the page to leave the cache? 
> (Maybe I'm not reading enough into it...)
> 
> The shared case I can see.

I meant `dirty' rather than `touch'.  If the page is mapped shared, writes
to it are visible to all users.  If it's mapped private, a write does
a copy-on-write.  The page leaves the page cache at this point because its
contents are no longer valuable to anyone except the process.

> > 	if (!page->mapping->i_mmap & !page->mapping->i_mmap_shared) {
> > 		set_bit(PG_dcache_dirty, &page->flags);
> > 	} else {
>
> Any particular reason for the bitwise and?

It's not a sequence point.  So it doesn't have to be test, branch, test,
branch; it can be test, test, branch.  I'm not sure how the compiler does
on this, to be honest.  It's a microoptimisation that's almost certainly
not worth making.

-- 
Revolutions do not require corporate support.