From: gmbnomis@gmail•com (Simon Baatz)
To: linux-arm-kernel@lists•infradead.org
Subject: [RESEND PATCH] ARM: Handle user space mapped pages in flush_kernel_dcache_page
Date: Sat, 28 Jul 2012 22:55:09 +0200 [thread overview]
Message-ID: <20120728205509.GA3676@schnuecks.de> (raw)
In-Reply-To: <20120728112707.GE6802@n2100.arm.linux.org.uk>
Hi Russel,
On Sat, Jul 28, 2012 at 12:27:07PM +0100, Russell King - ARM Linux wrote:
> On Sat, Jul 28, 2012 at 10:41:54AM +0200, Simon Baatz wrote:
> > a while ago I sent the patch above to fix a data corruption problem
> > on VIVT architectures (and probably VIPT aliasing). There has been a
> > bit of discussion with Catalin, but there was no real conclusion on
> > how to proceed. (See
> > http://www.spinics.net/lists/arm-kernel/msg176913.html for the
> > original post)
>
> Going back to that post:
>
> However, this assumption is not true for direct IO or SG_IO (see
> e.g. [1] and [2]). This is why vgchange fails with mv_cesa as reported
> by me in [3]. (Btw., flush_kernel_dcache_page is also called from
> "copy_strings" in fs/exec.c when copying env/arg strings between
> processes. Thus, its use is not restricted to device drivers.)
>
> The calls from copy_strings is not a problem - because the newly allocated
> pages will have PG_arch_1 clear, and when the page is passed to set_pte(),
> it will be flushed.
I was not implying that copy_strings has a problem. But, from
copy_string's comment:
/*
* 'copy_strings()' copies argument/environment strings from the old
* processes's memory to the new process's stack. ...
You said below that flush_kernel_dcache_page() is supposed to be
called for page cache pages only. Hmm, in the new process's stack?
> We _certainly_ do not want to make flush_kernel_dcache_page() do what you're
> doing, because it will mean we're over-flushing *all* pages on VIVT caches.
> Not only will we be flushing them for DMA, but we'll then do it again
> when flush_kernel_dcache_page() is invoked, and then possibly again when
> the page eventually ends up being visible to userspace.
Why should flush_kernel_dcache_page() be invoked at all for DMAed
pages? If you state that this patch over-flushes *all* pages, I
assume that you _certainly_ do not understand the mapping == NULL
case. ;-)
Can a page in the page cache have page->mapping == NULL? If not
page_mapping() only returns NULL in the anon case.
I found this strange myself, but this is the way I thought
flush_dcache_page() handles this. But now I realized it's probably
just a bug in that function, because flush_dcache_page() is not
supposed to handle anon pages at all. (However, it flushes the
kernel mapping currently, since mapping == NULL for these pages.)
If we find out that flush_kernel_dcache_page() needs to handle
anonymous pages, it should do this explicitly.
> > The case is not hit too often apparently; the ingredients are PIO
> > (like) driver, use of flush_kernel_dcache_page(), and direct I/O.
>
> Right, so we need to analyse the direct IO paths and work out whether
> they're using the flush_* interfaces correctly, and even whether they're
> using the correct interfaces.
I agree. I am not claiming that my patch is necessarily correct; this
is a tangled mess for me. But hey, it got the discussion finally
going.
> Note that flush_*dcache_page() are supposed to only be concerned with
> page cache pages, not anonymous pages. flush_anon_page() is all about
> anonymous pages only.
May be, may be not. From Documentation/cachetlb.txt:
void flush_dcache_page(struct page *page)
Any time the kernel writes to a page cache page, _OR_
the kernel is about to read from a page cache page and
user space shared/writable mappings of this page potentially
exist, this routine is called.
void flush_kernel_dcache_page(struct page *page)
When the kernel needs to modify a user page is has obtained
with kmap, it calls this function after all modifications are
complete (but before kunmapping it) to bring the underlying
page up to date.
- Simon
next prev parent reply other threads:[~2012-07-28 20:55 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-07-28 8:41 [RESEND PATCH] ARM: Handle user space mapped pages in flush_kernel_dcache_page Simon Baatz
2012-07-28 11:27 ` Russell King - ARM Linux
2012-07-28 20:55 ` Simon Baatz [this message]
2012-08-13 20:15 ` Simon Baatz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120728205509.GA3676@schnuecks.de \
--to=gmbnomis@gmail$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox