| Message ID | 177247.865216003-sendEmail@laptop-0p1i5f25 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: assembler: Update the yield NEON comment | expand |
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
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.
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(-) --