ARM: handle user space mapped pages in flush_kernel_dcache_page

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

Thus, continue to do lazy flushing if there are no user space mappings. Otherwise, flush the kernel cache lines directly.

Signed-off-by Simon Baatz <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Russell King <[email protected]>

An issue is that for kunmap_atomic() && VIVT we flush the same page twice. I don't think we should remove the cache flushing in __kunmap_atomic() for VIVT since not all kunmap_atomic() calls require flush_kernel_dcache_page() (unless we just assume that highmem && vivt don't work together).

Quoted 5 lines · Catalin Marinas

An issue is that for kunmap_atomic() && VIVT we flush the same page twice. I don't think we should remove the cache flushing in __kunmap_atomic() for VIVT since not all kunmap_atomic() calls require flush_kernel_dcache_page() (unless we just assume that highmem && vivt don't work together).

VIVT and highmem do work together. Highmem for ARM was in fact developed on such a platform.

Quoted 8 lines · Nicolas Pitre

Quoted 5 lines · Catalin Marinas

An issue is that for kunmap_atomic() && VIVT we flush the same page twice. I don't think we should remove the cache flushing in __kunmap_atomic() for VIVT since not all kunmap_atomic() calls require flush_kernel_dcache_page() (unless we just assume that highmem && vivt don't work together).

Quoted 2 lines

VIVT and highmem do work together. Highmem for ARM was in fact developed on such a platform.

Thanks for confirming. Do you remember why kunmap() doesn't (need to) flush the cache alias (for VIVT)? Or if it does, where?

Quoted 9 lines · Catalin Marinas

Quoted 8 lines · Nicolas Pitre

Quoted 5 lines · Catalin Marinas

An issue is that for kunmap_atomic() && VIVT we flush the same page twice. I don't think we should remove the cache flushing in __kunmap_atomic() for VIVT since not all kunmap_atomic() calls require flush_kernel_dcache_page() (unless we just assume that highmem && vivt don't work together).

Quoted 5 lines

Quoted 2 lines

VIVT and highmem do work together. Highmem for ARM was in fact developed on such a platform.

Thanks for confirming. Do you remember why kunmap() doesn't (need to) flush the cache alias (for VIVT)? Or if it does, where?

kunmap() calls kunmap_high() which actually does not unmap anything but only decrement a use count. The page is kept around in case it is kmap()'d again, in which case the kmap() will be almost free and the page still warm.

It is only when the kmap() for a new page runs out of free pkmap entries that the unused but still mapped pages are unmapped. This unmapping is done in flush_all_zero_pkmaps() where all the unused pages are all unmapped at once. The cache for all those pages is flushed with flush_cache_kmaps() which on ARM maps to flush_cache_all() when the cache is VIVT.

Quoted 22 lines · Nicolas Pitre

Quoted 9 lines · Catalin Marinas

Quoted 8 lines · Nicolas Pitre

Quoted 5 lines · Catalin Marinas

An issue is that for kunmap_atomic() && VIVT we flush the same page twice. I don't think we should remove the cache flushing in __kunmap_atomic() for VIVT since not all kunmap_atomic() calls require flush_kernel_dcache_page() (unless we just assume that highmem && vivt don't work together).

Quoted 5 lines

Quoted 2 lines

VIVT and highmem do work together. Highmem for ARM was in fact developed on such a platform.

Thanks for confirming. Do you remember why kunmap() doesn't (need to) flush the cache alias (for VIVT)? Or if it does, where?

kunmap() calls kunmap_high() which actually does not unmap anything but only decrement a use count. The page is kept around in case it is kmap()'d again, in which case the kmap() will be almost free and the page still warm.

Quoted 6 lines

It is only when the kmap() for a new page runs out of free pkmap entries that the unused but still mapped pages are unmapped. This unmapping is done in flush_all_zero_pkmaps() where all the unused pages are all unmapped at once. The cache for all those pages is flushed with flush_cache_kmaps() which on ARM maps to flush_cache_all() when the cache is VIVT.

Thanks for clarification Nico. So if kmap_high_get() returns NULL, there are no high aliases left in the cache.

Quoted 30 lines · Catalin Marinas

Quoted 28 lines · Nicolas Pitre

Quoted 9 lines · Catalin Marinas

Quoted 8 lines · Nicolas Pitre

Quoted 5 lines · Catalin Marinas

An issue is that for kunmap_atomic() && VIVT we flush the same page twice. I don't think we should remove the cache flushing in __kunmap_atomic() for VIVT since not all kunmap_atomic() calls require flush_kernel_dcache_page() (unless we just assume that highmem && vivt don't work together).

Quoted 5 lines

Quoted 2 lines

VIVT and highmem do work together. Highmem for ARM was in fact developed on such a platform.

Thanks for confirming. Do you remember why kunmap() doesn't (need to) flush the cache alias (for VIVT)? Or if it does, where?

kunmap() calls kunmap_high() which actually does not unmap anything but only decrement a use count. The page is kept around in case it is kmap()'d again, in which case the kmap() will be almost free and the page still warm.

It is only when the kmap() for a new page runs out of free pkmap entries that the unused but still mapped pages are unmapped. This unmapping is done in flush_all_zero_pkmaps() where all the unused pages are all unmapped at once. The cache for all those pages is flushed with flush_cache_kmaps() which on ARM maps to flush_cache_all() when the cache is VIVT.

Quoted 2 lines

Thanks for clarification Nico. So if kmap_high_get() returns NULL, there are no high aliases left in the cache.

Exact.

Quoted 32 lines · Nicolas Pitre

Quoted 30 lines · Catalin Marinas

Quoted 28 lines · Nicolas Pitre

Quoted 9 lines · Catalin Marinas

Quoted 8 lines · Nicolas Pitre

Quoted 5 lines · Catalin Marinas

An issue is that for kunmap_atomic() && VIVT we flush the same page twice. I don't think we should remove the cache flushing in __kunmap_atomic() for VIVT since not all kunmap_atomic() calls require flush_kernel_dcache_page() (unless we just assume that highmem && vivt don't work together).

Quoted 5 lines

Quoted 2 lines

VIVT and highmem do work together. Highmem for ARM was in fact developed on such a platform.

Thanks for confirming. Do you remember why kunmap() doesn't (need to) flush the cache alias (for VIVT)? Or if it does, where?

kunmap() calls kunmap_high() which actually does not unmap anything but only decrement a use count. The page is kept around in case it is kmap()'d again, in which case the kmap() will be almost free and the page still warm.

It is only when the kmap() for a new page runs out of free pkmap entries that the unused but still mapped pages are unmapped. This unmapping is done in flush_all_zero_pkmaps() where all the unused pages are all unmapped at once. The cache for all those pages is flushed with flush_cache_kmaps() which on ARM maps to flush_cache_all() when the cache is VIVT.

Quoted 4 lines

Quoted 2 lines

Thanks for clarification Nico. So if kmap_high_get() returns NULL, there are no high aliases left in the cache.

Exact.

Just to be clear, this is true for VIVT only. Unmapped highmem pages can still be cached with VIPT.

Quoted 10 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

Thus, continue to do lazy flushing if there are no user space mappings. Otherwise, flush the kernel cache lines directly.

Signed-off-by Simon Baatz <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Russell King <[email protected]>

An issue is that for kunmap_atomic() && VIVT we flush the same page twice. I don't think we should remove the cache flushing in __kunmap_atomic() for VIVT since not all kunmap_atomic() calls require flush_kernel_dcache_page() (unless we just assume that highmem && vivt don't work together). Flushing the VIVT cache on kunmap is essential since we just lose the alias after that.

Quoted 9 lines

Quoted 9 lines

Quoted 2 lines

We probably should reuse it in flush_kernel_dcache_page() to flush the kernel mapping. (The last else clause looks strange though)

I think it makes sense to reuse this logic in flush_kernel_dcache_page(). If the page is already mapped with kmap, then kmap_high_get() should return the actual address. If it was mapped with kmap_atomic, kmap_high_get() would return NULL, hence the 'else' clause and the additional kmap_atomic(). The cache_is_vipt() check is useful because kunmap_atomic() would flush VIVT caches anyway.

arch/arm/mm/flush.c void flush_dcache_page(struct page *page) · line 318

Quoted 3 lines

arch/arm/mm/flush.c +3 -0

+void flush_kernel_dcache_page(struct page *page)
+{
+	if (cache_is_vivt() || cache_is_vipt_aliasing()) {

So you could add the ((cache_is_vivt() || cache_is_vipt_aliasing()) && !PageHighMem(page)) check back (as it was prior to commit f8b63c184, actually reverting it) and the mapping tests as a possible optimisation. But in this case there is no need to split __flush_dcache_page() in two since we don't care about the PageHighMem() case.

And we need to fix kunmap() with VIVT to flush the cache as well (in case anyone uses highmem on VIVT hardware).

Quoted 12 lines · Catalin Marinas

Quoted 10 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

Thus, continue to do lazy flushing if there are no user space mappings. Otherwise, flush the kernel cache lines directly.

Signed-off-by Simon Baatz <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Russell King <[email protected]>

Quoted 6 lines

An issue is that for kunmap_atomic() && VIVT we flush the same page twice. I don't think we should remove the cache flushing in __kunmap_atomic() for VIVT since not all kunmap_atomic() calls require flush_kernel_dcache_page() (unless we just assume that highmem && vivt don't work together). Flushing the VIVT cache on kunmap is essential since we just lose the alias after that.

Not sure if I am missing something here. From our previous discussion:

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 2 lines

We probably should reuse it in flush_kernel_dcache_page() to flush the kernel mapping. (The last else clause looks strange though)

I think it makes sense to reuse this logic in flush_kernel_dcache_page(). If the page is already mapped with kmap, then kmap_high_get() should return the actual address. If it was mapped with kmap_atomic, kmap_high_get() would return NULL, hence the 'else' clause and the additional kmap_atomic(). The cache_is_vipt() check is useful because kunmap_atomic() would flush VIVT caches anyway.

As you say, this logic does not flush on VIVT if the page was mapped via kmap_atomic() (and kmap_atomic() could not reuse an existing mapping via kmap_high_get())

kmap_atomic() tries to reuse an existing mapping and only allocates a new dedicated kmap if it finds none. Consequently, __kunmap_atomic() only flushes in the latter case (because we lose the alias).

Thus, no double flushing should occur here. If kunmap_atomic() will flush, __flush_kernel_dcache_page() will not and vice versa.

Quoted 20 lines · Simon Baatz

Quoted 12 lines · Catalin Marinas

Quoted 10 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

Thus, continue to do lazy flushing if there are no user space mappings. Otherwise, flush the kernel cache lines directly.

Signed-off-by Simon Baatz <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Russell King <[email protected]>

Quoted 6 lines

An issue is that for kunmap_atomic() && VIVT we flush the same page twice. I don't think we should remove the cache flushing in __kunmap_atomic() for VIVT since not all kunmap_atomic() calls require flush_kernel_dcache_page() (unless we just assume that highmem && vivt don't work together). Flushing the VIVT cache on kunmap is essential since we just lose the alias after that.

Quoted 12 lines

Not sure if I am missing something here. From our previous discussion:

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 2 lines

We probably should reuse it in flush_kernel_dcache_page() to flush the kernel mapping. (The last else clause looks strange though)

I think it makes sense to reuse this logic in flush_kernel_dcache_page(). If the page is already mapped with kmap, then kmap_high_get() should return the actual address. If it was mapped with kmap_atomic, kmap_high_get() would return NULL, hence the 'else' clause and the additional kmap_atomic(). The cache_is_vipt() check is useful because kunmap_atomic() would flush VIVT caches anyway.

I think the context was that page_address() is not always valid after kmap_atomic(), so reusing the same logic would make sense.

But the key point I try to understand is why kunmap() does not need to flush the VIVT cache. If it does need to, then we can simplify your patch.

Quoted 7 lines

As you say, this logic does not flush on VIVT if the page was mapped via kmap_atomic() (and kmap_atomic() could not reuse an existing mapping via kmap_high_get())

kmap_atomic() tries to reuse an existing mapping and only allocates a new dedicated kmap if it finds none. Consequently, __kunmap_atomic() only flushes in the latter case (because we lose the alias).

Quoted 3 lines

Thus, no double flushing should occur here. If kunmap_atomic() will flush, __flush_kernel_dcache_page() will not and vice versa.

That's correct for the current code. Let's wait for some insight from Nico or Russell on whether kunmap() needs to lush the VIVT cache as well. If it doesn't need to, then my original comment made sense.

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

Thus, continue to do lazy flushing if there are no user space mappings. Otherwise, flush the kernel cache lines directly.

/*
+ * Ensure cache coherency for kernel mapping of this page.
+ *
+ * If the page only exists in the page cache and there are no user
+ * space mappings, this is a no-op since the page was already marked
+ * dirty at creation. Otherwise, we need to flush the dirty kernel
+ * cache lines directly.
+ */

arch/arm/mm/flush.c +1 -0

+void flush_kernel_dcache_page(struct page *page)

Quoted 3 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

After Nico's clarification, I think the original commit introducing this function was also incomplete (commit 73be1591 - [ARM] 5545/2: add flush_kernel_dcache_page() for ARM) since it ignores highmem pages and their flushing could be deferred for a long time.

For my understanding (if I re-read this tread) - basically code like this should not leave the user mapping inconsistent:

kmap()
...
flush_kernel_dcache_page()
kunmap()

If we use the atomic variants, we get the cache flushing automatically but the kunmap_high() does not flush the cache immediately, so we need to handle it in flush_kernel_dcache_page().

Quoted 2 lines

Thus, continue to do lazy flushing if there are no user space mappings. Otherwise, flush the kernel cache lines directly.

...

Quoted 15 lines

/*
+ * Ensure cache coherency for kernel mapping of this page.
+ *
+ * If the page only exists in the page cache and there are no user
+ * space mappings, this is a no-op since the page was already marked
+ * dirty at creation. Otherwise, we need to flush the dirty kernel
+ * cache lines directly.
+ */

arch/arm/mm/flush.c +7 -0

+void flush_kernel_dcache_page(struct page *page)
+{
+	if (cache_is_vivt() || cache_is_vipt_aliasing()) {
+		struct address_space *mapping;
+
+		mapping = page_mapping(page);
+

arch/arm/mm/flush.c void flush_dcache_page(struct page *page) · line 326

Quoted 4 lines

arch/arm/mm/flush.c +4 -0

+		if (!mapping || mapping_mapped(mapping))
+			__flush_kernel_dcache_page(page);
+	}
+}

BTW, does the mapping check optimise anything for the flush_kernel_dcache_page() uses? Would we have a mapping anyway (or anonymous page) in most cases?

Otherwise the patch looks good.

Quoted 9 lines · Catalin Marinas

Quoted 3 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

After Nico's clarification, I think the original commit introducing this function was also incomplete (commit 73be1591 - [ARM] 5545/2: add flush_kernel_dcache_page() for ARM) since it ignores highmem pages and their flushing could be deferred for a long time.

Yes, I agree.

Quoted 7 lines

For my understanding (if I re-read this tread) - basically code like this should not leave the user mapping inconsistent:

kmap()
...
flush_kernel_dcache_page()
kunmap()

s/user mapping/kernel mapping/
The user mapping(s) can be assumed to be consistent when entering such code. Either there is none or the page was obtained by a mechanism like __get_user_pages(). Otherwise, we have a problem anyway when accessing the page via the kernel mapping.

Quoted 18 lines

Quoted 2 lines

Thus, continue to do lazy flushing if there are no user space mappings. Otherwise, flush the kernel cache lines directly.

...

Quoted 15 lines

/*
+ * Ensure cache coherency for kernel mapping of this page.
+ *
+ * If the page only exists in the page cache and there are no user
+ * space mappings, this is a no-op since the page was already marked
+ * dirty at creation. Otherwise, we need to flush the dirty kernel
+ * cache lines directly.
+ */

arch/arm/mm/flush.c +7 -0

+void flush_kernel_dcache_page(struct page *page)
+{
+	if (cache_is_vivt() || cache_is_vipt_aliasing()) {
+		struct address_space *mapping;
+
+		mapping = page_mapping(page);
+

Quoted 8 lines

arch/arm/mm/flush.c void flush_dcache_page(struct page *page) · line 326

Quoted 4 lines

arch/arm/mm/flush.c +4 -0

+		if (!mapping || mapping_mapped(mapping))
+			__flush_kernel_dcache_page(page);
+	}
+}

BTW, does the mapping check optimise anything for the flush_kernel_dcache_page() uses? Would we have a mapping anyway (or anonymous page) in most cases?

Looking at the relevant uses in the kernel, we have:

drivers/scsi/libsas/sas_host_smp.c
drivers/mmc/host/mmc_spi.c
drivers/block/ps3disk.c
fs/exec.c
lib/scatterlist.c

That is not much (I omit my usual rant here that many uses of flush_dcache_page() in drivers are incorrect and should be uses of flush_kernel_dcache_page() ...).

Without looking at the actual code, we seem to have two basic use cases:

1. Block drivers (the ones listed above + those using the memory scatterlist iterator in scatterlist.c)

These will probably almost exclusively use page cache pages for which we can be lazy. Other pages only occur in special I/O situations like direct I/O.

2. fs/exec.c

I think these are anonymous pages only

Thus depending on the actual drivers used, usage can be dominated by page cache pages on one setup and anon pages on the other.

I prefer the currently proposed way since it prevents massive overflushing if flush_kernel_dcache_page() is used in an I/O path.

(Btw. I am still puzzled as to why making flush_kernel_dcache_page() the current nop apparently did not cause problems in fs/exec.c.)

Quoted 1 line

Otherwise the patch looks good.

Thanks. Especially, thanks for pointing out the highmem case.

Quoted 21 lines · Simon Baatz

Quoted 9 lines · Catalin Marinas

Quoted 3 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

After Nico's clarification, I think the original commit introducing this function was also incomplete (commit 73be1591 - [ARM] 5545/2: add flush_kernel_dcache_page() for ARM) since it ignores highmem pages and their flushing could be deferred for a long time.

Yes, I agree.

