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

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

On Thu, May 09, 2019 at 03:01:43PM +0000, Hillf Danton wrote:

Your mailer did something funny here and send a multipart MIME message.

If in doubt, use git send-email.

> The comment was a bit misleading when it was created in commit 24534b3511, and
> deserves a tweak like,
> 
> - * - 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 code fix was added in commit 7faa313f05 with the comment left behind untouched.
> 
> It no longer matches the code now, so fix it. It is changed along the original
> direction as much as I can, though simply deleting the relevant block looks fine.
> 
> And finally a question remains: is it needed to decrement preempt count before
> invoking kernel_neon_end() in which preempt_enable() is put at the end?

Nit: please follow the recommendations in
Documentation/process/submitting-patches.rst regarding formatting commit
messages.

(Or run scripts/checkpatch.pl.)

> 
> 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, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index c5308d0..8518a7b 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -713,13 +713,9 @@ USER(\label, ic	ivau, \tmp2)			// invalidate I line PoU
>   * Note that the patchup code does not support assembler directives that change
>   * 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.
> + * 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.

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?

Cheers
---Dave

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