Execute from CF causes segmentation faults

Execute from CF causes segmentation faults

[James Steward]

G'day all,

The message below is a response from Russell King on the arm 
linux mailing lists regarding a problem I encountered, and as
it turns out has been encountered before.  I'm not sure if anyone
from the IDE development group is working on this but as it is
more ide specific it may be best handled by some clever guru 
from this mail list. 

The symptom I see is that executing code from a CF disk causes
SEGVs and other nasties.  The workaround is to add a kernel
command line arg "cachepolicy=writethrough".  This is seen as a
bit of a bandaid and not a "good" solution.

Could someone please take a look?

Regards,
James.


On Thu, Dec 01, 2005 at 01:44:59PM +1300, Charles Manning wrote:
> Something that confuses me lightly is why this problem should exist at
> all.
>
> It seems that the problem is caused by stale data in the cache.
> Surely cache data should never be stale (ie. Anything in the cache
> should be current).

No.  I think what is happening is:
* the page we submit to the block layer has some cache lines already
  associated with the kernel mapping.
* the IDE driver uses PIO to read from CF and hits these cache lines
  making them dirty.
* the page is then mapped into userspace via a page fault.
* userspace reads the page.  Because some of the data is sitting in
  the cache corresponding with the kernel mapping of the page,
  userspace doesn't see the up to date data until later.

> It also seems odd to me that this requires fixes at the block driver
> level. I'd have thought this would get resolved at the page cache
> level, or fs level at absolute lowest.

If you do it there, you hurt DMA performance.  The DMA model ensures
cache coherency.  The IDE PIO IO model does not.

Basically, the problem stems from IDE PIO IO not providing the same
cache guarantees as other block device drivers do. 

Re: Execute from CF causes segmentation faults

[Tejun Heo]

James Steward wrote:
> G'day all,
> 
> The message below is a response from Russell King on the arm 
> linux mailing lists regarding a problem I encountered, and as
> it turns out has been encountered before.  I'm not sure if anyone
> from the IDE development group is working on this but as it is
> more ide specific it may be best handled by some clever guru 
> from this mail list. 
> 
> The symptom I see is that executing code from a CF disk causes
> SEGVs and other nasties.  The workaround is to add a kernel
> command line arg "cachepolicy=writethrough".  This is seen as a
> bit of a bandaid and not a "good" solution.
> 
> Could someone please take a look?
> 
> Regards,
> James.
> 
> 
> On Thu, Dec 01, 2005 at 01:44:59PM +1300, Charles Manning wrote:
> 
>>Something that confuses me lightly is why this problem should exist at
>>all.
>>
>>It seems that the problem is caused by stale data in the cache.
>>Surely cache data should never be stale (ie. Anything in the cache
>>should be current).
> 
> 
> No.  I think what is happening is:
> * the page we submit to the block layer has some cache lines already
>   associated with the kernel mapping.
> * the IDE driver uses PIO to read from CF and hits these cache lines
>   making them dirty.
> * the page is then mapped into userspace via a page fault.
> * userspace reads the page.  Because some of the data is sitting in
>   the cache corresponding with the kernel mapping of the page,
>   userspace doesn't see the up to date data until later.
> 
> 
>>It also seems odd to me that this requires fixes at the block driver
>>level. I'd have thought this would get resolved at the page cache
>>level, or fs level at absolute lowest.
> 
> 
> If you do it there, you hurt DMA performance.  The DMA model ensures
> cache coherency.  The IDE PIO IO model does not.
> 
> Basically, the problem stems from IDE PIO IO not providing the same
> cache guarantees as other block device drivers do. 