Quoted 7 lines

For my understanding (if I re-read this tread) - basically code like this should not leave the user mapping inconsistent:

kmap()
...
flush_kernel_dcache_page()
kunmap()

Quoted 5 lines

s/user mapping/kernel mapping/
The user mapping(s) can be assumed to be consistent when entering such code. Either there is none or the page was obtained by a mechanism like __get_user_pages(). Otherwise, we have a problem anyway when accessing the page via the kernel mapping.

Indeed. What I meant was user mapping inconsistent with the kernel mapping. It's the latter that needs flushing.

Quoted 53 lines

Quoted 18 lines

Quoted 2 lines

Thus, continue to do lazy flushing if there are no user space mappings. Otherwise, flush the kernel cache lines directly.

...

Quoted 15 lines

/*
+ * Ensure cache coherency for kernel mapping of this page.
+ *
+ * If the page only exists in the page cache and there are no user
+ * space mappings, this is a no-op since the page was already marked
+ * dirty at creation. Otherwise, we need to flush the dirty kernel
+ * cache lines directly.
+ */

arch/arm/mm/flush.c +7 -0

+void flush_kernel_dcache_page(struct page *page)
+{
+	if (cache_is_vivt() || cache_is_vipt_aliasing()) {
+		struct address_space *mapping;
+
+		mapping = page_mapping(page);
+

Quoted 8 lines

arch/arm/mm/flush.c void flush_dcache_page(struct page *page) · line 326

Quoted 4 lines

arch/arm/mm/flush.c +4 -0

+		if (!mapping || mapping_mapped(mapping))
+			__flush_kernel_dcache_page(page);
+	}
+}

BTW, does the mapping check optimise anything for the flush_kernel_dcache_page() uses? Would we have a mapping anyway (or anonymous page) in most cases?

Looking at the relevant uses in the kernel, we have:

drivers/scsi/libsas/sas_host_smp.c
drivers/mmc/host/mmc_spi.c
drivers/block/ps3disk.c
fs/exec.c
lib/scatterlist.c

That is not much (I omit my usual rant here that many uses of flush_dcache_page() in drivers are incorrect and should be uses of flush_kernel_dcache_page() ...).

Without looking at the actual code, we seem to have two basic use cases:

1. Block drivers (the ones listed above + those using the memory scatterlist iterator in scatterlist.c)

These will probably almost exclusively use page cache pages for which we can be lazy. Other pages only occur in special I/O situations like direct I/O.

2. fs/exec.c

I think these are anonymous pages only

Quoted 8 lines

Thus depending on the actual drivers used, usage can be dominated by page cache pages on one setup and anon pages on the other.

I prefer the currently proposed way since it prevents massive overflushing if flush_kernel_dcache_page() is used in an I/O path.

(Btw. I am still puzzled as to why making flush_kernel_dcache_page() the current nop apparently did not cause problems in fs/exec.c.)

We get extra flushing via flush_old_exec() -> exec_mmap() -> mmput() -> exit_mmap() -> flush_cache_mm() before we actually start the new exec so this would flush the arg page as well.

Quoted 3 lines

Quoted 1 line

Otherwise the patch looks good.

Thanks. Especially, thanks for pointing out the highmem case.

You can add my ack (before I forget the whole discussion ;))

Acked-by Catalin Marinas <[email protected]>

Quoted 101 lines · Catalin Marinas

Quoted 21 lines · Simon Baatz

Quoted 9 lines · Catalin Marinas

Quoted 3 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

After Nico's clarification, I think the original commit introducing this function was also incomplete (commit 73be1591 - [ARM] 5545/2: add flush_kernel_dcache_page() for ARM) since it ignores highmem pages and their flushing could be deferred for a long time.

Yes, I agree.

Quoted 7 lines

For my understanding (if I re-read this tread) - basically code like this should not leave the user mapping inconsistent:

kmap()
...
flush_kernel_dcache_page()
kunmap()

Quoted 5 lines

s/user mapping/kernel mapping/
The user mapping(s) can be assumed to be consistent when entering such code. Either there is none or the page was obtained by a mechanism like __get_user_pages(). Otherwise, we have a problem anyway when accessing the page via the kernel mapping.

Indeed. What I meant was user mapping inconsistent with the kernel mapping. It's the latter that needs flushing.

Quoted 53 lines

Quoted 18 lines

Quoted 2 lines

Thus, continue to do lazy flushing if there are no user space mappings. Otherwise, flush the kernel cache lines directly.

...

Quoted 15 lines

/*
+ * Ensure cache coherency for kernel mapping of this page.
+ *
+ * If the page only exists in the page cache and there are no user
+ * space mappings, this is a no-op since the page was already marked
+ * dirty at creation. Otherwise, we need to flush the dirty kernel
+ * cache lines directly.
+ */

arch/arm/mm/flush.c +7 -0

+void flush_kernel_dcache_page(struct page *page)
+{
+	if (cache_is_vivt() || cache_is_vipt_aliasing()) {
+		struct address_space *mapping;
+
+		mapping = page_mapping(page);
+

Quoted 8 lines

arch/arm/mm/flush.c void flush_dcache_page(struct page *page) · line 326

Quoted 4 lines

arch/arm/mm/flush.c +4 -0

+		if (!mapping || mapping_mapped(mapping))
+			__flush_kernel_dcache_page(page);
+	}
+}

BTW, does the mapping check optimise anything for the flush_kernel_dcache_page() uses? Would we have a mapping anyway (or anonymous page) in most cases?

Looking at the relevant uses in the kernel, we have:

drivers/scsi/libsas/sas_host_smp.c
drivers/mmc/host/mmc_spi.c
drivers/block/ps3disk.c
fs/exec.c
lib/scatterlist.c

That is not much (I omit my usual rant here that many uses of flush_dcache_page() in drivers are incorrect and should be uses of flush_kernel_dcache_page() ...).

Without looking at the actual code, we seem to have two basic use cases:

1. Block drivers (the ones listed above + those using the memory scatterlist iterator in scatterlist.c)

These will probably almost exclusively use page cache pages for which we can be lazy. Other pages only occur in special I/O situations like direct I/O.

2. fs/exec.c

I think these are anonymous pages only

Quoted 8 lines

Thus depending on the actual drivers used, usage can be dominated by page cache pages on one setup and anon pages on the other.

I prefer the currently proposed way since it prevents massive overflushing if flush_kernel_dcache_page() is used in an I/O path.

(Btw. I am still puzzled as to why making flush_kernel_dcache_page() the current nop apparently did not cause problems in fs/exec.c.)

We get extra flushing via flush_old_exec() -> exec_mmap() -> mmput() -> exit_mmap() -> flush_cache_mm() before we actually start the new exec so this would flush the arg page as well.

Quoted 3 lines

Quoted 1 line

Otherwise the patch looks good.

Thanks. Especially, thanks for pointing out the highmem case.

Quoted 3 lines

You can add my ack (before I forget the whole discussion ;))

Acked-by Catalin Marinas <[email protected]>

wrt to 73be1591, this appears to go all the way back to v2.6.31.x... Can Simon go ahead and put this in rmk's patch tracker and mention that it should go to all -stable trees?

Quoted 106 lines · Jason Cooper

Quoted 104 lines · Catalin Marinas

Quoted 21 lines · Simon Baatz

Quoted 9 lines · Catalin Marinas

Quoted 3 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

After Nico's clarification, I think the original commit introducing this function was also incomplete (commit 73be1591 - [ARM] 5545/2: add flush_kernel_dcache_page() for ARM) since it ignores highmem pages and their flushing could be deferred for a long time.

Yes, I agree.

Quoted 7 lines

For my understanding (if I re-read this tread) - basically code like this should not leave the user mapping inconsistent:

kmap()
...
flush_kernel_dcache_page()
kunmap()

Quoted 5 lines

s/user mapping/kernel mapping/
The user mapping(s) can be assumed to be consistent when entering such code. Either there is none or the page was obtained by a mechanism like __get_user_pages(). Otherwise, we have a problem anyway when accessing the page via the kernel mapping.

Indeed. What I meant was user mapping inconsistent with the kernel mapping. It's the latter that needs flushing.

Quoted 53 lines

Quoted 18 lines

Quoted 2 lines

Thus, continue to do lazy flushing if there are no user space mappings. Otherwise, flush the kernel cache lines directly.

...

Quoted 15 lines

/*
+ * Ensure cache coherency for kernel mapping of this page.
+ *
+ * If the page only exists in the page cache and there are no user
+ * space mappings, this is a no-op since the page was already marked
+ * dirty at creation. Otherwise, we need to flush the dirty kernel
+ * cache lines directly.
+ */

arch/arm/mm/flush.c +7 -0

+void flush_kernel_dcache_page(struct page *page)
+{
+	if (cache_is_vivt() || cache_is_vipt_aliasing()) {
+		struct address_space *mapping;
+
+		mapping = page_mapping(page);
+

Quoted 8 lines

arch/arm/mm/flush.c void flush_dcache_page(struct page *page) · line 326

Quoted 4 lines

arch/arm/mm/flush.c +4 -0

+		if (!mapping || mapping_mapped(mapping))
+			__flush_kernel_dcache_page(page);
+	}
+}

BTW, does the mapping check optimise anything for the flush_kernel_dcache_page() uses? Would we have a mapping anyway (or anonymous page) in most cases?

Looking at the relevant uses in the kernel, we have:

drivers/scsi/libsas/sas_host_smp.c
drivers/mmc/host/mmc_spi.c
drivers/block/ps3disk.c
fs/exec.c
lib/scatterlist.c

That is not much (I omit my usual rant here that many uses of flush_dcache_page() in drivers are incorrect and should be uses of flush_kernel_dcache_page() ...).

Without looking at the actual code, we seem to have two basic use cases:

1. Block drivers (the ones listed above + those using the memory scatterlist iterator in scatterlist.c)

These will probably almost exclusively use page cache pages for which we can be lazy. Other pages only occur in special I/O situations like direct I/O.

2. fs/exec.c

I think these are anonymous pages only

Quoted 8 lines

Thus depending on the actual drivers used, usage can be dominated by page cache pages on one setup and anon pages on the other.

I prefer the currently proposed way since it prevents massive overflushing if flush_kernel_dcache_page() is used in an I/O path.

(Btw. I am still puzzled as to why making flush_kernel_dcache_page() the current nop apparently did not cause problems in fs/exec.c.)

We get extra flushing via flush_old_exec() -> exec_mmap() -> mmput() -> exit_mmap() -> flush_cache_mm() before we actually start the new exec so this would flush the arg page as well.

Quoted 3 lines

Quoted 1 line

Otherwise the patch looks good.

Thanks. Especially, thanks for pointing out the highmem case.

You can add my ack (before I forget the whole discussion ;))

Acked-by Catalin Marinas <[email protected]>

Quoted 3 lines

wrt to 73be1591, this appears to go all the way back to v2.6.31.x... Can Simon go ahead and put this in rmk's patch tracker and mention that it should go to all -stable trees?

I'm not sure how easy it is to apply this patch on past stable versions. Maybe something simpler for stable like always flushing the cache in flush_kernel_dcache_page() with optimisation only in recent stable versions.

Quoted 22 lines · Jason Cooper

Quoted 21 lines · Catalin Marinas

Quoted 20 lines · Simon Baatz

Quoted 9 lines · Catalin Marinas

Quoted 3 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

After Nico's clarification, I think the original commit introducing this function was also incomplete (commit 73be1591 - [ARM] 5545/2: add flush_kernel_dcache_page() for ARM) since it ignores highmem pages and their flushing could be deferred for a long time.

Yes, I agree.

Quoted 7 lines

For my understanding (if I re-read this tread) - basically code like this should not leave the user mapping inconsistent:

kmap()
...
flush_kernel_dcache_page()
kunmap()

...

Quoted 1 line

Quoted 3 lines

wrt to 73be1591, this appears to go all the way back to v2.6.31.x... Can Simon go ahead and put this in rmk's patch tracker and mention that it should go to all -stable trees?

I'm not sure how easy it is to apply this patch on past stable versions. Maybe something simpler for stable like always flushing the cache in flush_kernel_dcache_page() with optimisation only in recent stable versions.

