| Message ID | 20131115220529.GA3160@ls3530.box (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote: > When a user page mapping is released via kunmap*() functions, the D-cache needs > to be flushed via flush_dcache_page() to avoid D-cache aliasing issues. > > This patch fixes aio on the parisc platform (and probably others). This should be flush_kernel_dcache_page(). flush_dcache_page() is for full coherency but for unmap, we know the page was coherent going in and may have been modified by the kernel, so only the kernel view needs to be sync'd. Technically, by the kernel API, the flush should be done *before* unmapping. This would have mattered on parisc until we did flush via tmpalias which means we no-longer care if the mapping for the flush exists or not because we always recreate it via the tmpalias pages. James > Signed-off-by: Helge Deller <deller@gmx.de> > To: Benjamin LaHaise <bcrl@kvack.org> > To: linux-aio@kvack.org > Cc: stable@vger.kernel.org # 3.12+ > > diff --git a/fs/aio.c b/fs/aio.c > index 823efcb..2181821 100644 > --- a/fs/aio.c > +++ b/fs/aio.c > @@ -514,6 +514,7 @@ static void free_ioctx(struct work_struct *work) > atomic_add(avail, &ctx->reqs_available); > ring->head = ring->tail; > kunmap_atomic(ring); > + flush_dcache_page(ctx->ring_pages[0]); > > if (atomic_read(&ctx->reqs_available) >= ctx->nr_events - 1) > break; > @@ -568,6 +569,7 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm) > ring = kmap_atomic(ctx->ring_pages[0]); > ring->id = ctx->id; > kunmap_atomic(ring); > + flush_dcache_page(ctx->ring_pages[0]); > return 0; > } > > @@ -1032,6 +1034,7 @@ static long aio_read_events_ring(struct kioctx *ctx, > head = ring->head; > tail = ring->tail; > kunmap_atomic(ring); > + flush_dcache_page(ctx->ring_pages[0]); > > pr_debug("h%u t%u m%u\n", head, tail, ctx->nr_events); > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-parisc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote: > On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote: > > When a user page mapping is released via kunmap*() functions, the D-cache needs > > to be flushed via flush_dcache_page() to avoid D-cache aliasing issues. > > > > This patch fixes aio on the parisc platform (and probably others). > > This should be flush_kernel_dcache_page(). flush_dcache_page() is for > full coherency but for unmap, we know the page was coherent going in and > may have been modified by the kernel, so only the kernel view needs to > be sync'd. Technically, by the kernel API, the flush should be done > *before* unmapping. This would have mattered on parisc until we did > flush via tmpalias which means we no-longer care if the mapping for the > flush exists or not because we always recreate it via the tmpalias > pages. On ARM, flush_kernel_dcache_page() actually assumes that the page is mapped. It avoids double flushing of highmem pages by not flushing in those cases where kunmap_atomic() already takes care of flushing. - Simon -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Nov 16, 2013 at 09:07:18PM +0100, Simon Baatz wrote: > On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote: > > On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote: > > > When a user page mapping is released via kunmap*() functions, the D-cache needs > > > to be flushed via flush_dcache_page() to avoid D-cache aliasing issues. > > > > > > This patch fixes aio on the parisc platform (and probably others). > > > > This should be flush_kernel_dcache_page(). flush_dcache_page() is for > > full coherency but for unmap, we know the page was coherent going in and > > may have been modified by the kernel, so only the kernel view needs to > > be sync'd. Technically, by the kernel API, the flush should be done > > *before* unmapping. This would have mattered on parisc until we did > > flush via tmpalias which means we no-longer care if the mapping for the > > flush exists or not because we always recreate it via the tmpalias > > pages. > > On ARM, flush_kernel_dcache_page() actually assumes that the page is > mapped. It avoids double flushing of highmem pages by not flushing > in those cases where kunmap_atomic() already takes care of flushing. Helge -- are you going to resubmit a version of this patch that makes the recommended change? -ben > > > - Simon
On Sat, 2013-11-16 at 21:07 +0100, Simon Baatz wrote: > On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote: > > On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote: > > > When a user page mapping is released via kunmap*() functions, the D-cache needs > > > to be flushed via flush_dcache_page() to avoid D-cache aliasing issues. > > > > > > This patch fixes aio on the parisc platform (and probably others). > > > > This should be flush_kernel_dcache_page(). flush_dcache_page() is for > > full coherency but for unmap, we know the page was coherent going in and > > may have been modified by the kernel, so only the kernel view needs to > > be sync'd. Technically, by the kernel API, the flush should be done > > *before* unmapping. This would have mattered on parisc until we did > > flush via tmpalias which means we no-longer care if the mapping for the > > flush exists or not because we always recreate it via the tmpalias > > pages. > > On ARM, flush_kernel_dcache_page() actually assumes that the page is > mapped. It avoids double flushing of highmem pages by not flushing > in those cases where kunmap_atomic() already takes care of flushing. On Parisc, kmap/kunmap is currently a nop. However, if we ever implemented highmem, we would also need to flush before we unmap, which is why the flush needs to go before the kunmap. I've got to say on all of this, we've implemented the most inane set of primitives. It would be much easier if kmap/kunmap just took care of all of this. kmap should bring userspace into coherence (because why else would we be kmapping) and kunmap should force coherence on the kernel address ... and then no-one would need to worry when to and when not to flush. James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16-Nov-13, at 5:06 PM, James Bottomley wrote: > On Sat, 2013-11-16 at 21:07 +0100, Simon Baatz wrote: >> On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote: >>> On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote: >>>> When a user page mapping is released via kunmap*() functions, the >>>> D-cache needs >>>> to be flushed via flush_dcache_page() to avoid D-cache aliasing >>>> issues. >>>> >>>> This patch fixes aio on the parisc platform (and probably others). >>> >>> This should be flush_kernel_dcache_page(). flush_dcache_page() is >>> for >>> full coherency but for unmap, we know the page was coherent going >>> in and >>> may have been modified by the kernel, so only the kernel view >>> needs to >>> be sync'd. Technically, by the kernel API, the flush should be done >>> *before* unmapping. This would have mattered on parisc until we did >>> flush via tmpalias which means we no-longer care if the mapping >>> for the >>> flush exists or not because we always recreate it via the tmpalias >>> pages. >> >> On ARM, flush_kernel_dcache_page() actually assumes that the page is >> mapped. It avoids double flushing of highmem pages by not flushing >> in those cases where kunmap_atomic() already takes care of flushing. > > On Parisc, kmap/kunmap is currently a nop. However, if we ever > implemented highmem, we would also need to flush before we unmap, > which > is why the flush needs to go before the kunmap. Not quite. On PA8800/PA8900, we currently do a flush in kunmap. I'm fairly certain from discussion with Helge that he saw this bug on a C8000 with PA8800 processor. In that case, adding a call to flush_kernel_dcache_page() just duplicates the call in kunmap(). Could be wrong though. The problem is the coherence with userspace. That's why flush_dcache_page() is called in several places in this file. I agree with you that this should be done before. > > I've got to say on all of this, we've implemented the most inane set > of > primitives. It would be much easier if kmap/kunmap just took care of > all of this. kmap should bring userspace into coherence (because why > else would we be kmapping) and kunmap should force coherence on the > kernel address ... and then no-one would need to worry when to and > when > not to flush. I'm working on adding a flush to our kmap code to bring userspace into coherence on PA8800/PA8900. It's ugly because it requires looping over vma's as we do in in flush_dcache_page(). Dave -- John David Anglin dave.anglin@bell.net -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2013-11-16 at 17:32 -0500, John David Anglin wrote: > On 16-Nov-13, at 5:06 PM, James Bottomley wrote: > > > On Sat, 2013-11-16 at 21:07 +0100, Simon Baatz wrote: > >> On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote: > >>> On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote: > >>>> When a user page mapping is released via kunmap*() functions, the > >>>> D-cache needs > >>>> to be flushed via flush_dcache_page() to avoid D-cache aliasing > >>>> issues. > >>>> > >>>> This patch fixes aio on the parisc platform (and probably others). > >>> > >>> This should be flush_kernel_dcache_page(). flush_dcache_page() is > >>> for > >>> full coherency but for unmap, we know the page was coherent going > >>> in and > >>> may have been modified by the kernel, so only the kernel view > >>> needs to > >>> be sync'd. Technically, by the kernel API, the flush should be done > >>> *before* unmapping. This would have mattered on parisc until we did > >>> flush via tmpalias which means we no-longer care if the mapping > >>> for the > >>> flush exists or not because we always recreate it via the tmpalias > >>> pages. > >> > >> On ARM, flush_kernel_dcache_page() actually assumes that the page is > >> mapped. It avoids double flushing of highmem pages by not flushing > >> in those cases where kunmap_atomic() already takes care of flushing. > > > > On Parisc, kmap/kunmap is currently a nop. However, if we ever > > implemented highmem, we would also need to flush before we unmap, > > which > > is why the flush needs to go before the kunmap. > > Not quite. On PA8800/PA8900, we currently do a flush in kunmap. > > I'm fairly certain from discussion with Helge that he saw this bug on > a C8000 > with PA8800 processor. In that case, adding a call to > flush_kernel_dcache_page() > just duplicates the call in kunmap(). Yes, if that's true it won't be the solution. > Could be wrong though. > > The problem is the coherence with userspace. That's why > flush_dcache_page() > is called in several places in this file. I agree with you that this > should be done > before. If we need coherence with userspace, we can't do it as we unmap because by then we've used the data in the kernel. We have to do it as we kmap, because that's when we're preparing to use the data. So the solution, if this is the problem, would be flush_dcache_page() just after kmap. James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16-Nov-13, at 5:37 PM, James Bottomley wrote: > On Sat, 2013-11-16 at 17:32 -0500, John David Anglin wrote: >> On 16-Nov-13, at 5:06 PM, James Bottomley wrote: >> >>> On Sat, 2013-11-16 at 21:07 +0100, Simon Baatz wrote: >>>> On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote: >>>>> On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote: >>>>>> When a user page mapping is released via kunmap*() functions, the >>>>>> D-cache needs >>>>>> to be flushed via flush_dcache_page() to avoid D-cache aliasing >>>>>> issues. >>>>>> >>>>>> This patch fixes aio on the parisc platform (and probably >>>>>> others). >>>>> >>>>> This should be flush_kernel_dcache_page(). flush_dcache_page() is >>>>> for >>>>> full coherency but for unmap, we know the page was coherent going >>>>> in and >>>>> may have been modified by the kernel, so only the kernel view >>>>> needs to >>>>> be sync'd. Technically, by the kernel API, the flush should be >>>>> done >>>>> *before* unmapping. This would have mattered on parisc until we >>>>> did >>>>> flush via tmpalias which means we no-longer care if the mapping >>>>> for the >>>>> flush exists or not because we always recreate it via the tmpalias >>>>> pages. >>>> >>>> On ARM, flush_kernel_dcache_page() actually assumes that the page >>>> is >>>> mapped. It avoids double flushing of highmem pages by not flushing >>>> in those cases where kunmap_atomic() already takes care of >>>> flushing. >>> >>> On Parisc, kmap/kunmap is currently a nop. However, if we ever >>> implemented highmem, we would also need to flush before we unmap, >>> which >>> is why the flush needs to go before the kunmap. >> >> Not quite. On PA8800/PA8900, we currently do a flush in kunmap. >> >> I'm fairly certain from discussion with Helge that he saw this bug on >> a C8000 >> with PA8800 processor. In that case, adding a call to >> flush_kernel_dcache_page() >> just duplicates the call in kunmap(). > > Yes, if that's true it won't be the solution. The calls may be needed for the two cases where the page is changed, but I don't know how to avoid double flush on PA8800/PA8900 without changing the primatives. Dave -- John David Anglin dave.anglin@bell.net -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/16/2013 09:09 PM, Benjamin LaHaise wrote: > On Sat, Nov 16, 2013 at 09:07:18PM +0100, Simon Baatz wrote: >> On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote: >>> On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote: >>>> When a user page mapping is released via kunmap*() functions, the D-cache needs >>>> to be flushed via flush_dcache_page() to avoid D-cache aliasing issues. >>>> >>>> This patch fixes aio on the parisc platform (and probably others). >>> >>> This should be flush_kernel_dcache_page(). flush_dcache_page() is for >>> full coherency but for unmap, we know the page was coherent going in and >>> may have been modified by the kernel, so only the kernel view needs to >>> be sync'd. Technically, by the kernel API, the flush should be done >>> *before* unmapping. This would have mattered on parisc until we did >>> flush via tmpalias which means we no-longer care if the mapping for the >>> flush exists or not because we always recreate it via the tmpalias >>> pages. >> >> On ARM, flush_kernel_dcache_page() actually assumes that the page is >> mapped. It avoids double flushing of highmem pages by not flushing >> in those cases where kunmap_atomic() already takes care of flushing. > > Helge -- are you going to resubmit a version of this patch that makes the > recommended change? Sure, I'll do. May need some time for testing the various machine types though. Maybe in the end you can drop my patch since we might be able to fix it in the parisc arch code. @James, Dave: The patch I sent here was not used on the C8000. The C8000 (PA8800) works out of the box (and it uses the flush_kernel_dcache_page() function which is provided by kunmap()). It seems we require the flush_kernel_dcache_page() on the other machines too? Helge -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2013-11-17 at 22:23 +0100, Helge Deller wrote: > On 11/16/2013 09:09 PM, Benjamin LaHaise wrote: > > On Sat, Nov 16, 2013 at 09:07:18PM +0100, Simon Baatz wrote: > >> On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote: > >>> On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote: > >>>> When a user page mapping is released via kunmap*() functions, the D-cache needs > >>>> to be flushed via flush_dcache_page() to avoid D-cache aliasing issues. > >>>> > >>>> This patch fixes aio on the parisc platform (and probably others). > >>> > >>> This should be flush_kernel_dcache_page(). flush_dcache_page() is for > >>> full coherency but for unmap, we know the page was coherent going in and > >>> may have been modified by the kernel, so only the kernel view needs to > >>> be sync'd. Technically, by the kernel API, the flush should be done > >>> *before* unmapping. This would have mattered on parisc until we did > >>> flush via tmpalias which means we no-longer care if the mapping for the > >>> flush exists or not because we always recreate it via the tmpalias > >>> pages. > >> > >> On ARM, flush_kernel_dcache_page() actually assumes that the page is > >> mapped. It avoids double flushing of highmem pages by not flushing > >> in those cases where kunmap_atomic() already takes care of flushing. > > > > Helge -- are you going to resubmit a version of this patch that makes the > > recommended change? > > Sure, I'll do. May need some time for testing the various machine types though. > Maybe in the end you can drop my patch since we might be able to fix it in the > parisc arch code. > > @James, Dave: The patch I sent here was not used on the C8000. The C8000 (PA8800) > works out of the box (and it uses the flush_kernel_dcache_page() function which > is provided by kunmap()). It seems we require the flush_kernel_dcache_page() on > the other machines too? Well, this is why I think our in-kernel API (not the parisc one) for kmap/kunmap is faulty. I can't see any valid use of kmap/kunmap where you don't need to flush. Either you kmapped for reading, in which case you need to flush user space before the kernel access, or you kmapped for writing, in which case you need to flush user space just before mapping (eliminate any dirty user line) and the kernel cache before unmap (so the modification becomes visible in main memory). I could see an argument that flushing all the user mappings is expensive so only do it if necessary, but I think it looks to be always necessary: even if you're writing only, which means purging the lines above the user mappings, most hardware will panic (HPMC) if it sees inequivalent aliases with clashing dirty lines. I think the reason for the complexity is issues on ARM. The user space cache lines have to remain empty until the kunmap. On parisc we can ensure this by purging the tlb entries to prevent speculative move in (if the user deliberately accesses while we're modifying, it's caveat emptor), but I don't believe ARM can control this speculation, so they always need to re-invalidate all of user space before unmap just to eliminate speculated lines ... in any case, that could still be done as part of the kunmap API. James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Helge, On Sun, Nov 17, 2013 at 10:23:08PM +0100, Helge Deller wrote: > On 11/16/2013 09:09 PM, Benjamin LaHaise wrote: > > On Sat, Nov 16, 2013 at 09:07:18PM +0100, Simon Baatz wrote: > >> On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote: > >>> On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote: > >>>> When a user page mapping is released via kunmap*() functions, the D-cache needs > >>>> to be flushed via flush_dcache_page() to avoid D-cache aliasing issues. > >>>> > >>>> This patch fixes aio on the parisc platform (and probably others). > >>> > >>> This should be flush_kernel_dcache_page(). flush_dcache_page() is for > >>> full coherency but for unmap, we know the page was coherent going in and > >>> may have been modified by the kernel, so only the kernel view needs to > >>> be sync'd. Technically, by the kernel API, the flush should be done > >>> *before* unmapping. This would have mattered on parisc until we did > >>> flush via tmpalias which means we no-longer care if the mapping for the > >>> flush exists or not because we always recreate it via the tmpalias > >>> pages. > >> > >> On ARM, flush_kernel_dcache_page() actually assumes that the page is > >> mapped. It avoids double flushing of highmem pages by not flushing > >> in those cases where kunmap_atomic() already takes care of flushing. > > > > Helge -- are you going to resubmit a version of this patch that makes the > > recommended change? > > Sure, I'll do. May need some time for testing the various machine types though. > Maybe in the end you can drop my patch since we might be able to fix it in the > parisc arch code. Could you provide me with the test case(s) you are running to reproduce the problem? I could test this on aliasing D-cache on ARM as well. Since kmap/kunmap do not flush in general on ARM, we might need the explicit flushes here. - Simon -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Hi Helge, > Could you provide me with the test case(s) you are running to > reproduce the problem? I could test this on aliasing D-cache on ARM > as well. Since kmap/kunmap do not flush in general on ARM, we > might need the explicit flushes here. Just google for aio-stress.c (e.g. https://github.com/repoforge/rpms/blob/master/specs/aio-stress/aio-stress.c). And the aio01 / aio02 testcases from the Linux Test Project (https://github.com/linux-test-project/ltp). Helge -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/aio.c b/fs/aio.c index 823efcb..2181821 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -514,6 +514,7 @@ static void free_ioctx(struct work_struct *work) atomic_add(avail, &ctx->reqs_available); ring->head = ring->tail; kunmap_atomic(ring); + flush_dcache_page(ctx->ring_pages[0]); if (atomic_read(&ctx->reqs_available) >= ctx->nr_events - 1) break; @@ -568,6 +569,7 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm) ring = kmap_atomic(ctx->ring_pages[0]); ring->id = ctx->id; kunmap_atomic(ring); + flush_dcache_page(ctx->ring_pages[0]); return 0; } @@ -1032,6 +1034,7 @@ static long aio_read_events_ring(struct kioctx *ctx, head = ring->head; tail = ring->tail; kunmap_atomic(ring); + flush_dcache_page(ctx->ring_pages[0]); pr_debug("h%u t%u m%u\n", head, tail, ctx->nr_events);
When a user page mapping is released via kunmap*() functions, the D-cache needs to be flushed via flush_dcache_page() to avoid D-cache aliasing issues. This patch fixes aio on the parisc platform (and probably others). Signed-off-by: Helge Deller <deller@gmx.de> To: Benjamin LaHaise <bcrl@kvack.org> To: linux-aio@kvack.org Cc: stable@vger.kernel.org # 3.12+ -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html