Patchwork [GIT,PULL] Cacheflush updates for 3.12

login
register
mail settings
Submitter Will Deacon
Date Aug. 12, 2013, 5:31 p.m.
Message ID <20130812173155.GI25995@mudshark.cambridge.arm.com>
Download mbox
Permalink /patch/266597/
State New
Headers show

Pull-request

git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git for-rmk/cacheflush

Comments

Will Deacon - Aug. 12, 2013, 5:31 p.m.
Hello again,

Please pull the following user-cacheflush updates for 3.12. This series both
improves performance of cacheflush-heavy workloads (i.e. browser benchmarks)
and also addresses a DoS issue on non-preemptible systems.

Will

--->8

The following changes since commit 3b2f64d00c46e1e4e9bd0bb9bb12619adac27a4b:

  Linux 3.11-rc2 (2013-07-21 12:05:29 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git for-rmk/cacheflush

for you to fetch changes up to d3065541b5a95e5d0ef37e0dc8ada80fd1fba963:

  ARM: cacheflush: don't bother rounding to nearest vma (2013-07-22 10:49:00 +0100)

----------------------------------------------------------------
Will Deacon (4):
      ARM: entry: allow ARM-private syscalls to be restarted
      ARM: cacheflush: split user cache-flushing into interruptible chunks
      ARM: cacheflush: don't round address range up to nearest page
      ARM: cacheflush: don't bother rounding to nearest vma

 arch/arm/include/asm/cacheflush.h  |  3 +--
 arch/arm/include/asm/thread_info.h | 11 +++++++++++
 arch/arm/kernel/entry-common.S     |  4 ++--
 arch/arm/kernel/traps.c            | 64 +++++++++++++++++++++++++++++++++++++++++++++++++---------------
 4 files changed, 63 insertions(+), 19 deletions(-)
Christian Gmeiner - Dec. 4, 2013, 3:37 p.m.
2013/8/12 Will Deacon <will.deacon@arm.com>:
> Hello again,
>
> Please pull the following user-cacheflush updates for 3.12. This series both
> improves performance of cacheflush-heavy workloads (i.e. browser benchmarks)
> and also addresses a DoS issue on non-preemptible systems.
>
> Will
>
> --->8
>
> The following changes since commit 3b2f64d00c46e1e4e9bd0bb9bb12619adac27a4b:
>
>   Linux 3.11-rc2 (2013-07-21 12:05:29 -0700)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git for-rmk/cacheflush
>
> for you to fetch changes up to d3065541b5a95e5d0ef37e0dc8ada80fd1fba963:
>
>   ARM: cacheflush: don't bother rounding to nearest vma (2013-07-22 10:49:00 +0100)
>
> ----------------------------------------------------------------
> Will Deacon (4):
>       ARM: entry: allow ARM-private syscalls to be restarted
>       ARM: cacheflush: split user cache-flushing into interruptible chunks
>       ARM: cacheflush: don't round address range up to nearest page
>       ARM: cacheflush: don't bother rounding to nearest vma
>

Hi all.

I spend the last day running a bisect and I think I have found a problem :)

I have a simple automated test case running, which looks like this:

imx6d based device running X, chromium and x11vnc <----> windows pc connected
via VNC to the device. With this patchset applyed the browser tab
crashed after about
5 minutes hitting the F5/refresh button every 1-3 seconds.

The last good kernel is 3.11.10 and the first bad one is 3.12. This is
the result of my bisect:

root@arm:~/kernel/linux-3.12.2# cat ~/bisect.log
Bisecting: a merge base must be tested
[6e4664525b1db28f8c4e1130957f70a94c19213e] Linux 3.11
Bisecting: 5700 revisions left to test after this (roughly 13 steps)
[cc998ff8811530be521f6b316f37ab7676a07938] Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next
Bisecting: 2850 revisions left to test after this (roughly 12 steps)
[f9121153fdfbfaa930bf65077a5597e20d3ac608] mm/hwpoison: don't need to
hold compound lock for hugetlbfs page
Bisecting: 1428 revisions left to test after this (roughly 11 steps)
[327fff3e1391a27dcc89de6e0481689a865361c9] Merge branch 'misc' of
git://git.kernel.org/pub/scm/linux/kernel/git/mmarek/kbuild
Bisecting: 775 revisions left to test after this (roughly 10 steps)
[8e73e367f7dc50f1d1bc22a63e5764bb4eea9b48] Merge tag
'cleanup-for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc
Bisecting: 247 revisions left to test after this (roughly 8 steps)
[39eda2aba6be642b71f2e0ad623dcb09fd9d79cf] Merge branch 'next' of
git://git.kernel.org/pub/scm/linux/kernel/git/benh/powerpc
Bisecting: 205 revisions left to test after this (roughly 8 steps)
[22e04f6b4b04a8afe9af9239224591d06ba3b24d] Merge branch 'for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid
Bisecting: 110 revisions left to test after this (roughly 7 steps)
[2e032852245b3dcfe5461d7353e34eb6da095ccf] Merge branch 'for-linus' of
git://git.linaro.org/people/rmk/linux-arm
Bisecting: 55 revisions left to test after this (roughly 5 steps)
[5cc91e0460889c8461620904968e193dddb1beb3] Merge branch
'for-rmk/cacheflush-v2' of
git://git.kernel.org/pub/scm/linux/kernel/git/will/linux into
devel-stable
Bisecting: 11 revisions left to test after this (roughly 4 steps)
[6af396a6b6c698eb3834184518fc9a59bc22c817] ARM: cacheflush: use -ishst
dsb variant for ensuring flush completion
Bisecting: 5 revisions left to test after this (roughly 3 steps)
[b4f656eea63376da79b0b5a17660c4ce14b71b74] Pull branch 'for-rmk' of
git://git.linaro.org/people/ardbiesheuvel/linux-arm into devel-stable
Bisecting: 2 revisions left to test after this (roughly 2 steps)
[d9524dc32cab52714dee0c8e59c7437ee33a239a] ARM: cacheflush: don't
round address range up to nearest page
Bisecting: 0 revisions left to test after this (roughly 1 step)
[28256d612726a28a8b9d3c49f2b74198c4423d6a] ARM: cacheflush: split user
cache-flushing into interruptible chunks
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[377747c40657eb35ad98a56439606d96a928425a] ARM: entry: allow
ARM-private syscalls to be restarted
28256d612726a28a8b9d3c49f2b74198c4423d6a is the first bad commit
commit 28256d612726a28a8b9d3c49f2b74198c4423d6a
Author: Will Deacon <will.deacon@arm.com>
Date:   Mon May 13 15:21:49 2013 +0100

    ARM: cacheflush: split user cache-flushing into interruptible chunks

    Flushing a large, non-faulting VMA from userspace can potentially result
    in a long time spent flushing the cache line-by-line without preemption
    occurring (in the case of CONFIG_PREEMPT=n).

    Whilst this doesn't affect the stability of the system, it can certainly
    affect the responsiveness and CPU availability for other tasks.

    This patch splits up the user cacheflush code so that it flushes in
    chunks of a page. After each chunk has been flushed, we may reschedule
    if appropriate and, before processing the next chunk, we allow any
    pending signals to be handled before resuming from where we left off.

    Signed-off-by: Will Deacon <will.deacon@arm.com>

:040000 040000 33ebf747dde46884ce4e7d4ce922fef3cd5b580e
22cdb8a0bc6dc72cb92d93c13ed1a45081269f77 M      arch


If I revert 28256d612726a28a8b9d3c49f2b74198c4423d6a and
97c72d89ce0ec8c73f19d5e35ec1f90f7a14bed7 my "test" runs hours.


What debug options should I enable to get meaningful output from the kernel?


greets
--
Christian Gmeiner, MSc
Will Deacon - Dec. 4, 2013, 4:13 p.m.
On Wed, Dec 04, 2013 at 03:37:36PM +0000, Christian Gmeiner wrote:
> 2013/8/12 Will Deacon <will.deacon@arm.com>:
> > Please pull the following user-cacheflush updates for 3.12. This series both
> > improves performance of cacheflush-heavy workloads (i.e. browser benchmarks)
> > and also addresses a DoS issue on non-preemptible systems.

[...]

> Hi all.

Hello,

> I spend the last day running a bisect and I think I have found a problem :)
> 
> I have a simple automated test case running, which looks like this:
> 
> imx6d based device running X, chromium and x11vnc <----> windows pc connected
> via VNC to the device. With this patchset applyed the browser tab
> crashed after about
> 5 minutes hitting the F5/refresh button every 1-3 seconds.