Hi Catalin,

Quoted 25 lines · Catalin Marinas

Quoted 22 lines · Jason Cooper

Quoted 21 lines · Catalin Marinas

Quoted 20 lines · Simon Baatz

Quoted 9 lines · Catalin Marinas

Quoted 3 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

After Nico's clarification, I think the original commit introducing this function was also incomplete (commit 73be1591 - [ARM] 5545/2: add flush_kernel_dcache_page() for ARM) since it ignores highmem pages and their flushing could be deferred for a long time.

Yes, I agree.

Quoted 7 lines

For my understanding (if I re-read this tread) - basically code like this should not leave the user mapping inconsistent:

kmap()
...
flush_kernel_dcache_page()
kunmap()

...

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

wrt to 73be1591, this appears to go all the way back to v2.6.31.x... Can Simon go ahead and put this in rmk's patch tracker and mention that it should go to all -stable trees?

I'm not sure how easy it is to apply this patch on past stable versions. Maybe something simpler for stable like always flushing the cache in flush_kernel_dcache_page() with optimisation only in recent stable versions.

Looking at how to adapt the change to previous kernel versions, it occurred to me that we could probably simplify the current patch since we can make stronger assumptions than __flush_dcache_page() can do. As discussed, __flush_dcache_page() does:

                        addr = kmap_high_get(page);
                        if (addr) {
                                __cpuc_flush_dcache_area(addr, PAGE_SIZE);
                                kunmap_high(page);
                        }

The kmap_high_get() is used to ensure that the page is/remains pinned during the flush. However, the API of flush_kernel_dcache_page() ensures that the page is already kmapped when it is called (as in the quoted example above).

Thus, it is already pinned and we can simply look at page_address(). If it is NULL, the page must have been obtained via kmap_atomic() and we don't need to flush anyway since kunmap_atomic() will do this.

The actual flush covering both the lowmem and highmem cases actually is as simple as this (inspired by __flush_dcache_page() in 2.6.32):

	addr = page_address(page);
#ifdef CONFIG_HIGHMEM
	/*
	 * kmap_atomic() doesn't set the page virtual address, and
	 * kunmap_atomic() takes care of cache flushing already.
	 */
	if (addr)
#endif
		__cpuc_flush_dcache_area(addr, PAGE_SIZE);

If you agree that this makes sense, I will send a new version.

If we change the patch this way, there is no need to split off __flush_kernel_dcache_page() from __flush_dcache_page(), which also makes it simpler to apply to past stable kernels.

The only thing we need to be aware of is the change of flush_kernel_dcache_page() in 2.6.37 (commit f8b63c1). In earlier versions, we would only need to fix the highmem case. However, I wonder whether it makes sense to apply a fix to 2.6.32 and 2.6.34 without this ever being reported as a problem.

All,

Adding gregkh for insight into -stable and applying fixes to previous kernels when the bug was not against them (but the offending commit exists in them).

Greg,

Some history: This bug stems from attempting to write O_DIRECT to a dm-crypt device on ARM architectures. In my case, it was trying (years ago!) to put an LVM into a dm-crypt container. This data corruption can be more readily seen by using dd to write to a dm-crypt device with the direct flag.

...

Quoted 6 lines

The only thing we need to be aware of is the change of flush_kernel_dcache_page() in 2.6.37 (commit f8b63c1). In earlier versions, we would only need to fix the highmem case. However, I wonder whether it makes sense to apply a fix to 2.6.32 and 2.6.34 without this ever being reported as a problem.

imho, if you discover a problem while running kernel v3.9.x, and the commit causing the problem goes all the way back to 2.6.32, then the fix should go back that far as well (assuming those kernels are still actively maintained). I don't think we would need a bug report for each kernel version.

All,

Adding gregkh for insight into -stable and applying fixes to previous kernels when the bug was not against them (but the offending commit exists in them).

Greg,

Some history: This bug stems from attempting to write O_DIRECT to a dm-crypt device on ARM architectures. In my case, it was trying (years ago!) to put an LVM into a dm-crypt container. This data corruption can be more readily seen by using dd to write to a dm-crypt device with the direct flag.

Quoted 76 lines · Simon Baatz

Hi Catalin,

Quoted 25 lines · Catalin Marinas

Quoted 22 lines · Jason Cooper

Quoted 21 lines · Catalin Marinas

Quoted 20 lines · Simon Baatz

Quoted 9 lines · Catalin Marinas

Quoted 3 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

After Nico's clarification, I think the original commit introducing this function was also incomplete (commit 73be1591 - [ARM] 5545/2: add flush_kernel_dcache_page() for ARM) since it ignores highmem pages and their flushing could be deferred for a long time.

Yes, I agree.

Quoted 7 lines

For my understanding (if I re-read this tread) - basically code like this should not leave the user mapping inconsistent:

kmap()
...
flush_kernel_dcache_page()
kunmap()

...

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

wrt to 73be1591, this appears to go all the way back to v2.6.31.x... Can Simon go ahead and put this in rmk's patch tracker and mention that it should go to all -stable trees?

I'm not sure how easy it is to apply this patch on past stable versions. Maybe something simpler for stable like always flushing the cache in flush_kernel_dcache_page() with optimisation only in recent stable versions.

Looking at how to adapt the change to previous kernel versions, it occurred to me that we could probably simplify the current patch since we can make stronger assumptions than __flush_dcache_page() can do. As discussed, __flush_dcache_page() does:

                        addr = kmap_high_get(page);
                        if (addr) {
                                __cpuc_flush_dcache_area(addr, PAGE_SIZE);
                                kunmap_high(page);
                        }

The kmap_high_get() is used to ensure that the page is/remains pinned during the flush. However, the API of flush_kernel_dcache_page() ensures that the page is already kmapped when it is called (as in the quoted example above).

Thus, it is already pinned and we can simply look at page_address(). If it is NULL, the page must have been obtained via kmap_atomic() and we don't need to flush anyway since kunmap_atomic() will do this.

The actual flush covering both the lowmem and highmem cases actually is as simple as this (inspired by __flush_dcache_page() in 2.6.32):

	addr = page_address(page);
