Re: [patch for 2.6.33? 1/1] ata: call flush_dcache_page() around PIO data transfers in libata-aff.c

[Catalin Marinas <catalin.marinas@arm.com>]

On Wed, 2010-02-03 at 17:15 +0000, Jeff Garzik wrote:
> On 02/03/2010 12:06 PM, Andrew Morton wrote:
> > On Wed, 03 Feb 2010 12:00:58 -0500 Jeff Garzik<jgarzik@pobox.com>  wrote:
> >
> >> On 02/03/2010 11:40 AM, James Bottomley wrote:
> >>> The fix to libata looks to be just that it should kmap all the time
> >>> rather than trying to fiddle with the page to see if its higmem.  For
> >>> kmap on a normal page, we should just return the offset map address and
> >>> do all the flushing.
> >>
> >> libata tests PageHighMem() because it was measurably faster to do things
> >> the current way (which includes local_irq_save/restore, only for
> >> highmem) on boxes where it actually matters.
> >>
> >> It seems more efficient to add a flush where necessary, than
> >> unconditionally punish everyone...
> >
> > kmap_atomic() tests PageHighMem() too - it's pretty lightweight for
> > lowmem pages.
> 
> Note the lack of local_irq_save/restore in our code, though...  These
> PIO xfers are __slow__, from the perspective of a CPU manufactured in
> the past decade; you are definitely disabling local interrupts for a
> long time.  I suppose we could do
> 
>         if (high mem)
>                 local irq save
>                 kmap
>                 xfer
>                 kunmap
>                 local irq restore
>         else
>                 kmap
>                 xfer
>                 kunmap
> 
> does that solve the problem for ARM, for 2.6.33?

No, it doesn't. If the page isn't a highmem one, the kunmap doesn't do
anything to the caches (and I also have highmem disabled). My logic was
based on a statement in cachetlb.txt that flush_dcache_page() should be
called if the kernel writes to a page cache page, hence the patch (as it
was pointed out, the first call to flush_dcache_page() isn't needed).

-- 
Catalin