[PATCH v2 5/7] ARM: switch_to: clean up Thumb2 code path

[Ard Biesheuvel]

On Mon, 18 Oct 2021 at 18:47, Nicolas Pitre <nico at fluxnic.net> wrote:
>
> On Mon, 18 Oct 2021, Ard Biesheuvel wrote:
>
> > On Mon, 18 Oct 2021 at 17:37, Nicolas Pitre <nico at fluxnic.net> wrote:
> > >
> > > On Mon, 18 Oct 2021, Ard Biesheuvel wrote:
> > >
> > > > The load-multiple instruction that essentially performs the switch_to
> > > > operation in ARM mode, by loading all callee save registers as well the
> > > > stack pointer and the program counter, is split into 3 separate loads
> > > > for Thumb-2, with the IP register used as a temporary to capture the
> > > > value of R4 before it gets overwritten.
> > > >
> > > > We can clean this up a bit, by sticking with a single LDMIA instruction,
> > > > but one that pops SP and PC into IP and LR, respectively, and by using
> > > > ordinary move register and branch instructions to get those values into
> > > > SP and PC. This also allows us to move the set_current call closer to
> > > > the assignment of SP, reducing the window where those are mutually out
> > > > of sync. This is especially relevant for CONFIG_VMAP_STACK, which is
> > > > being introduced in a subsequent patch, where we need to issue a load
> > > > that might fault from the new stack while running from the old one, to
> > > > ensure that stale PMD entries in the VMALLOC space are synced up.
> > >
> > > What could happen if they are of sync? The window is small but still.
> > >
> >
> > It would mean 'current' as well as 'current_thread_info' would point
> > to the new task while running on the old stack, whereas without
> > THREAD_INFO_IN_TASK, those are guaranteed to be in sync.
> >
> > I think it probably doesn't make lots of difference, although after
> > the next patch, we may take a fault in this code path on a stale PMD
> > entry, and I suppose the accounting etc would be affected.
> >
> > In any case, this is something Russell brought up in the review of the
> > THREAD_INFO_IN_TASK series, and I agree that it's best to keep these
> > in sync as much as we can.
>
> If this happens only once in a blue moon and the consequence is benign
> (like wrong accounting for one perf sample or the like) then this is
> probably OK. However it would be a good idea to document it in the code
> and not only in the commit log.
>

Fair enough. I'll add something like

"""
When CONFIG_THREAD_INFO_IN_TASK=n, the update of SP itself is what
effectuates the task switch, as that is what causes the observable
values of current and current_thread_info to change. When
CONFIG_THREAD_INFO_IN_TASK=y, setting current (and therefore
current_thread_info) is done explicitly, and the update of SP just
switches us to another stack, with few other side effects. In order to
prevent this distinction from causing any inconsistencies, let's keep
the 'set_current' call as close as we can to the update of SP.
"""