#ifdef CONFIG_HIGHMEM
	/*
	 * kmap_atomic() doesn't set the page virtual address, and
	 * kunmap_atomic() takes care of cache flushing already.
	 */
	if (addr)
#endif
		__cpuc_flush_dcache_area(addr, PAGE_SIZE);

If you agree that this makes sense, I will send a new version.

If we change the patch this way, there is no need to split off __flush_kernel_dcache_page() from __flush_dcache_page(), which also makes it simpler to apply to past stable kernels.

Quoted 5 lines

The only thing we need to be aware of is the change of flush_kernel_dcache_page() in 2.6.37 (commit f8b63c1). In earlier versions, we would only need to fix the highmem case. However, I wonder whether it makes sense to apply a fix to 2.6.32 and 2.6.34 without this ever being reported as a problem.

imho, if you discover a problem while running kernel v3.9.x, and the commit causing the problem goes all the way back to 2.6.32, then the fix should go back that far as well (assuming those kernels are still actively maintained). I don't think we would need a bug report for each kernel version.

However, some more nuanced problems are not the result of a single commit, but potentially the interaction of several commits. In this case, a more conservative application may be in order.

Greg, any thoughts?

thx,

Jason.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

It would be helpful if I actually did as I stated ;-)

Adding Greg for real this time.

Quoted 108 lines · Jason Cooper

All,

Adding gregkh for insight into -stable and applying fixes to previous kernels when the bug was not against them (but the offending commit exists in them).

Greg,

Some history: This bug stems from attempting to write O_DIRECT to a dm-crypt device on ARM architectures. In my case, it was trying (years ago!) to put an LVM into a dm-crypt container. This data corruption can be more readily seen by using dd to write to a dm-crypt device with the direct flag.

Quoted 76 lines · Simon Baatz

Hi Catalin,

Quoted 25 lines · Catalin Marinas

Quoted 22 lines · Jason Cooper

Quoted 21 lines · Catalin Marinas

Quoted 20 lines · Simon Baatz

Quoted 9 lines · Catalin Marinas

Quoted 3 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

After Nico's clarification, I think the original commit introducing this function was also incomplete (commit 73be1591 - [ARM] 5545/2: add flush_kernel_dcache_page() for ARM) since it ignores highmem pages and their flushing could be deferred for a long time.

Yes, I agree.

Quoted 7 lines

For my understanding (if I re-read this tread) - basically code like this should not leave the user mapping inconsistent:

kmap()
...
flush_kernel_dcache_page()
kunmap()

...

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

wrt to 73be1591, this appears to go all the way back to v2.6.31.x... Can Simon go ahead and put this in rmk's patch tracker and mention that it should go to all -stable trees?

I'm not sure how easy it is to apply this patch on past stable versions. Maybe something simpler for stable like always flushing the cache in flush_kernel_dcache_page() with optimisation only in recent stable versions.

Looking at how to adapt the change to previous kernel versions, it occurred to me that we could probably simplify the current patch since we can make stronger assumptions than __flush_dcache_page() can do. As discussed, __flush_dcache_page() does:

                        addr = kmap_high_get(page);
                        if (addr) {
                                __cpuc_flush_dcache_area(addr, PAGE_SIZE);
                                kunmap_high(page);
                        }

The kmap_high_get() is used to ensure that the page is/remains pinned during the flush. However, the API of flush_kernel_dcache_page() ensures that the page is already kmapped when it is called (as in the quoted example above).

Thus, it is already pinned and we can simply look at page_address(). If it is NULL, the page must have been obtained via kmap_atomic() and we don't need to flush anyway since kunmap_atomic() will do this.

The actual flush covering both the lowmem and highmem cases actually is as simple as this (inspired by __flush_dcache_page() in 2.6.32):

	addr = page_address(page);
#ifdef CONFIG_HIGHMEM
	/*
	 * kmap_atomic() doesn't set the page virtual address, and
	 * kunmap_atomic() takes care of cache flushing already.
	 */
	if (addr)
#endif
		__cpuc_flush_dcache_area(addr, PAGE_SIZE);

If you agree that this makes sense, I will send a new version.

If we change the patch this way, there is no need to split off __flush_kernel_dcache_page() from __flush_dcache_page(), which also makes it simpler to apply to past stable kernels.

Quoted 5 lines

The only thing we need to be aware of is the change of flush_kernel_dcache_page() in 2.6.37 (commit f8b63c1). In earlier versions, we would only need to fix the highmem case. However, I wonder whether it makes sense to apply a fix to 2.6.32 and 2.6.34 without this ever being reported as a problem.

imho, if you discover a problem while running kernel v3.9.x, and the commit causing the problem goes all the way back to 2.6.32, then the fix should go back that far as well (assuming those kernels are still actively maintained). I don't think we would need a bug report for each kernel version.

However, some more nuanced problems are not the result of a single commit, but potentially the interaction of several commits. In this case, a more conservative application may be in order.

Greg, any thoughts?

Quoted 25 lines · Catalin Marinas

Quoted 22 lines · Jason Cooper

Quoted 21 lines · Catalin Marinas

Quoted 20 lines · Simon Baatz

Quoted 9 lines · Catalin Marinas

Quoted 3 lines · Simon Baatz

Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages it needs to handle are kernel mapped only. However, for example when doing direct I/O, pages with user space mappings may occur.

After Nico's clarification, I think the original commit introducing this function was also incomplete (commit 73be1591 - [ARM] 5545/2: add flush_kernel_dcache_page() for ARM) since it ignores highmem pages and their flushing could be deferred for a long time.

Yes, I agree.

Quoted 7 lines

For my understanding (if I re-read this tread) - basically code like this should not leave the user mapping inconsistent:

kmap()
...
flush_kernel_dcache_page()
kunmap()

...

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

wrt to 73be1591, this appears to go all the way back to v2.6.31.x... Can Simon go ahead and put this in rmk's patch tracker and mention that it should go to all -stable trees?

I'm not sure how easy it is to apply this patch on past stable versions. Maybe something simpler for stable like always flushing the cache in flush_kernel_dcache_page() with optimisation only in recent stable versions.

Looking at how to adapt the change to previous kernel versions, it occurred to me that we could probably simplify the current patch since we can make stronger assumptions than __flush_dcache_page() can do. As discussed, __flush_dcache_page() does:

                        addr = kmap_high_get(page);
                        if (addr) {
                                __cpuc_flush_dcache_area(addr, PAGE_SIZE);
                                kunmap_high(page);
                        }

The kmap_high_get() is used to ensure that the page is/remains pinned during the flush. However, the API of flush_kernel_dcache_page() ensures that the page is already kmapped when it is called (as in the quoted example above).

Thus, it is already pinned and we can simply look at page_address(). If it is NULL, the page must have been obtained via kmap_atomic() and we don't need to flush anyway since kunmap_atomic() will do this.

The actual flush covering both the lowmem and highmem cases actually is as simple as this (inspired by __flush_dcache_page() in 2.6.32):

	addr = page_address(page);
#ifdef CONFIG_HIGHMEM
	/*
	 * kmap_atomic() doesn't set the page virtual address, and
	 * kunmap_atomic() takes care of cache flushing already.
	 */
	if (addr)
#endif
		__cpuc_flush_dcache_area(addr, PAGE_SIZE);

If you agree that this makes sense, I will send a new version.

If we change the patch this way, there is no need to split off __flush_kernel_dcache_page() from __flush_dcache_page(), which also makes it simpler to apply to past stable kernels.

The only thing we need to be aware of is the change of flush_kernel_dcache_page() in 2.6.37 (commit f8b63c1). In earlier versions, we would only need to fix the highmem case. However, I wonder whether it makes sense to apply a fix to 2.6.32 and 2.6.34 without this ever being reported as a problem.

On Mon, Jun 03, 2013 at 06:38:18PM +0100, Simon Baatz wrote:

Quoted 13 lines

The actual flush covering both the lowmem and highmem cases actually is as simple as this (inspired by __flush_dcache_page() in 2.6.32):

	addr = page_address(page);
#ifdef CONFIG_HIGHMEM
	/*
	 * kmap_atomic() doesn't set the page virtual address, and
	 * kunmap_atomic() takes care of cache flushing already.
	 */
	if (addr)
#endif
		__cpuc_flush_dcache_area(addr, PAGE_SIZE);

Quoted 1 line

If you agree that this makes sense, I will send a new version.

It makes sense (with the addition of the cache_is_vivt() check). Maybe something like

	if (IS_ENABLED(CONFIG_HIGHMEM) && addr)

to avoid the #ifdef.

Quoted 9 lines

If we change the patch this way, there is no need to split off __flush_kernel_dcache_page() from __flush_dcache_page(), which also makes it simpler to apply to past stable kernels.

The only thing we need to be aware of is the change of flush_kernel_dcache_page() in 2.6.37 (commit f8b63c1). In earlier versions, we would only need to fix the highmem case. However, I wonder whether it makes sense to apply a fix to 2.6.32 and 2.6.34 without this ever being reported as a problem.

I think you can cc stable with a dependency for 2.6.37 onwards and send a different patch for earlier versions directly to the stable at vger.kernel.org alias, specifying the versions it needs to be applied to.

+ __flush_kernel_dcache_page(page);

arch/arm/mm/flush.c void flush_dcache_page(struct page *page) · line 323

Quoted 8 lines · Simon Baatz

arch/arm/mm/flush.c +8 -0

+void flush_kernel_dcache_page(struct page *page)
+{
+       if (cache_is_vivt() || cache_is_vipt_aliasing()) {
+               struct address_space *mapping;
+
+               mapping = page_mapping(page);
+
+               if (!mapping || mapping_mapped(mapping))

It is better to replace mapping_mapped(mapping) with page_mapped(page).

Quoted 1 line

+ __flush_kernel_dcache_page(page);

I am wondering if I-cache should be flushed here since the user mapping on the page may be executable and I/D coherency should be maintained.

Quoted 11 lines · Ming Lei

arch/arm/mm/flush.c void flush_dcache_page(struct page *page) · line 323

Quoted 8 lines · Simon Baatz

arch/arm/mm/flush.c +8 -0

+void flush_kernel_dcache_page(struct page *page)
+{
+       if (cache_is_vivt() || cache_is_vipt_aliasing()) {
+               struct address_space *mapping;
+
+               mapping = page_mapping(page);
+
+               if (!mapping || mapping_mapped(mapping))

It is better to replace mapping_mapped(mapping) with page_mapped(page).

Good point. I did not have the time to look into your proposed change for flush_dcache_page() yet.

However, this patch here is a fix for a long standing bug, which should also be applied to stable in one form or another. I would not like to mix it with a separate performance improvement.

But, of course, if we decide to change the respective condition in flush_dcache_page(), we should also change it in flush_kernel_dcache_page() at the same time (provided this patch gets accepted).

Quoted 4 lines

Quoted 1 line

+ __flush_kernel_dcache_page(page);

I am wondering if I-cache should be flushed here since the user mapping on the page may be executable and I/D coherency should be maintained.

Me too. In fact, I wondered so much that I proposed it myself in a previous version of the patch (see [1]) :-) However, this has never been part of flush_kernel_dcache_page() and it is not clear whether the flush for I/D-cache coherency is really needed here (or in flush_dcache_page() for that matter) or whether it is sufficient to do it when mapping the page into user space.

Quoted 7 lines · Simon Baatz

Quoted 5 lines · Ming Lei

Quoted 1 line · Simon Baatz

+ __flush_kernel_dcache_page(page);

I am wondering if I-cache should be flushed here since the user mapping on the page may be executable and I/D coherency should be maintained.

Quoted 6 lines

Me too. In fact, I wondered so much that I proposed it myself in a previous version of the patch (see [1]) :-) However, this has never been part of flush_kernel_dcache_page() and it is not clear whether the flush for I/D-cache coherency is really needed here (or in flush_dcache_page() for that matter) or whether it is sufficient to do it when mapping the page into user space.

After reading cachetlb.txt again, looks your patch is correct, because this kernel API is called in the situations:

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()).

So only flushing kernel mapping for the page is correct if callers of the API obey the rule.