Hmm... it would be great if we had a simpler way to reproduce this, but ok.
How many cores do you have on your IMX6? Also, how does the browser tab crash?
Does it receive a SIGILL?

> 28256d612726a28a8b9d3c49f2b74198c4423d6a is the first bad commit
> commit 28256d612726a28a8b9d3c49f2b74198c4423d6a
> Author: Will Deacon <will.deacon@arm.com>
> Date:   Mon May 13 15:21:49 2013 +0100
> 
>     ARM: cacheflush: split user cache-flushing into interruptible chunks
> 
>     Flushing a large, non-faulting VMA from userspace can potentially result
>     in a long time spent flushing the cache line-by-line without preemption
>     occurring (in the case of CONFIG_PREEMPT=n).
> 
>     Whilst this doesn't affect the stability of the system, it can certainly
>     affect the responsiveness and CPU availability for other tasks.
> 
>     This patch splits up the user cacheflush code so that it flushes in
>     chunks of a page. After each chunk has been flushed, we may reschedule
>     if appropriate and, before processing the next chunk, we allow any
>     pending signals to be handled before resuming from where we left off.
> 
>     Signed-off-by: Will Deacon <will.deacon@arm.com>

I took another look at that patch and can't see anything obviously wrong
with it. It may, however, be exposing bugs in userspace that you would
struggle to hit before.

> :040000 040000 33ebf747dde46884ce4e7d4ce922fef3cd5b580e
> 22cdb8a0bc6dc72cb92d93c13ed1a45081269f77 M      arch
> 
> 
> If I revert 28256d612726a28a8b9d3c49f2b74198c4423d6a and
> 97c72d89ce0ec8c73f19d5e35ec1f90f7a14bed7 my "test" runs hours.
> 
> 
> What debug options should I enable to get meaningful output from the kernel?

An strace log of the failing case would be good. Another thing you could try
is commenting out the cond_resched in __do_cache_op and see if that helps.

