Re: [PATCH 2/3] ARM: Move thread_info into task_struct (v7 only)

[Keith Packard]

Ard Biesheuvel <ardb@xxxxxxxxxx> writes:

> c13 is not a register, it is a value in one of the dimensions of the
> ARM sysreg space, and probably covers other system registers that
> pre-v7 cores do implement.

> Better to use its architectural name (TPIDRPRW) and clarify that
> pre-v6k/v7 cores simply don't implement it.

Thanks, I'll reword the commit message

> Could we split this up?

I could split up the assembler macro changes which add a temporary
register to the calls getting the current thread_info/task_struct value?
If that would help get this reviewed, I'd be happy to do
that. Otherwise, it's pretty much all-or-nothing as enabling
THREAD_INFO_IN_TASK requires a bunch of synchronized changes.

>> +#ifdef CONFIG_THREAD_INFO_IN_TASK
>
> No need for this #ifdef - it only guards macros that will have no
> effect if they are never instantiated (another case below)

Sure, the resulting code is a bit less noisy, which seems good.

>> +DECLARE_PER_CPU(struct task_struct *, current_task);
>> +
>> +static __always_inline struct task_struct *get_current(void)
>> +{
>> +       return raw_cpu_read(current_task);
>
> This needs to run with preemption disabled, or we might get migrated
> to another CPU halfway through, and produce the wrong result (i.e.,
> the current task of the CPU we migrated from). However, it appears
> that manipulating the preempt count will create a circular dependency
> here.

Yeah, I really don't understand the restrictions of this API well. Any
code fetching the current task pointer better not be subject to
preemption or that value will be stale at some point after it was
computed. Do you know if it could ever be run in a context allowing
preemption?
>
> Instead of using a per-CPU variable for current, I think it might be
> better to borrow another system register (TPIDRURO or TPIDRURW) to
> carry 'current' when THREAD_INFO_IN_TASK is in effect, similar to how
> arm64 uses SP_EL0 - those registers could be preserved/restored in the
> entry/exit from/to user space paths rather than in the context switch
> path, and then be used any way we like while running in the kernel.

Making sure these values don't leak through to user space somehow? I'm
not excited by that prospect at all. But, perhaps someone can help me
understand the conditions under which the current task value can be
computed where preemption was enabled and have that not be a problem for
the enclosing code?

-- 
-keith

Attachment: signature.asc
Description: PGP signature