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

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

[Dave Martin]

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

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

[Will Deacon]

On Mon, May 13, 2019 at 09:56:26AM +0100, Dave Martin wrote:
> 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?

Yes. The value of !need_resched is stored adjacent to the preempt count,
which is why 7faa313f05ca ("arm64: preempt: Fix big-endian when checking
preempt count in assembly") can load both of the variables using a single
64-bit LDR instruction.

So we continue to check both the preempt count and the need_resched flag,
as before.

Will

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

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

[Hillf Danton]

[-- Attachment #1.1: Type: text/plain, Size: 2312 bytes --]

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?

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.
  *
  * This macro sequence may clobber all CPU state that is not guaranteed by the
  * AAPCS to be preserved across an ordinary function call.
--


[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

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

[Dave Martin]

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