[CC'ing Bartlomiej and Russell King]

Hello, all.

kmap/kunmap are to PIO what dma_map/unmap are to DMA.  dma_map/unmap do
the following two things.

1. make the pages accessible to the DMA'ing device
2. take care of cache consistency

kmap/unmap currently performs the counterpart of #1.

1. make the pages accessible to the accessing device (CPU)

So, adding cache consistency handling to kmap/unmap seems more logical
solution than handling cache consistency in all places where PIO occurs
(there are quite a few and some are buried pretty deep).

Hmmm... I'm not really sure whether the places where cache consistency
handling is needed coincides with kmap/unmap.  Would the said scheme
unnecessrily perform cache consistency operations in some places and
thus degrade performance?

Thanks.

-- 
tejun

Re: Execute from CF causes segmentation faults

[Bartlomiej Zolnierkiewicz]

Hi,

On 12/20/05, Tejun Heo <htejun@gmail.com> wrote:
> James Steward wrote:
> > G'day all,
> >
> > The message below is a response from Russell King on the arm
> > linux mailing lists regarding a problem I encountered, and as
> > it turns out has been encountered before.  I'm not sure if anyone
> > from the IDE development group is working on this but as it is
> > more ide specific it may be best handled by some clever guru
> > from this mail list.
> >
> > The symptom I see is that executing code from a CF disk causes
> > SEGVs and other nasties.  The workaround is to add a kernel
> > command line arg "cachepolicy=writethrough".  This is seen as a
> > bit of a bandaid and not a "good" solution.
> >
> > Could someone please take a look?
> >
> > Regards,
> > James.
> >
> >
> > On Thu, Dec 01, 2005 at 01:44:59PM +1300, Charles Manning wrote:
> >
> >>Something that confuses me lightly is why this problem should exist at
> >>all.
> >>
> >>It seems that the problem is caused by stale data in the cache.
> >>Surely cache data should never be stale (ie. Anything in the cache
> >>should be current).
> >
> >
> > No.  I think what is happening is:
> > * the page we submit to the block layer has some cache lines already
> >   associated with the kernel mapping.

I'm not familiar with cache coherency issues so please help me...

I see that flush_dcache_page() call in filemap.c:do_generic_mapping_read()
takes care of user virtual mapping but there is no code to take care of kernel
virtual mapping?  But how could this happen in the first place that some
cache lines are associated with the kernel mapping?

> > * the IDE driver uses PIO to read from CF and hits these cache lines
> >   making them dirty.
> > * the page is then mapped into userspace via a page fault.
> > * userspace reads the page.  Because some of the data is sitting in
> >   the cache corresponding with the kernel mapping of the page,
> >   userspace doesn't see the up to date data until later.
> >
> >
> >>It also seems odd to me that this requires fixes at the block driver
> >>level. I'd have thought this would get resolved at the page cache
> >>level, or fs level at absolute lowest.
> >
> >
> > If you do it there, you hurt DMA performance.  The DMA model ensures
> > cache coherency.  The IDE PIO IO model does not.
> >
> > Basically, the problem stems from IDE PIO IO not providing the same
> > cache guarantees as other block device drivers do.

Could you point me at such block device drivers
or explain how it should be done properly?

I tried looking at mmc, libata and scsi drivers and I couldn't find
any driver which actually does provide such guarantees...

> [CC'ing Bartlomiej and Russell King]
>
> Hello, all.
>
> kmap/kunmap are to PIO what dma_map/unmap are to DMA.  dma_map/unmap do
> the following two things.
>
> 1. make the pages accessible to the DMA'ing device
> 2. take care of cache consistency
>
> kmap/unmap currently performs the counterpart of #1.
>
> 1. make the pages accessible to the accessing device (CPU)
>
> So, adding cache consistency handling to kmap/unmap seems more logical
> solution than handling cache consistency in all places where PIO occurs
> (there are quite a few and some are buried pretty deep).
>
> Hmmm... I'm not really sure whether the places where cache consistency
> handling is needed coincides with kmap/unmap.  Would the said scheme
> unnecessrily perform cache consistency operations in some places and
> thus degrade performance?

>From what I understand:
* DMA API provides coherency w.r.t. to write buffers not cache aliasing issues
* k(un)map() is also used for addresses which have only kernel mappings

?

Thanks,
Bartlomiej

Re: Execute from CF causes segmentation faults

[Tejun Heo]

On Tue, Dec 20, 2005 at 05:23:45PM +0100, Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> On 12/20/05, Tejun Heo <htejun@gmail.com> wrote:
> > >
> > > No.  I think what is happening is:
> > > * the page we submit to the block layer has some cache lines already
> > >   associated with the kernel mapping.
> 
> I'm not familiar with cache coherency issues so please help me...
> 
> I see that flush_dcache_page() call in filemap.c:do_generic_mapping_read()
> takes care of user virtual mapping but there is no code to take care of kernel
> virtual mapping?  But how could this happen in the first place that some
> cache lines are associated with the kernel mapping?

When filling page cache, cache coherency is driver's reponsibility,
which is logical as in most cases only the driver knows what's
necessary to achieve coherency.  This usually doesn't require any
extra care as dma mapping/unmapping functions automatically take care
of coherency.

However, when performing PIO, kmap/unmap are used to access the pages
but they don't do any automatic cache flushing operations.  I've
grepped through the source and it seems that it's the caller's
responsibility to explicitly flush kernel mapping's cache before
unmapping when modifying possibly-user-mapped pages.

> > > * the IDE driver uses PIO to read from CF and hits these cache lines
> > >   making them dirty.
> > > * the page is then mapped into userspace via a page fault.
> > > * userspace reads the page.  Because some of the data is sitting in
> > >   the cache corresponding with the kernel mapping of the page,
> > >   userspace doesn't see the up to date data until later.
> > >
> > >
> > >>It also seems odd to me that this requires fixes at the block driver
> > >>level. I'd have thought this would get resolved at the page cache
> > >>level, or fs level at absolute lowest.
> > >
> > >
> > > If you do it there, you hurt DMA performance.  The DMA model ensures
> > > cache coherency.  The IDE PIO IO model does not.
> > >
> > > Basically, the problem stems from IDE PIO IO not providing the same
> > > cache guarantees as other block device drivers do.
> 
> Could you point me at such block device drivers
> or explain how it should be done properly?
> 
> I tried looking at mmc, libata and scsi drivers and I couldn't find
> any driver which actually does provide such guarantees...

Yeap, they all seem to need fixing.

> 
> > [CC'ing Bartlomiej and Russell King]
> >
> > Hello, all.
> >
> > kmap/kunmap are to PIO what dma_map/unmap are to DMA.  dma_map/unmap do
> > the following two things.
> >
> > 1. make the pages accessible to the DMA'ing device
> > 2. take care of cache consistency
> >
> > kmap/unmap currently performs the counterpart of #1.
> >
> > 1. make the pages accessible to the accessing device (CPU)
> >
> > So, adding cache consistency handling to kmap/unmap seems more logical
> > solution than handling cache consistency in all places where PIO occurs
> > (there are quite a few and some are buried pretty deep).
> >
> > Hmmm... I'm not really sure whether the places where cache consistency
> > handling is needed coincides with kmap/unmap.  Would the said scheme
> > unnecessrily perform cache consistency operations in some places and
> > thus degrade performance?
> 
> From what I understand:
> * DMA API provides coherency w.r.t. to write buffers not cache aliasing issues

DMA API provides all needed coherency.  It's responsible for evicting
all related cpu cachelines before starting DMA.

> * k(un)map() is also used for addresses which have only kernel mappings

Yes, it seems that the correct way to fix this is adding
flush_dcache_page prior to kunmap in all drivers which perform PIO -
IDE, libata, SCSI, mmc...

I originally thought that performing some extra flush_dcach_page
should be okay considering most mainstream architectures have
physically indexed cache.  But as this kind of problems are already
handled by manually performing flushe in other places, it seems that
block drivers should do the same.

I'll follow up with patches soon.

-- 
tejun

[PATCH] ide: add dcache flushing after PIO

[Tejun Heo]

Block drivers are responsible for cache coherency before and after IO.
When using DMA, DMA API takes care of it but drivers should do manual
flushing with PIO.

Signed-off-by: Tejun Heo <htejun@gmail.com>

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 9e293c8..4fb0d9d 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -617,6 +617,7 @@ static void idefloppy_input_buffers (ide
 
 			data = bvec_kmap_irq(bvec, &flags);
 			drive->hwif->atapi_input_bytes(drive, data, count);
+			flush_dcache_page(bvec->page);
 			bvec_kunmap_irq(data, &flags);
 
 			bcount -= count;
@@ -650,6 +651,7 @@ static void idefloppy_output_buffers (id
 			count = min(bvec->bv_len, bcount);
 
 			data = bvec_kmap_irq(bvec, &flags);
+			flush_dcache_page(bvec->page);
 			drive->hwif->atapi_output_bytes(drive, data, count);
 			bvec_kunmap_irq(data, &flags);
 
diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
index 62ebefd..78256d9 100644
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -278,6 +278,8 @@ static void ide_pio_sector(ide_drive_t *
 	local_irq_save(flags);
 #endif
 	buf = kmap_atomic(page, KM_BIO_SRC_IRQ) + offset;
+	if (write)
+		flush_dcache_page(page);
 
 	hwif->nleft--;
 	hwif->cursg_ofs++;
@@ -293,6 +295,8 @@ static void ide_pio_sector(ide_drive_t *
 	else
 		taskfile_input_data(drive, buf, SECTOR_WORDS);
 
+	if (!write)
+		flush_dcache_page(page);
 	kunmap_atomic(buf, KM_BIO_SRC_IRQ);
 #ifdef CONFIG_HIGHMEM
 	local_irq_restore(flags);

Re: [PATCH] ide: add dcache flushing after PIO

[Russell King]

On Wed, Dec 21, 2005 at 11:00:22PM +0900, Tejun Heo wrote:
> Block drivers are responsible for cache coherency before and after IO.
> When using DMA, DMA API takes care of it but drivers should do manual
> flushing with PIO.

Are you sure you need the ones in the write path?  No other driver
seems to do this.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] ide: add dcache flushing after PIO

[Tejun Heo]

Russell King wrote:
> On Wed, Dec 21, 2005 at 11:00:22PM +0900, Tejun Heo wrote:
> 
>>Block drivers are responsible for cache coherency before and after IO.
>>When using DMA, DMA API takes care of it but drivers should do manual
>>flushing with PIO.
> 
> 
> Are you sure you need the ones in the write path?  No other driver
> seems to do this.
> 

Unfortunately, I'm not.  I was trying to be on the safe side.

Are the page caches guaranteed to have no dirty alias on entry to block 
layer for write?  I can see that aliases are handled for read(2)s and 
write(2)s but I don't know much about mmap writebacks.

Another question is whether or not it's guaranteed that there's no 
user-mapped dirty cachelines on entry to block layer for read.  DMA API 
clears all cpu caches before IO and, as DMA IO doesn't touch any cpu 
caches, it doesn't do anything after IO.  The previous patch adds a 
flush_dcache_page after IO which makes sure that the kernel cache line 
is gone, but it doesn't do anything to make sure that there's no dirty 
user-mapped cachelines hanging around before IO.

I couldn't find an exemplary driver doing this kind of things with page 
caches.  Most other flush_dcache_page usages I looked at didn't deal 
with user mapped page caches.

-- 
tejun

Re: Execute from CF causes segmentation faults

[Russell King]

On Tue, Dec 20, 2005 at 05:23:45PM +0100, Bartlomiej Zolnierkiewicz wrote:
> I'm not familiar with cache coherency issues so please help me...

I'll do my best, but I'm not very familiar with the issues, even
less familiar with the Linux MM internals.  So what follows is a
fairly vague explaination.

Maybe some of the Linux MM gurus can fill in some of the bits as
well.

> I see that flush_dcache_page() call in filemap.c:do_generic_mapping_read()
> takes care of user virtual mapping but there is no code to take care
> of kernel virtual mapping?  But how could this happen in the first place
> that some cache lines are associated with the kernel mapping?

What does do_generic_mapping_read do and where's it called from?
It looks like it's called in response to the read() syscall.  If
that's so, that's not directly a problem for cache coherency.

When a driver using PIO reads data into the page cache, it uses the
CPU write instructions.  In the process of doing this, we may hit
some already allocated cache lines corresponding with this page, or
in the case of a write-allocating cache, we may allocate cache lines
in doing this.

When the read() syscall reads data from the page into userspace,
it reads data out of the page using the same address as the PIO
driver used to write data there.  Hence, there is no coherency
problem.

The problem comes when you have a mmap() mapping of a file.  For
this you want to look at filemap_nopage().  In this case, a PIO
driver reads data into the page cache as described above.

However, that page may eventually be mapped into user space.  Because
it will be mapped at a different address from the kernel mapping, you
can have cache aliasing occuring depending on the type of CPU cache.

> Could you point me at such block device drivers
> or explain how it should be done properly?

ramdisks and md are the only two at present.

> I tried looking at mmc, libata and scsi drivers and I couldn't find
> any driver which actually does provide such guarantees...

I've just been trying to sort out the mmc mmci driver, but the
hardware I've been trying to test it out on appears to have decided
to throw an instability fit today.

scsi drivers aren't popular on ARM, so if there's any bugs lurking
in there, they'll go unnoticed.  libata?  pass.

> >From what I understand:
> * DMA API provides coherency w.r.t. to write buffers not cache
>   aliasing issues

The DMA API solves the coherency issues between the device doing DMA
and the kernel mapping of the data.

For read, the guarantee it provides is that any data read from the device
will be placed in physical RAM and will be visible to the CPU via the
kernel mapping once the DMA has been unmapped.

Conversely, for write, the guarantee it provides is that any data placed
by the CPU into the kernel mapping will be visible to the DMA device
for writing.

> * k(un)map() is also used for addresses which have only kernel mappings

k*map have nothing to do with cache coherency.  Their only function is
to deal with highmem pages.

All that said, I don't think I'm the best person to explain these
issues to you - I only have a very superficial understanding of the
Linux MM.  I'm sure there's others with a more detailed understanding
who would be able to convey the requirements far better than my
attempt above.

Nevertheless, I hope you find the above useful in trying to understand
the problem.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] ide: add dcache flushing after PIO

[Bartlomiej Zolnierkiewicz]

On 12/21/05, Tejun Heo <htejun@gmail.com> wrote:
> Russell King wrote:
> > On Wed, Dec 21, 2005 at 11:00:22PM +0900, Tejun Heo wrote:
> >
> >>Block drivers are responsible for cache coherency before and after IO.
> >>When using DMA, DMA API takes care of it but drivers should do manual
> >>flushing with PIO.
> >
> >
> > Are you sure you need the ones in the write path?  No other driver
> > seems to do this.
> >
>
> Unfortunately, I'm not.  I was trying to be on the safe side.
>
> Are the page caches guaranteed to have no dirty alias on entry to block
> layer for write?  I can see that aliases are handled for read(2)s and
> write(2)s but I don't know much about mmap writebacks.
>
> Another question is whether or not it's guaranteed that there's no
> user-mapped dirty cachelines on entry to block layer for read.  DMA API

It seems to be guaranteed for read() and write() but not for mmap().

> clears all cpu caches before IO and, as DMA IO doesn't touch any cpu
> caches, it doesn't do anything after IO.  The previous patch adds a
> flush_dcache_page after IO which makes sure that the kernel cache line
> is gone, but it doesn't do anything to make sure that there's no dirty
> user-mapped cachelines hanging around before IO.
>
> I couldn't find an exemplary driver doing this kind of things with page
> caches.  Most other flush_dcache_page usages I looked at didn't deal
> with user mapped page caches.

After reading excellent explanation by rmk I think that it should be
fixed at VM layer (filemap_nopage() perhaps).

Could you move the discussion to LKML (with some generic "subject")
so we can get some help from MM hackers?

Bartlomiej

Re: [PATCH] ide: add dcache flushing after PIO

[Tejun Heo]

Bartlomiej Zolnierkiewicz wrote:
> On 12/21/05, Tejun Heo <htejun@gmail.com> wrote:
>> Russell King wrote:
>>> On Wed, Dec 21, 2005 at 11:00:22PM +0900, Tejun Heo wrote:
>>>
>>>> Block drivers are responsible for cache coherency before and after IO.
>>>> When using DMA, DMA API takes care of it but drivers should do manual
>>>> flushing with PIO.
>>>
>>> Are you sure you need the ones in the write path?  No other driver
>>> seems to do this.
>>>
>> Unfortunately, I'm not.  I was trying to be on the safe side.
>>
>> Are the page caches guaranteed to have no dirty alias on entry to block
>> layer for write?  I can see that aliases are handled for read(2)s and
>> write(2)s but I don't know much about mmap writebacks.
>>
>> Another question is whether or not it's guaranteed that there's no
>> user-mapped dirty cachelines on entry to block layer for read.  DMA API
> 
> It seems to be guaranteed for read() and write() but not for mmap().
> 
>> clears all cpu caches before IO and, as DMA IO doesn't touch any cpu
>> caches, it doesn't do anything after IO.  The previous patch adds a
>> flush_dcache_page after IO which makes sure that the kernel cache line
>> is gone, but it doesn't do anything to make sure that there's no dirty
>> user-mapped cachelines hanging around before IO.
>>
>> I couldn't find an exemplary driver doing this kind of things with page
>> caches.  Most other flush_dcache_page usages I looked at didn't deal
>> with user mapped page caches.
> 
> After reading excellent explanation by rmk I think that it should be
> fixed at VM layer (filemap_nopage() perhaps).
> 
> Could you move the discussion to LKML (with some generic "subject")
> so we can get some help from MM hackers?
> 
> Bartlomiej

Hi, Bartlomiej.

I'll post a new message to LKML first thing tomorrow.  :-)

-- 
tejun

Re: [PATCH] ide: add dcache flushing after PIO

[Russell King]

On Wed, Dec 21, 2005 at 04:57:05PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On 12/21/05, Tejun Heo <htejun@gmail.com> wrote:
> > clears all cpu caches before IO and, as DMA IO doesn't touch any cpu
> > caches, it doesn't do anything after IO.  The previous patch adds a
> > flush_dcache_page after IO which makes sure that the kernel cache line
> > is gone, but it doesn't do anything to make sure that there's no dirty
> > user-mapped cachelines hanging around before IO.
> >
> > I couldn't find an exemplary driver doing this kind of things with page
> > caches.  Most other flush_dcache_page usages I looked at didn't deal
> > with user mapped page caches.
> 
> After reading excellent explanation by rmk I think that it should be
> fixed at VM layer (filemap_nopage() perhaps).

DaveM vetoed that with good reason - the VM layer doesn't know whether
the driver is doing PIO or not.  If it is doing PIO, it needs the
cache flush.  If it's doing DMA, the cache flush is entirely a
performance bottleneck.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] ide: add dcache flushing after PIO

[Russell King]

On Wed, Dec 21, 2005 at 05:54:16PM +0000, Russell King wrote:
> On Wed, Dec 21, 2005 at 04:57:05PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > On 12/21/05, Tejun Heo <htejun@gmail.com> wrote:
> > > clears all cpu caches before IO and, as DMA IO doesn't touch any cpu
> > > caches, it doesn't do anything after IO.  The previous patch adds a
> > > flush_dcache_page after IO which makes sure that the kernel cache line
> > > is gone, but it doesn't do anything to make sure that there's no dirty
> > > user-mapped cachelines hanging around before IO.
> > >
> > > I couldn't find an exemplary driver doing this kind of things with page
> > > caches.  Most other flush_dcache_page usages I looked at didn't deal
> > > with user mapped page caches.
> > 
> > After reading excellent explanation by rmk I think that it should be
> > fixed at VM layer (filemap_nopage() perhaps).
> 
> DaveM vetoed that with good reason - the VM layer doesn't know whether
> the driver is doing PIO or not.  If it is doing PIO, it needs the
> cache flush.  If it's doing DMA, the cache flush is entirely a
> performance bottleneck.

Bart,

Did you miss my message, or have you decided to pay no further
attention to this issue?

If the latter, I will mark IDE with a dependency of BROKEN || !ARM
until it's fixed.  It's not fair on folk to lead them to think that
PIO-based IDE might work for them when the subsystem is technically
broken on ARM architectures.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] ide: add dcache flushing after PIO

[Bartlomiej Zolnierkiewicz]

On 1/7/06, Russell King <rmk@arm.linux.org.uk> wrote:
> On Wed, Dec 21, 2005 at 05:54:16PM +0000, Russell King wrote:
> > On Wed, Dec 21, 2005 at 04:57:05PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > > On 12/21/05, Tejun Heo <htejun@gmail.com> wrote:
> > > > clears all cpu caches before IO and, as DMA IO doesn't touch any cpu
> > > > caches, it doesn't do anything after IO.  The previous patch adds a
> > > > flush_dcache_page after IO which makes sure that the kernel cache line
> > > > is gone, but it doesn't do anything to make sure that there's no dirty
> > > > user-mapped cachelines hanging around before IO.
> > > >
> > > > I couldn't find an exemplary driver doing this kind of things with page
> > > > caches.  Most other flush_dcache_page usages I looked at didn't deal
> > > > with user mapped page caches.
> > >
> > > After reading excellent explanation by rmk I think that it should be
> > > fixed at VM layer (filemap_nopage() perhaps).
> >
> > DaveM vetoed that with good reason - the VM layer doesn't know whether
> > the driver is doing PIO or not.  If it is doing PIO, it needs the
> > cache flush.  If it's doing DMA, the cache flush is entirely a
> > performance bottleneck.
>
> Bart,
>
> Did you miss my message, or have you decided to pay no further
> attention to this issue?

No, I'm waiting for ARM folks to give Tejun and me some feedback.
So far there was none and nobody seemed to care to test/comment
this patch ("works fine" is enough).  As soon as it happens I'll ack the
patch and forward it to akpm.

> If the latter, I will mark IDE with a dependency of BROKEN || !ARM
> until it's fixed.  It's not fair on folk to lead them to think that
> PIO-based IDE might work for them when the subsystem is technically
> broken on ARM architectures.

What about testing the patch instead?

Bartlomiej

Re: [PATCH] ide: add dcache flushing after PIO

[Russell King]

On Sat, Jan 07, 2006 at 09:17:11PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On 1/7/06, Russell King <rmk@arm.linux.org.uk> wrote:
> > On Wed, Dec 21, 2005 at 05:54:16PM +0000, Russell King wrote:
> > > DaveM vetoed that with good reason - the VM layer doesn't know whether
> > > the driver is doing PIO or not.  If it is doing PIO, it needs the
> > > cache flush.  If it's doing DMA, the cache flush is entirely a
> > > performance bottleneck.
> >
> > Bart,
> >
> > Did you miss my message, or have you decided to pay no further
> > attention to this issue?
> 
> No, I'm waiting for ARM folks to give Tejun and me some feedback.

We have, and I think it's been soo long that you're now confused.
See the entire thread on marc:

http://marc.theaimsgroup.com/?t=113517374400003&r=1&w=2

Yes, Tejun produced a patch and I queried whether it was going to
far.  There was a bit of discussion which resulted in a comment from
you saying:

| After reading excellent explanation by rmk I think that it should be
| fixed at VM layer (filemap_nopage() perhaps).

To which I replied:

| DaveM vetoed that with good reason - the VM layer doesn't know whether
| the driver is doing PIO or not.  If it is doing PIO, it needs the
| cache flush.  If it's doing DMA, the cache flush is entirely a
| performance bottleneck.

That's where we stand now - I'm waiting for some progress.

> So far there was none and nobody seemed to care to test/comment
> this patch ("works fine" is enough).

Odd, as demonstrated by the linux-ide archives above I've already
commented on it.

> What about testing the patch instead?

Since there hasn't been a follow-up patch, there isn't anything to
test.  Please can we have a revised patch so folk _can_ do some
testing?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] ide: add dcache flushing after PIO

[Bartlomiej Zolnierkiewicz]

On 1/7/06, Russell King <rmk@arm.linux.org.uk> wrote:
> On Sat, Jan 07, 2006 at 09:17:11PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > On 1/7/06, Russell King <rmk@arm.linux.org.uk> wrote:
> > > On Wed, Dec 21, 2005 at 05:54:16PM +0000, Russell King wrote:
> > > > DaveM vetoed that with good reason - the VM layer doesn't know whether
> > > > the driver is doing PIO or not.  If it is doing PIO, it needs the
> > > > cache flush.  If it's doing DMA, the cache flush is entirely a
> > > > performance bottleneck.
> > >
> > > Bart,
> > >
> > > Did you miss my message, or have you decided to pay no further
> > > attention to this issue?
> >
> > No, I'm waiting for ARM folks to give Tejun and me some feedback.
>
> We have, and I think it's been soo long that you're now confused.
> See the entire thread on marc:
>
> http://marc.theaimsgroup.com/?t=113517374400003&r=1&w=2
>
> Yes, Tejun produced a patch and I queried whether it was going to
> far.  There was a bit of discussion which resulted in a comment from
> you saying:
>
> | After reading excellent explanation by rmk I think that it should be
> | fixed at VM layer (filemap_nopage() perhaps).
>
> To which I replied:
>
> | DaveM vetoed that with good reason - the VM layer doesn't know whether
> | the driver is doing PIO or not.  If it is doing PIO, it needs the
> | cache flush.  If it's doing DMA, the cache flush is entirely a
> | performance bottleneck.
>
> That's where we stand now - I'm waiting for some progress.

Tejun posted RFC to lkml which got zero replies so I thought
that I will apply his patch if it *works*.

We can fix the problem in IDE locally - fine with me - but I want
to know if we are actually *really* fixing the problem or just adding
more confusion to IDE subsystem.

> > So far there was none and nobody seemed to care to test/comment
> > this patch ("works fine" is enough).
>
> Odd, as demonstrated by the linux-ide archives above I've already
> commented on it.

OK, we have discussed the way to fix the problem.

Still nothing was said whether this patch *actually* works or not.

> > What about testing the patch instead?
>
> Since there hasn't been a follow-up patch, there isn't anything to
> test.  Please can we have a revised patch so folk _can_ do some
> testing?

Test original Tejun's patch then.

I need some help from you and other ARM folks to fix this properly,
I'm just a stupid IDE developer... :-)

Bartlomiej

Re: [PATCH] ide: add dcache flushing after PIO

[james]

On Sat, 2006-01-07 at 23:41 +0100, Bartlomiej Zolnierkiewicz wrote:
> On 1/7/06, Russell King <rmk@arm.linux.org.uk> wrote:
> > On Sat, Jan 07, 2006 at 09:17:11PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > > On 1/7/06, Russell King <rmk@arm.linux.org.uk> wrote:
> > > > On Wed, Dec 21, 2005 at 05:54:16PM +0000, Russell King wrote:
> > > > > DaveM vetoed that with good reason - the VM layer doesn't know whether
> > > > > the driver is doing PIO or not.  If it is doing PIO, it needs the
> > > > > cache flush.  If it's doing DMA, the cache flush is entirely a
> > > > > performance bottleneck.
> > > >
> > > > Bart,
> > > >
> > > > Did you miss my message, or have you decided to pay no further
> > > > attention to this issue?
> > >
> > > No, I'm waiting for ARM folks to give Tejun and me some feedback.
> >
> > We have, and I think it's been soo long that you're now confused.
> > See the entire thread on marc:
> >
> > http://marc.theaimsgroup.com/?t=113517374400003&r=1&w=2
> >
> > Yes, Tejun produced a patch and I queried whether it was going to
> > far.  There was a bit of discussion which resulted in a comment from
> > you saying:
> >
> > | After reading excellent explanation by rmk I think that it should be
> > | fixed at VM layer (filemap_nopage() perhaps).
> >
> > To which I replied:
> >
> > | DaveM vetoed that with good reason - the VM layer doesn't know whether
> > | the driver is doing PIO or not.  If it is doing PIO, it needs the
> > | cache flush.  If it's doing DMA, the cache flush is entirely a
> > | performance bottleneck.
> >
> > That's where we stand now - I'm waiting for some progress.
> 
> Tejun posted RFC to lkml which got zero replies so I thought
> that I will apply his patch if it *works*.
> 
> We can fix the problem in IDE locally - fine with me - but I want
> to know if we are actually *really* fixing the problem or just adding
> more confusion to IDE subsystem.
> 
> > > So far there was none and nobody seemed to care to test/comment
> > > this patch ("works fine" is enough).
> >
> > Odd, as demonstrated by the linux-ide archives above I've already
> > commented on it.
> 
> OK, we have discussed the way to fix the problem.
> 
> Still nothing was said whether this patch *actually* works or not.
> 
> > > What about testing the patch instead?
> >
> > Since there hasn't been a follow-up patch, there isn't anything to
> > test.  Please can we have a revised patch so folk _can_ do some
> > testing?
> 
> Test original Tejun's patch then.
> 
> I need some help from you and other ARM folks to fix this properly,
> I'm just a stupid IDE developer... :-)

I'm happy to see if it works.  I'm running 2.6.13.2 so if it applies
cleanly...

Any hints on what to look for?

I have the cachepolicy=writethrough kernel arg set at the mo to work
around the problem - I'll not set that see if I can run apps from the CF
I guess.

I saw segvs w/o the cachepolicy=... set.  So is there anything else I
need to do?  What other tests can I do and feedback can I give?

Please advise.

Russell, I note the comments by you and others above on Tejun's patch,
is there anything you (or others) think I should look for in testing
this?


BTW - thanks for keeping this going.

Happy New Year and warm regards,
James.

43 degrees C in Melbourne, Australia New Years Eve!

> 
> Bartlomiej
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ide: add dcache flushing after PIO

[Russell King]

On Sat, Jan 07, 2006 at 11:41:53PM +0100, Bartlomiej Zolnierkiewicz wrote:
> Tejun posted RFC to lkml which got zero replies so I thought
> that I will apply his patch if it *works*.

Which appears to have been missed.

> > > So far there was none and nobody seemed to care to test/comment
> > > this patch ("works fine" is enough).
> >
> > Odd, as demonstrated by the linux-ide archives above I've already
> > commented on it.
> 
> OK, we have discussed the way to fix the problem.
> 
> Still nothing was said whether this patch *actually* works or not.

That's because I've already suggested that the patch is over-complex
and I'm waiting for an updated patch.  I'm not going to ask folk to
test one patch which I know is doing far too much, only to have to
ask them to test another patch later on.

IOW, I'm not going to waste folk's time with a patch I know isn't
suitable.

> Test original Tejun's patch then.

I can't - I'm not able to reproduce the problem due to the systems
I have available to me.

The way forward is an updated patch - Tejun's original patch updated
with my comments.  Once that's available, folk can test a patch which
stands a chance of making it into mainline.


Let me make my position clear: I don't care about this problem myself
because I don't see it.  That's why it's hung around since 2.4 times.
My only involvement here is due to (a) other folk caring about it and
(b) the lack of knowledge to get it fixed.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] ide: add dcache flushing after PIO

[Tejun Heo]

Russell King wrote:
> On Sat, Jan 07, 2006 at 11:41:53PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
>>Tejun posted RFC to lkml which got zero replies so I thought
>>that I will apply his patch if it *works*.
> 
> 
> Which appears to have been missed.
> 
> 
>>>>So far there was none and nobody seemed to care to test/comment
>>>>this patch ("works fine" is enough).
>>>
>>>Odd, as demonstrated by the linux-ide archives above I've already
>>>commented on it.
>>
>>OK, we have discussed the way to fix the problem.
>>
>>Still nothing was said whether this patch *actually* works or not.
> 
> 
> That's because I've already suggested that the patch is over-complex
> and I'm waiting for an updated patch.  I'm not going to ask folk to
> test one patch which I know is doing far too much, only to have to
> ask them to test another patch later on.
> 
> IOW, I'm not going to waste folk's time with a patch I know isn't
> suitable.
> 
> 
>>Test original Tejun's patch then.
> 
> 
> I can't - I'm not able to reproduce the problem due to the systems
> I have available to me.
> 
> The way forward is an updated patch - Tejun's original patch updated
> with my comments.  Once that's available, folk can test a patch which
> stands a chance of making it into mainline.
> 
> 
> Let me make my position clear: I don't care about this problem myself
> because I don't see it.  That's why it's hung around since 2.4 times.
> My only involvement here is due to (a) other folk caring about it and
> (b) the lack of knowledge to get it fixed.
> 

Hi, Russel & Bartlomiej.

As the message to the lkml didn't get any replies, I kind of forgot 
about it.  And after Russel's explanation, I don't think my first 
approach is good.  I will make another patch according to what I've 
proposed in the message to lkml.  I'm currently burried under libata EH 
related stuff, but I think (or hope) it will be sorted out in a few days.

So, see you guys in a few days.  Thanks.

-- 
tejun