Re: [patch] arm64: assembler: Update the yield NEON comment

[Dave Martin <Dave.Martin@arm.com>]

On Sat, May 11, 2019 at 09:48:40AM +0100, Hillf Danton wrote:
> > On Thu, May 09, 2019, Dave Martin wrote:
> > > On Thu, May 09, 2019 at 03:01:43PM +0000, Hillf Danton wrote:
> > 
> [...]
> 
> > >   *
> > > - * The yield itself consists of the following:
> > > - * - Check whether the preempt count is exactly 1, in which case disabling
> > > - *   preemption once will make the task preemptible. If this is not the case,
> > > - *   yielding is pointless.
> > > - * - Check whether TIF_NEED_RESCHED is set, and if so, disable and re-enable
> > > - *   kernel mode NEON (which will trigger a reschedule), and branch to the
> > > - *   yield fixup code.
> > > + * The yield itself decrements the preempt count and if count hits zero, disable
> > > + * and re-enable kernel mode NEON (which will trigger a reschedule), and branch
> > > + * to the yield fixup code.
> > 
> > Good spot -- the original comment is definitely wrong: disabling
> > preemption certainly shouldn't make the task preemptible!
> > 
> > So, I'm certainly in favour of fixing that.
> > 
> > Your new text doesn't look right, though: AFAICT commit 7faa313f05
> > didn't intentionally change the behaviour here.  It looks to me like
> > the preempt count (i.e., current_thread_info()->preempt) is not
> > decremented by this code: the value is read and the subtraction is done
> > for comparison purposes, but no value is stored back.
> > 
> Got it, thanks.
> 
> > This is intentional, because the the code needs to do some cleanup
> > before preemption can be enabled for real: the call to kernel_neon_end
> > does both of those jobs.
> > 
> > Or did I miss something?
> > 
> No, you didn't, Sir.
> 
> And I try again with log message updated and comment text corrected.
> 
> Hillf
> ----------------->8-----------------------------------------------------------
> From: Hillf Danton <hdanton@sina.com>
> Subject: [patch] arm64: assembler: Update the yield NEON comment
> 
> The comment for specifying preempt count check was incorrect in commit
> 24534b3511, and deserves a tiny tweak like the following for instance.
> 
> - * - Check whether the preempt count is exactly 1, in which case disabling
> - *   preemption once will make the task preemptible. If this is not the case,
> + * - Check whether the preempt count is exactly 1, in which case decrementing
> + *   preempt count once will make the task preemptible. If this is not the case,
> 
> Then checking of TIF_NEED_RESCHED was removed in commit 7faa313f05, with the
> comment left behind untouched.

Hmm, you're right here, I hadn't fully understood the context of that
commit.

Will, Ard, can you comment on the rationale for that?  The commit
message says nothing explicit about why the TIF_NEED_RESCHED check went
away: it's now checked as part of the preempt count, right?

> It no longer matches the code now, so update it.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Dave Martin <Dave.Martin@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Hillf Danton <hdanton@sina.com>
> ---
>  arch/arm64/include/asm/assembler.h | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index c5308d0..9945ca4 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -714,12 +714,10 @@ USER(\label, ic	ivau, \tmp2)			// invalidate I line PoU
>   * the output section, any use of such directives is undefined.
>   *
>   * The yield itself consists of the following:
> - * - Check whether the preempt count is exactly 1, in which case disabling
> - *   preemption once will make the task preemptible. If this is not the case,
> - *   yielding is pointless.
> - * - Check whether TIF_NEED_RESCHED is set, and if so, disable and re-enable
> - *   kernel mode NEON (which will trigger a reschedule), and branch to the
> - *   yield fixup code.
> + * - Check whether the preempt count is exactly 1, in which case the calling of
> + *   preempt_enable() in kernel_neon_end() will trigger a reschedule. If it is
> + *   not the case, yielding is pointless.

Looking at the code, this should probably say something like

	Check whether the preempt count is exactly 1 and a reschedule
	is also needed.  If so, calling [...]

It's the fact that the preempt count and the "needs reschedule" flag are
compared as a single number that confused me here.

> + * - Disable and re-enable kernel mode NEON, and branch to the yield fixup code.

Otherwise, looks good.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel