lkml.org 
[lkml]   [2018]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH RT v3] arm64: fpsimd: use preemp_disable in addition to local_bh_disable()
    On 2018-07-27 16:35:59 [+0100], Dave Martin wrote:
    > On Thu, Jul 26, 2018 at 05:06:34PM +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 preempt_disable()/enable() to enfore the required semantic on -RT.
    >
    > Does this supersede the local_lock based approach?

    Yes, it does.

    > That would have been nice to have, but if there are open questions about
    > how to do it then I guess something like this patch makes sense as a
    > stopgap solution.
    >
    > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
    > > ---
    > > This should work. Compiling currently gcc-6 on the box to see what
    > > happens. Since the crypto disables preemption "frequently" and I don't
    > > expect or see anything to worry about.
    > >
    > > arch/arm64/kernel/fpsimd.c | 30 ++++++++++++++++++++++++++++--
    > > 1 file changed, 28 insertions(+), 2 deletions(-)
    > >
    > > --- a/arch/arm64/kernel/fpsimd.c
    > > +++ b/arch/arm64/kernel/fpsimd.c
    > > @@ -157,6 +157,15 @@ static void sve_free(struct task_struct
    > > __sve_free(task);
    > > }
    > >
    > > +static void *sve_free_atomic(struct task_struct *task)
    > > +{
    > > + void *sve_state = task->thread.sve_state;
    > > +
    > > + WARN_ON(test_tsk_thread_flag(task, TIF_SVE));
    > > +
    > > + task->thread.sve_state = NULL;
    > > + return sve_state;
    > > +}
    >
    > This feels a bit excessive. Since there's only one call site, I'd
    > prefer if the necessary code were simply inlined. We wouldn't need the
    > WARN either in that case, since (IIUC) it's only there to check for
    > accidental misuse of this helper.
    okay.

    > > /* Offset of FFR in the SVE register dump */
    > > static size_t sve_ffr_offset(int vl)

    > I think we should have local helpers for the preempt+local_bh
    > maintenance, since they're needed all over the place in this
    > file.
    okay.

    > Cheers
    > ---Dave

    Sebastian

    \
     
     \ /
      Last update: 2018-07-27 18:27    [W:6.787 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site