Re: [PATCH 4/4] powerpc/pseries: warn if recursing into the hcall tracing code

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

Nicholas Piggin wrote:
> Excerpts from Naveen N. Rao's message of May 4, 2021 8:25 pm:
>> Nicholas Piggin wrote:
>>> Excerpts from Naveen N. Rao's message of April 27, 2021 11:59 pm:
>>>> Nicholas Piggin wrote:
>>>>> + *
>>>>> + * H_CONFER from spin locks must be treated separately though and use _notrace
>>>>> + * plpar_hcall variants, see yield_to_preempted().
>>>>>   */
>>>>>  static DEFINE_PER_CPU(unsigned int, hcall_trace_depth);
>>>>> 
>>>>> @@ -1843,7 +1846,7 @@ notrace void __trace_hcall_entry(unsigned long opcode, unsigned long *args)
>>>>> 
>>>>>  	depth = this_cpu_ptr(&hcall_trace_depth);
>>>>> 
>>>>> -	if (*depth)
>>>>> +	if (WARN_ON_ONCE(*depth))
>>>>>  		goto out;
>>>> 
>>>> I don't think this will be helpful. The hcall trace depth tracking is 
>>>> for the tracepoint and I suspect that this warning will be triggered 
>>>> quite easily. Since we have recursion protection, I don't think we 
>>>> should warn here.
>>> 
>>> What would trigger recursion?
>> 
>> The trace code that this protects: trace_hcall_entry(). The tracing code 
>> itself can end up doing a hcall as we see in the first patch in this 
>> series:
>>   plpar_hcall_norets_trace+0x34/0x8c (unreliable)
>>   __pv_queued_spin_lock_slowpath+0x684/0x710
>>   trace_clock_global+0x148/0x150
>>   ring_buffer_lock_reserve+0x12c/0x630
>>   trace_event_buffer_lock_reserve+0x80/0x220
>>   trace_event_buffer_reserve+0x7c/0xd0
>>   trace_event_raw_event_hcall_entry+0x68/0x150
>>   __trace_hcall_entry+0x160/0x180
>> 
>> 
>> There is also a comment aroung hcall_trace_depth that mentions this:
>> 
>>   /*
>>    * Since the tracing code might execute hcalls we need to guard against
>>    * recursion. One example of this are spinlocks calling H_YIELD on
>>    * shared processor partitions.
>>    */
> 
> Right but since fixing those, my thought is we better not cause more
> any recursion, so we should fix anything that does.

Ah ok, I got confused by the reference to recursion, since the current 
fix did not involve hcall trace recursion per se.

Yes, with the current patch set, we have ensured that at least the 
qspinlock code doesn't invoke tracing if it has to do H_CONFER hcall. I 
am not entirely sure if that's the only hcall that could be invoked by 
the tracing code.

The reason I felt that this warning isn't useful is because it can be 
triggered by the perf NMI -- if the perf NMI happens while we are 
tracing a different hcall.


Thanks,
Naveen