| Message ID | CACVXFVPOuabwAa1VDmSnfDG=M-1Df02_kpbm0UVAVctCAB-U4Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, May 04, 2013 at 09:21:27AM +0100, Ming Lei wrote: > On Fri, May 3, 2013 at 6:02 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > >> > >> I assume that you inhibited the call to flush_dcache_page() in > >> __get_user_pages() for anon pages. Otherwise, you will be flooded > >> with warnings. > > > > I haven't done any stress testing so I don't think I hit this code path, > > so no warning. But yes, it should have triggered. Anyway, in this case > > flush_dcache_page() should have just ignored (clearing PG_arch_1 is > > harmless anyway if we also ignore this bit in __sync_icache_dcache for > > non-aliasing caches). > > Yes, maybe we can do a little optimization for O_DIRECT since no > dcache alias and I/D coherency problem in this case on ARMv7, how > about below change? > > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c > index 1c8f7f5..962a657 100644 > --- a/arch/arm/mm/flush.c > +++ b/arch/arm/mm/flush.c > @@ -287,6 +287,8 @@ void flush_dcache_page(struct page *page) > mapping && !mapping_mapped(mapping)) > clear_bit(PG_dcache_clean, &page->flags); > else { > + if (!mapping && cache_is_vipt_nonaliasing()) > + return; > __flush_dcache_page(mapping, page); > if (mapping && cache_is_vivt()) > __flush_dcache_aliases(mapping, page); I wonder whether we could move the: if (!mapping) return at the top of this function, IOW don't touch any anonymous pages. Would anything be broken (apart from wrong API use)?
On Wed, May 08, 2013 at 04:08:00PM +0100, Catalin Marinas wrote: > On Sat, May 04, 2013 at 09:21:27AM +0100, Ming Lei wrote: > > On Fri, May 3, 2013 at 6:02 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > > >> > > >> I assume that you inhibited the call to flush_dcache_page() in > > >> __get_user_pages() for anon pages. Otherwise, you will be flooded > > >> with warnings. > > > > > > I haven't done any stress testing so I don't think I hit this code path, > > > so no warning. But yes, it should have triggered. Anyway, in this case > > > flush_dcache_page() should have just ignored (clearing PG_arch_1 is > > > harmless anyway if we also ignore this bit in __sync_icache_dcache for > > > non-aliasing caches). > > > > Yes, maybe we can do a little optimization for O_DIRECT since no > > dcache alias and I/D coherency problem in this case on ARMv7, how > > about below change? > > > > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c > > index 1c8f7f5..962a657 100644 > > --- a/arch/arm/mm/flush.c > > +++ b/arch/arm/mm/flush.c > > @@ -287,6 +287,8 @@ void flush_dcache_page(struct page *page) > > mapping && !mapping_mapped(mapping)) > > clear_bit(PG_dcache_clean, &page->flags); > > else { > > + if (!mapping && cache_is_vipt_nonaliasing()) > > + return; > > __flush_dcache_page(mapping, page); > > if (mapping && cache_is_vivt()) > > __flush_dcache_aliases(mapping, page); > > I wonder whether we could move the: > > if (!mapping) > return > > at the top of this function, IOW don't touch any anonymous pages. Would > anything be broken (apart from wrong API use)? I suspect you may be missing something - and I suggest reading the Sparc version, which is where this PG_arch_1 stuff was first dreamed up (by DaveM). Are you positive that this path can be eliminated? Totally sure? Have you checked the arg/env pages that fs/exec.c and the binfmt's insert into the new process? The comments against the Sparc version suggest that the "else" clause is necessary ensure that case is adequately covered.
Hi Russel, On Wed, May 08, 2013 at 07:43:54PM +0100, Russell King - ARM Linux wrote: > On Wed, May 08, 2013 at 04:08:00PM +0100, Catalin Marinas wrote: > > On Sat, May 04, 2013 at 09:21:27AM +0100, Ming Lei wrote: > > > On Fri, May 3, 2013 at 6:02 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > >> > > > >> I assume that you inhibited the call to flush_dcache_page() in > > > >> __get_user_pages() for anon pages. Otherwise, you will be flooded > > > >> with warnings. > > > > > > > > I haven't done any stress testing so I don't think I hit this code path, > > > > so no warning. But yes, it should have triggered. Anyway, in this case > > > > flush_dcache_page() should have just ignored (clearing PG_arch_1 is > > > > harmless anyway if we also ignore this bit in __sync_icache_dcache for > > > > non-aliasing caches). > > > > > > Yes, maybe we can do a little optimization for O_DIRECT since no > > > dcache alias and I/D coherency problem in this case on ARMv7, how > > > about below change? > > > > > > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c > > > index 1c8f7f5..962a657 100644 > > > --- a/arch/arm/mm/flush.c > > > +++ b/arch/arm/mm/flush.c > > > @@ -287,6 +287,8 @@ void flush_dcache_page(struct page *page) > > > mapping && !mapping_mapped(mapping)) > > > clear_bit(PG_dcache_clean, &page->flags); > > > else { > > > + if (!mapping && cache_is_vipt_nonaliasing()) > > > + return; > > > __flush_dcache_page(mapping, page); > > > if (mapping && cache_is_vivt()) > > > __flush_dcache_aliases(mapping, page); > > > > I wonder whether we could move the: > > > > if (!mapping) > > return > > > > at the top of this function, IOW don't touch any anonymous pages. Would > > anything be broken (apart from wrong API use)? > > I suspect you may be missing something - and I suggest reading the Sparc > version, which is where this PG_arch_1 stuff was first dreamed up (by > DaveM). > > Are you positive that this path can be eliminated? Totally sure? Have > you checked the arg/env pages that fs/exec.c and the binfmt's insert into > the new process? > > The comments against the Sparc version suggest that the "else" clause > is necessary ensure that case is adequately covered. At least the fs/exec.c and the binfmt stuff use flush_kernel_dcache_page() for 5 years or so now (see commit b6a2fe). Which makes sense, since flush_dcache_page() is not supposed to handle anon pages anyway. The problem are the tons of flush_dcache_page() uses that wrongly assume that no anon pages need to be handled (e.g. drivers/block/loop.c and crypto/scatterwalk.c just to name two). - Simon
On Wed, May 08, 2013 at 08:31:30PM +0100, Simon Baatz wrote: > On Wed, May 08, 2013 at 07:43:54PM +0100, Russell King - ARM Linux wrote: > > On Wed, May 08, 2013 at 04:08:00PM +0100, Catalin Marinas wrote: > > > On Sat, May 04, 2013 at 09:21:27AM +0100, Ming Lei wrote: > > > > On Fri, May 3, 2013 at 6:02 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > >> > > > > >> I assume that you inhibited the call to flush_dcache_page() in > > > > >> __get_user_pages() for anon pages. Otherwise, you will be flooded > > > > >> with warnings. > > > > > > > > > > I haven't done any stress testing so I don't think I hit this code path, > > > > > so no warning. But yes, it should have triggered. Anyway, in this case > > > > > flush_dcache_page() should have just ignored (clearing PG_arch_1 is > > > > > harmless anyway if we also ignore this bit in __sync_icache_dcache for > > > > > non-aliasing caches). > > > > > > > > Yes, maybe we can do a little optimization for O_DIRECT since no > > > > dcache alias and I/D coherency problem in this case on ARMv7, how > > > > about below change? > > > > > > > > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c > > > > index 1c8f7f5..962a657 100644 > > > > --- a/arch/arm/mm/flush.c > > > > +++ b/arch/arm/mm/flush.c > > > > @@ -287,6 +287,8 @@ void flush_dcache_page(struct page *page) > > > > mapping && !mapping_mapped(mapping)) > > > > clear_bit(PG_dcache_clean, &page->flags); > > > > else { > > > > + if (!mapping && cache_is_vipt_nonaliasing()) > > > > + return; > > > > __flush_dcache_page(mapping, page); > > > > if (mapping && cache_is_vivt()) > > > > __flush_dcache_aliases(mapping, page); > > > > > > I wonder whether we could move the: > > > > > > if (!mapping) > > > return > > > > > > at the top of this function, IOW don't touch any anonymous pages. Would > > > anything be broken (apart from wrong API use)? > > > > I suspect you may be missing something - and I suggest reading the Sparc > > version, which is where this PG_arch_1 stuff was first dreamed up (by > > DaveM). > > > > Are you positive that this path can be eliminated? Totally sure? Have > > you checked the arg/env pages that fs/exec.c and the binfmt's insert into > > the new process? > > > > The comments against the Sparc version suggest that the "else" clause > > is necessary ensure that case is adequately covered. > > At least the fs/exec.c and the binfmt stuff use > flush_kernel_dcache_page() for 5 years or so now (see commit b6a2fe). > Which makes sense, since flush_dcache_page() is not supposed to > handle anon pages anyway. Indeed, the sparc comment on arg pages no longer applies. It was also more of an optimisation which I'm not convinced it would have saved anything on ARM.
On Wed, May 8, 2013 at 11:08 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Sat, May 04, 2013 at 09:21:27AM +0100, Ming Lei wrote: >> On Fri, May 3, 2013 at 6:02 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: >> >> >> >> I assume that you inhibited the call to flush_dcache_page() in >> >> __get_user_pages() for anon pages. Otherwise, you will be flooded >> >> with warnings. >> > >> > I haven't done any stress testing so I don't think I hit this code path, >> > so no warning. But yes, it should have triggered. Anyway, in this case >> > flush_dcache_page() should have just ignored (clearing PG_arch_1 is >> > harmless anyway if we also ignore this bit in __sync_icache_dcache for >> > non-aliasing caches). >> >> Yes, maybe we can do a little optimization for O_DIRECT since no >> dcache alias and I/D coherency problem in this case on ARMv7, how >> about below change? >> >> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c >> index 1c8f7f5..962a657 100644 >> --- a/arch/arm/mm/flush.c >> +++ b/arch/arm/mm/flush.c >> @@ -287,6 +287,8 @@ void flush_dcache_page(struct page *page) >> mapping && !mapping_mapped(mapping)) >> clear_bit(PG_dcache_clean, &page->flags); >> else { >> + if (!mapping && cache_is_vipt_nonaliasing()) >> + return; >> __flush_dcache_page(mapping, page); >> if (mapping && cache_is_vivt()) >> __flush_dcache_aliases(mapping, page); > > I wonder whether we could move the: > > if (!mapping) > return > > at the top of this function, IOW don't touch any anonymous pages. Would > anything be broken (apart from wrong API use)? In case of O_DIRECT, the anonymous page may be shared, so looks it should be flushed for vipt_aliasing cache. Catalin, btw, would you mind give comments on the patch below(it might be a 'fix' on one commit you submitted)? http://marc.info/?l=linux-arm-kernel&m=136757247311694&w=2 Thanks
On Sat, May 04, 2013 at 04:21:27PM +0800, Ming Lei wrote: > On Fri, May 3, 2013 at 6:02 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > >> > >> I assume that you inhibited the call to flush_dcache_page() in > >> __get_user_pages() for anon pages. Otherwise, you will be flooded > >> with warnings. > > > > I haven't done any stress testing so I don't think I hit this code path, > > so no warning. But yes, it should have triggered. Anyway, in this case > > flush_dcache_page() should have just ignored (clearing PG_arch_1 is > > harmless anyway if we also ignore this bit in __sync_icache_dcache for > > non-aliasing caches). > > Yes, maybe we can do a little optimization for O_DIRECT since no > dcache alias and I/D coherency problem in this case on ARMv7, how > about below change? > > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c > index 1c8f7f5..962a657 100644 > --- a/arch/arm/mm/flush.c > +++ b/arch/arm/mm/flush.c > @@ -287,6 +287,8 @@ void flush_dcache_page(struct page *page) > mapping && !mapping_mapped(mapping)) > clear_bit(PG_dcache_clean, &page->flags); > else { > + if (!mapping && cache_is_vipt_nonaliasing()) > + return; > __flush_dcache_page(mapping, page); > if (mapping && cache_is_vivt()) > __flush_dcache_aliases(mapping, page); > Yes, this should not be needed. However, I seem to get funny pixel errors on an Exynos 5 based Chromebook when not flushing in this case. Strange. This is not on a mainline kernel, I will need to have a look what happens here. - Simon
On Sat, May 11, 2013 at 2:27 PM, Simon Baatz <gmbnomis@gmail.com> wrote: > On Sat, May 04, 2013 at 04:21:27PM +0800, Ming Lei wrote: >> On Fri, May 3, 2013 at 6:02 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: >> >> >> >> I assume that you inhibited the call to flush_dcache_page() in >> >> __get_user_pages() for anon pages. Otherwise, you will be flooded >> >> with warnings. >> > >> > I haven't done any stress testing so I don't think I hit this code path, >> > so no warning. But yes, it should have triggered. Anyway, in this case >> > flush_dcache_page() should have just ignored (clearing PG_arch_1 is >> > harmless anyway if we also ignore this bit in __sync_icache_dcache for >> > non-aliasing caches). >> >> Yes, maybe we can do a little optimization for O_DIRECT since no >> dcache alias and I/D coherency problem in this case on ARMv7, how >> about below change? >> >> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c >> index 1c8f7f5..962a657 100644 >> --- a/arch/arm/mm/flush.c >> +++ b/arch/arm/mm/flush.c >> @@ -287,6 +287,8 @@ void flush_dcache_page(struct page *page) >> mapping && !mapping_mapped(mapping)) >> clear_bit(PG_dcache_clean, &page->flags); >> else { >> + if (!mapping && cache_is_vipt_nonaliasing()) >> + return; >> __flush_dcache_page(mapping, page); >> if (mapping && cache_is_vivt()) >> __flush_dcache_aliases(mapping, page); >> > > Yes, this should not be needed. However, I seem to get funny pixel > errors on an Exynos 5 based Chromebook when not flushing in this > case. Strange. This is not on a mainline kernel, I will need to > have a look what happens here. Yes, it is a bit strange, not sure how flush_dcache_page() is used in memory mapped by mmap.(suppose the display buffer is mmaped) I test O_DIRECT writing to USB mass storage with the patch, looks the data written is correct. [tom]$dd if=/mnt/nfs/test/usb/qieteing-512M.rmvb of=/mnt/usb/tests/qieteing-512M.rmvb oflag=direct,sync bs=512M count=1 1+0 records in 1+0 records out 536870912 bytes (537 MB) copied, 111.631 s, 4.8 MB/s #echo 3 > /proc/sys/vm/drop_caches [tom]$cd /mnt/usb/tests/ [tom]$md5sum -c /mnt/nfs/test/usb/qieteing-512M.rmvb.md5 qieteing-512M.rmvb: OK Thanks,
diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c index 1c8f7f5..962a657 100644 --- a/arch/arm/mm/flush.c +++ b/arch/arm/mm/flush.c @@ -287,6 +287,8 @@ void flush_dcache_page(struct page *page) mapping && !mapping_mapped(mapping)) clear_bit(PG_dcache_clean, &page->flags); else { + if (!mapping && cache_is_vipt_nonaliasing()) + return; __flush_dcache_page(mapping, page); if (mapping && cache_is_vivt()) __flush_dcache_aliases(mapping, page);