Re: [PATCH] powerpc/paravirt: correct preempt debug splat in vcpu_is_preempted()

[Nathan Lynch <nathanl@linux.ibm.com>]

Michael Ellerman <mpe@ellerman.id.au> writes:
> Nathan Lynch <nathanl@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.