Michael Ellerman <m...@ellerman.id.au> writes:
> Nathan Lynch <nath...@linux.ibm.com> writes:
>> --- a/arch/powerpc/include/asm/paravirt.h
>> +++ b/arch/powerpc/include/asm/paravirt.h
>> @@ -97,7 +97,14 @@ static inline bool vcpu_is_preempted(int cpu)
>>
>> #ifdef CONFIG_PPC_SPLPAR
>> if (!is_kvm_guest()) {
>> - int first_cpu = cpu_first_thread_sibling(smp_processor_id());
>> + int first_cpu;
>> +
>> + /*
>> + * This is only a guess at best, and this function may be
>> + * called with preemption enabled. Using raw_smp_processor_id()
>> + * does not damage accuracy.
>> + */
>> + first_cpu = cpu_first_thread_sibling(raw_smp_processor_id());
>
> This change seems good, except I think the comment needs to be a lot
> more explicit about what it's doing and why.
>
> A casual reader is going to be confused about vcpu preemption vs
> "preemption", which are basically unrelated yet use the same word.
>
> It's not clear how raw_smp_processor_id() is related to (Linux)
> preemption, unless you know that smp_processor_id() is the alternative
> and it contains a preemption check.
>
> And "this is only a guess" is not clear on what *this* is, you're
> referring to the result of the whole function, but that's not obvious.
You're right.
>
>> /*
>> * Preemption can only happen at core granularity. This CPU
> ^^^^^^^^^^
> Means something different to "preemption" above.
>
> I know you didn't write that comment, and maybe we need to rewrite some
> of those existing comments to make it clear they're not talking about
> Linux preemption.
Thanks, agreed on all points. I'll rework the existing comments and any
new ones to clearly distinguish between the two senses of preemption
here.
