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

[Will Deacon <will.deacon@arm.com>]

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