Will
Christian Gmeiner - Dec. 5, 2013, 5:16 p.m.
2013/12/5 Jon Medhurst (Tixy) <tixy@linaro.org>:
> On Thu, 2013-12-05 at 14:23 +0000, Jon Medhurst (Tixy) wrote:
>> On Wed, 2013-12-04 at 16:13 +0000, Will Deacon wrote:
>> > took another look at that patch and can't see anything obviously wrong
>> > with it.
>>
>> If the memory region isn't guaranteed to be page aligned then doesn't it
>> flush up to PAGE_SIZE-1 more bytes than requested and so exceed the
>> bounds check in do_cache_op? Fixing this as below _appears_ to stop the
>> Browser crashes I'm seeing (still doing some more testing)...
>
> Actually, a smaller patch and one which avoids the possibility of
> arithmetic owerflow is...
>
> From: Jon Medhurst <tixy@linaro.org>
> Date: Thu, 5 Dec 2013 14:29:24 +0000
> Subject: [PATCH] ARM: cacheflush: correctly limit range of memory region
>  being flushed
>
> The __do_cache_op function operates with a 'chunk' size of one page
> but fails to limit the size of the final chunk so as to not exceed
> the specified memory region. Fix this.
>
> Signed-off-by: Jon Medhurst <tixy@linaro.org>
> ---
>  arch/arm/kernel/traps.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index dbf0923..7940241 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -509,9 +509,10 @@ static inline int
>  __do_cache_op(unsigned long start, unsigned long end)
>  {
>         int ret;
> -       unsigned long chunk = PAGE_SIZE;
>
>         do {
> +               unsigned long chunk = min(PAGE_SIZE, end - start);
> +
>                 if (signal_pending(current)) {
>                         struct thread_info *ti = current_thread_info();
>
> --
> 1.7.10.4
>
>

I am running my test overnight with this patch... stay tuned.

greets
--
Christian Gmeiner, MSc
Christian Gmeiner - Dec. 6, 2013, 8:05 a.m.
2013/12/5 Christian Gmeiner <christian.gmeiner@gmail.com>:
> 2013/12/5 Jon Medhurst (Tixy) <tixy@linaro.org>:
>> On Thu, 2013-12-05 at 14:23 +0000, Jon Medhurst (Tixy) wrote:
>>> On Wed, 2013-12-04 at 16:13 +0000, Will Deacon wrote:
>>> > took another look at that patch and can't see anything obviously wrong
>>> > with it.
>>>
>>> If the memory region isn't guaranteed to be page aligned then doesn't it
>>> flush up to PAGE_SIZE-1 more bytes than requested and so exceed the
>>> bounds check in do_cache_op? Fixing this as below _appears_ to stop the
>>> Browser crashes I'm seeing (still doing some more testing)...
>>
>> Actually, a smaller patch and one which avoids the possibility of
>> arithmetic owerflow is...
>>
>> From: Jon Medhurst <tixy@linaro.org>
>> Date: Thu, 5 Dec 2013 14:29:24 +0000
>> Subject: [PATCH] ARM: cacheflush: correctly limit range of memory region
>>  being flushed
>>
>> The __do_cache_op function operates with a 'chunk' size of one page
>> but fails to limit the size of the final chunk so as to not exceed
>> the specified memory region. Fix this.
>>
>> Signed-off-by: Jon Medhurst <tixy@linaro.org>
>> ---
>>  arch/arm/kernel/traps.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>> index dbf0923..7940241 100644
>> --- a/arch/arm/kernel/traps.c
>> +++ b/arch/arm/kernel/traps.c
>> @@ -509,9 +509,10 @@ static inline int
>>  __do_cache_op(unsigned long start, unsigned long end)
>>  {
>>         int ret;
>> -       unsigned long chunk = PAGE_SIZE;
>>
>>         do {
>> +               unsigned long chunk = min(PAGE_SIZE, end - start);
>> +
>>                 if (signal_pending(current)) {
>>                         struct thread_info *ti = current_thread_info();
>>
>> --
>> 1.7.10.4
>>
>>
>
> I am running my test overnight with this patch... stay tuned.
>

My test is still running, which is good.

Tested-by: Christian Gmeiner <christian.gmeiner@gmail.com>
Will Deacon - Dec. 6, 2013, 11:50 a.m.
Hi guys,

I was out-of-office yesterday, so sorry for the radio silence.

On Thu, Dec 05, 2013 at 03:14:31PM +0000, Jon Medhurst (Tixy) wrote:
> On Thu, 2013-12-05 at 14:23 +0000, Jon Medhurst (Tixy) wrote:
> > On Wed, 2013-12-04 at 16:13 +0000, Will Deacon wrote:
> > > took another look at that patch and can't see anything obviously wrong
> > > with it. 
> > 
> > If the memory region isn't guaranteed to be page aligned then doesn't it
> > flush up to PAGE_SIZE-1 more bytes than requested and so exceed the
> > bounds check in do_cache_op? Fixing this as below _appears_ to stop the
> > Browser crashes I'm seeing (still doing some more testing)...

Well spotted! We used to round down to a page, but I fixed that in the same
series and didn't notice I'd introduce a bug here. Thanks for digging.

Interesting that we've not heard more reports of this...

> Actually, a smaller patch and one which avoids the possibility of
> arithmetic owerflow is... 
> 
> From: Jon Medhurst <tixy@linaro.org>
> Date: Thu, 5 Dec 2013 14:29:24 +0000
> Subject: [PATCH] ARM: cacheflush: correctly limit range of memory region
>  being flushed
> 
> The __do_cache_op function operates with a 'chunk' size of one page
> but fails to limit the size of the final chunk so as to not exceed
> the specified memory region. Fix this.
> 
> Signed-off-by: Jon Medhurst <tixy@linaro.org>
> ---
>  arch/arm/kernel/traps.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index dbf0923..7940241 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -509,9 +509,10 @@ static inline int
>  __do_cache_op(unsigned long start, unsigned long end)
>  {
>  	int ret;
> -	unsigned long chunk = PAGE_SIZE;
>  
>  	do {
> +		unsigned long chunk = min(PAGE_SIZE, end - start);
> +
>  		if (signal_pending(current)) {
>  			struct thread_info *ti = current_thread_info();

  Acked-by: Will Deacon <will.deacon@arm.com>

Please can you put this into the patch system, along with a CC stable?

Cheers,

Will