Re: [PATCHSET] block: fix PIO cache coherency bug

[Tejun Heo]

James Bottomley wrote:
> On Wed, 2006-02-22 at 17:27 +0900, Tejun Heo wrote:
>> \This thread has been dead for quite some time mainly because I didn't
>> know what to do.  As it is a real outstanding bug bugging people and
>> Matt Reimer thankfully reminded me[1], I'm giving another shot at
>> resolving this.
>>
>> People seem to agree that it is the responsibility of the driver to
>> make sure read data gets to the page cache page (or whatever kernel
>> page).  Only driver knows how and when.
>>
>> The objection raised by James Bottomley is that although syncing the
>> kernel page is the responsbility of the driver, syncing user page is
>> not; thus, use of flush_dcache_page() is excessive.  James suggested
>> use of flush_kernel_dcache_page().
>>
>> I also asked similar question[2] on lkml and Russell replied that
>> depending on arch implementation it shouldn't be much of a problem[3].
>> Another thing to consider is that all other drivers which currently
>> manage cache coherency use flush_dcache_page().
>>
>> So, the questions are...
>>
>> q1. James, besides from the use of flush_dcache_page(), do you agree
>>     with the block layer kmap/kunmap API?
>>
>> 2. Is flush_kernel_dcache_page() the correct one?
>>
>> Whether or not flush_kernel_dcache_page() is the one or not, I think
>> we should first go with flush_dcache_page() as that's what drivers
>> have been doing upto this point.  Switching from flush_dcache_page()
>> to flush_kernel_dcache_page() is very easy and should be done in a
>> separate patch anyway.  No?
>>
>> Another thing mind is that this problem is not limited block drivers.
>> All the codes that perform writes to kmap'ed pages take care of
>> synchronization themselves and the popular choice seems to be
>> flush_dcache_page().
>>
>> IMHO, kmap API should have a flag or something to tell it how the page
>> is being used such that kmap API can take care of synchronization like
>> dma mapping API does rather than scattering sync code all over the
>> kernel.  And if that's the right thing to do, some of blk kmap
>> wrappers can/should be removed.
>>
>> What do you guys think?
> 
> Here's my proposal to break this logjam.
> 
> I'm proposing introducing a new memory coherency API:
> 
> flush_kernel_dcache_page()
> 
> Which would be tasked with bringing cache coherency back to the kernel's
> image of a user page after the kernel has modified it.  On parisc this
> will be a simple flush through the kernel cache.
> 
> I think on arm this should be implemented as
> 
> __cpuc_flush_dcache_page(page_address(page))
> 
> but you can implement this as flush_dcache_page() if you wish (I warn
> you now that you have the same flush_dcache_mmap_lock problem that we
> have on parisc, so if you do this, you'll return from
> flush_dcache_page() with interrupts enabled, but at least this will no
> longer be a parisc problem).
> 
> If everyone's happy with this approach, I'll take it over to linux-arch.
> 
> James
> 
> diff --git a/Documentation/cachetlb.txt b/Documentation/cachetlb.txt
> index 4ae4188..6232dd7 100644
> --- a/Documentation/cachetlb.txt
> +++ b/Documentation/cachetlb.txt
> @@ -362,6 +362,15 @@ maps this page at its virtual address.
>  	likely that you will need to flush the instruction cache
>  	for copy_to_user_page().
>  
> +  void flush_kernel_dcache_page(struct page *page)
> +	When the kernel needs to modify a user page is has obtained
> +	with kmap, it calls this function after all modifications are
> +	complete (but before kunmapping it) to bring the underlying
> +	page up to date.  It is assumed here that the user has no
> +	incoherent cached copies (i.e. the original page was obtained
> +	from a mechanism like get_user_pages())
> +
> +
>    void flush_icache_range(unsigned long start, unsigned long end)
>    	When the kernel stores into addresses that it will execute
>  	out of (eg when loading modules), this function is called.
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index 6bece92..157bd2f 100644
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -7,6 +7,12 @@
>  
>  #include <asm/cacheflush.h>
>  
> +#ifndef ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE
> +static inline void flush_kernel_dcache_page(struct page *page)
> +{
> +}
> +#endif
> +
>  #ifdef CONFIG_HIGHMEM
>  
>  #include <asm/highmem.h>
> 
> 

Okay by me, although I think we also need to fix flush_dcache_page()
such that it doesn't abruptly enable irq after itself. That's just
broken. I'll make some kmap wrappers such that callers don't have to try
too hard to get synchronization correct.

Thanks.

-- 
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/