On Thu, May 24, 2018 at 10:33:50AM +0200, Christoffer Dall wrote:
> On Wed, May 23, 2018 at 04:03:37PM +0100, Dave Martin wrote:
> > On Wed, May 23, 2018 at 03:56:57PM +0100, Catalin Marinas wrote:
> > > On Wed, May 23, 2018 at 02:31:59PM +0100, Dave P Martin wrote:
> > > > On Wed, May 23, 2018 at 01:48:12PM +0200, Christoffer Dall wrote:
> > > > > On Tue, May 22, 2018 at 05:05:08PM +0100, Dave Martin wrote:
> > > > > > This is true by construction however: TIF_FOREIGN_FPSTATE is never
> > > > > > cleared except when returning to userspace or returning from a
> > > > > > signal: thus, for a true kernel thread no FPSIMD context is ever
> > > > > > loaded, TIF_FOREIGN_FPSTATE will remain set and no context will
> > > > > > ever be saved.
> > > > >
> > > > > I don't understand this construction proof; from looking at the patch
> > > > > below it is not obvious to me why fpsimd_thread_switch() can never have
> > > > > !wrong_task && !wrong_cpu and therefore clear TIF_FOREIGN_FPSTATE for a
> > > > > kernel thread?
> > > >
> > > > Looking at this again, I think it is poorly worded. This patch aims to
> > > > make it true by construction, but it isn't prior to the patch.
> > > >
> > > > I'm tempted to delete the paragraph: the assertion of both untrue and
> > > > not the best way to justify that this patch works.
> > > >
> > > >
> > > > How about:
> > > >
> > > > -8<-
> > > >
> > > > The context switch logic already isolates user threads from each other.
> > > > This, it is sufficient for isolating user threads from the kernel,
>
> s/This/Thus/ ?
>
> I don't understand what 'it' refers to here?
>
> > > > since the goal either way is to ensure that code executing in userspace
> > > > cannot see any FPSIMD state except its own. Thus, there is no special
> > > > property of kernel threads that we care about except that it is
> > > > pointless to save or load FPSIMD register state for them.
>
> Actually, I'm not really sure what this paragraph is getting at.
Reading this again, I don't think the paragraph adds much useful.
So I propose deleting that too.
> > > >
> > > > At worst, the removal of all the kernel thread special cases by this
> > > > patch would thus spuriously load and save state for kernel threads when
> > > > unnecessary.
> > > >
> > > > But the context switch logic is already deliberately optimised to defer
> > > > reloads of the regs until ret_to_user (or sigreturn as a special case),
> > > > which kernel threads by definition never reach.
> > > >
> > > > ->8-
> > >
> > > The "at worst" paragraph makes it look like it could happen (at least
> > > until you reach the last paragraph). Maybe you can just say that
> > > wrong_task and wrong_cpu (with the fpsimd_cpu = NR_CPUS addition) are
> > > always true for kernel threads. You should probably mention this in a
> > > comment in the code as well.
> >
> > What if I just delete the second paragraph, and remove the "But" from
> > the start of the third, and append:
> >
> > "As a result, the wrong_task and wrong_cpu tests in
> > fpsimd_thread_switch() will always yield false for kernel threads."
> >
> > ...with a similar comment in the code?
>
> ...with a risk of being a bit over-pedantic and annoying, may I suggest
> the following complete commit text:
>
> ------8<------
> Currently the FPSIMD handling code uses the condition task->mm ==
> NULL as a hint that task has no FPSIMD register context.
>
> The ->mm check is only there to filter out tasks that cannot
> possibly have FPSIMD context loaded, for optimisation purposes.
> However, TIF_FOREIGN_FPSTATE must always be checked anyway before
> saving FPSIMD context back to memory. For this reason, the ->mm
> checks are not useful, providing that that TIF_FOREIGN_FPSTATE is
> maintained properly for kernel threads.
>
> FPSIMD context is never preserved for kernel threads across a context
> switch and therefore TIF_FOREIGN_FPSTATE should always be true for
(This refactoring opens up the interesting possibility of making
kernel-mode NEON in task context preemptible for kernel threads so
that we actually do preserve state... but that's a discussion for
another day. There may be code around that relies on
kernel_neon_begin() disabling preemption for real.)
> kernel threads. This is indeed the case, as the wrong_task and
This suggests that TIF_FOREIGN_FPSTATE is always true for kernel
threads today. This is not quite because use_mm() can make mm non-
NULL.
> wrong_cpu tests in fpsimd_thread_switch() will always yield false for
> kernel threads.
("false" -> "true". My bad.)
> Further, the context switch logic is already deliberately optimised to
> defer reloads of the FPSIMD context until ret_to_user (or sigreturn as a
> special case), which kernel threads by definition never reach, and
> therefore this change introduces no additional work in the critical
> path.
>
> This patch removes the redundant checks and special-case code.
> ------8<------
Looking at my existing text, I rather reworded it like this.
Does this work any better for you?
--8<--
Currently the FPSIMD handling code uses the condition task->mm ==
NULL as a hint that task has no FPSIMD register context.
The ->mm check is only there to filter out tasks that cannot
possibly have FPSIMD context loaded, for optimisation purposes.
Also, TIF_FOREIGN_FPSTATE must always be checked anyway before
saving FPSIMD context back to memory. For these reasons, the ->mm
checks are not useful, providing that TIF_FOREIGN_FPSTATE is
maintained in a consistent way for kernel threads.
The context switch logic is already deliberately optimised to defer
reloads of the regs until ret_to_user (or sigreturn as a special
case), and save them only if they have been previously loaded.
Kernel threads by definition never reach these paths. As a result,
the wrong_task and wrong_cpu tests in fpsimd_thread_switch() will
always yield true for kernel threads.
This patch removes the redundant checks and special-case code, ensuring that TIF_FOREIGN_FPSTATE is set whenever a kernel thread is scheduled in, and ensures that this flag is set for the init
task. The fpsimd_flush_task_state() call already present in copy_thread() ensures the same for any new task.
With TIF_FOREIGN_FPSTATE always set for kernel threads, this patch
ensures that no extra context save work is added for kernel
threads, and eliminates the redundant context saving that may
currently occur for kernel threads that have acquired an mm via
use_mm().
-->8--
Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
