linux-rt-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mike Galbraith <efault@gmx.de>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Steven Rostedt <rostedt@goodmis.org>
Cc: linux-rt-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	tglx@linutronix.de, Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH RT v2] arm64: fpsimd: use a local_lock() in addition to local_bh_disable()
Date: Sat, 14 Jul 2018 00:03:44 +0200	[thread overview]
Message-ID: <1531519424.23898.68.camel@gmx.de> (raw)
In-Reply-To: <20180713174937.5ddaqpylalcmc3jq@linutronix.de>

On Fri, 2018-07-13 at 19:49 +0200, Sebastian Andrzej Siewior wrote:
> In v4.16-RT I noticed a number of warnings from task_fpsimd_load(). The
> code disables BH and expects that it is not preemptible. On -RT the
> task remains preemptible but remains the same CPU. This may corrupt the
> content of the SIMD registers if the task is preempted during
> saving/restoring those registers.
> Add a locallock around this process. This avoids that the any function
> within the locallock block is invoked more than once on the same CPU.
> 
> The kernel_neon_begin() can't be kept preemptible. If the task-switch notices
> TIF_FOREIGN_FPSTATE then it would restore task's SIMD state and we lose the
> state of registers used for in-kernel-work. We would require additional storage
> for the in-kernel copy of the registers. But then the NEON-crypto checks for
> the need-resched flag so it shouldn't that bad.
> The preempt_disable() avoids the context switch while the kernel uses the SIMD
> registers. Unfortunately we have to balance out the migrate_disable() counter
> because local_lock_bh() is invoked in different context compared to its unlock
> counterpart.
> 
> __efi_fpsimd_begin() should not use kernel_fpu_begin() due to its
> preempt_disable() context and instead save the registers always in its
> extra spot on RT.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> 
> This seems to make work (crypto chacha20-neon + cyclictest). I have no
> EFI so I have no clue if saving SIMD while calling to EFI works.

All is not well on cavium test box.  I'm seeing random errors ala...

./include/linux/fs.h:3137:11: internal compiler error: Segmentation fault
./include/linux/bio.h:175:1: internal compiler error: in grokdeclarator, at c/c-decl.c:7023

...during make -j96 (2*cpus) kbuild.  Turns out 4.14-rt has this issue
as well, which is unsurprising if it's related to fpsimd woes.  Box
does not exhibit the issue with NONRT kernels, PREEMPT or NOPREEMPT.

To file under FWIW, arm64 configured SLE15-RT, 4.12 based kernel
containing virgin @stable arch/arm64/kernel/fpsimd.c, does not exhibit
the problem. (relevant? dunno, it may be unrelated to fpsimd.c).

	-Mike

  parent reply	other threads:[~2018-07-13 22:03 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-17 12:40 [PATCH RT] arm64: fpsimd: use a local_lock() in addition to local_bh_disable() Sebastian Andrzej Siewior
2018-05-17 18:19 ` Dave Martin
2018-05-18 12:46   ` Dave Martin
2018-05-23 14:34     ` Sebastian Andrzej Siewior
2018-05-23 14:31   ` Sebastian Andrzej Siewior
2018-05-23 14:55     ` Dave Martin
2018-05-22 17:10 ` Steven Rostedt
2018-05-22 17:21   ` Sebastian Andrzej Siewior
2018-05-22 17:24     ` Steven Rostedt
2018-05-22 17:33       ` Sebastian Andrzej Siewior
2018-07-11 13:25         ` Steven Rostedt
2018-07-11 13:31           ` Sebastian Andrzej Siewior
2018-07-11 13:33             ` Steven Rostedt
2018-07-13 17:49               ` [PATCH RT v2] " Sebastian Andrzej Siewior
2018-07-13 17:50                 ` [PATCH RT] locallock: add local_lock_bh() Sebastian Andrzej Siewior
2018-07-13 22:03                 ` Mike Galbraith [this message]
2018-07-15  7:22                   ` [PATCH RT v2] arm64: fpsimd: use a local_lock() in addition to local_bh_disable() Mike Galbraith
2018-07-18 10:30                     ` Mike Galbraith
2018-07-18  9:27                   ` Sebastian Andrzej Siewior
2018-07-18 10:28                     ` Mike Galbraith
2018-07-18 10:36                       ` Sebastian Andrzej Siewior
2018-07-16 15:17                 ` Dave Martin
2018-07-18  9:12                   ` Sebastian Andrzej Siewior
2018-07-18  9:24                     ` Sebastian Andrzej Siewior
2018-07-24 14:45                       ` Dave Martin
2018-07-24 15:15                         ` Ard Biesheuvel
2018-07-24 13:46                     ` Steven Rostedt
2018-07-24 13:57                       ` Sebastian Andrzej Siewior
2018-07-26 15:06                       ` [PATCH RT v3] arm64: fpsimd: use preemp_disable " Sebastian Andrzej Siewior
2018-07-27  3:17                         ` Mike Galbraith
2018-07-27  7:56                           ` Sebastian Andrzej Siewior
2018-07-27 15:35                         ` Dave Martin
2018-07-27 16:26                           ` Sebastian Andrzej Siewior
2018-07-11 17:07             ` [PATCH RT] arm64: fpsimd: use a local_lock() " Mike Galbraith

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=1531519424.23898.68.camel@gmx.de \
    --to=efault@gmx.de \
    --cc=bigeasy@linutronix.de \
    --cc=catalin.marinas@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=will.deacon@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).