diff mbox series

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

Message ID 20210423031108.1046067-5-npiggin@gmail.com (mailing list archive)
State Accepted
Headers show
Series Fix queued spinlocks and a bit more | expand
Related show

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (d20f726744a0312b4b6613333bda7da9bc52fb75)
snowpatch_ozlabs/build-ppc64le success Build succeeded
snowpatch_ozlabs/build-ppc64be success Build succeeded
snowpatch_ozlabs/build-ppc64e success Build succeeded
snowpatch_ozlabs/build-pmac32 success Build succeeded
snowpatch_ozlabs/checkpatch warning total: 1 errors, 1 warnings, 0 checks, 29 lines checked
snowpatch_ozlabs/needsstable success Patch has no Fixes tags

Commit Message

Nicholas Piggin April 23, 2021, 3:11 a.m. UTC 
---
 arch/powerpc/platforms/pseries/lpar.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Naveen N. Rao April 27, 2021, 1:59 p.m. UTC | #1 
Nicholas Piggin wrote:
> ---
>  arch/powerpc/platforms/pseries/lpar.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
> index 835e7f661a05..a961a7ebeab3 100644
> --- a/arch/powerpc/platforms/pseries/lpar.c
> +++ b/arch/powerpc/platforms/pseries/lpar.c
> @@ -1828,8 +1828,11 @@ void hcall_tracepoint_unregfunc(void)
> 
>  /*
>   * Since the tracing code might execute hcalls we need to guard against
> - * recursion. H_CONFER from spin locks must be treated separately though
> - * and use _notrace plpar_hcall variants, see yield_to_preempted().
> + * recursion, but this always seems risky -- __trace_hcall_entry might be
> + * ftraced, for example. So warn in this case.

__trace_hcall_[entry|exit] aren't traced anymore since they now have the 
'notrace' annotation.

> + *
> + * 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.


- Naveen
Nicholas Piggin May 1, 2021, 1:24 a.m. UTC | #2 
Excerpts from Naveen N. Rao's message of April 27, 2021 11:59 pm:
> Nicholas Piggin wrote:
>> ---
>>  arch/powerpc/platforms/pseries/lpar.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
>> index 835e7f661a05..a961a7ebeab3 100644
>> --- a/arch/powerpc/platforms/pseries/lpar.c
>> +++ b/arch/powerpc/platforms/pseries/lpar.c
>> @@ -1828,8 +1828,11 @@ void hcall_tracepoint_unregfunc(void)
>> 
>>  /*
>>   * Since the tracing code might execute hcalls we need to guard against
>> - * recursion. H_CONFER from spin locks must be treated separately though
>> - * and use _notrace plpar_hcall variants, see yield_to_preempted().
>> + * recursion, but this always seems risky -- __trace_hcall_entry might be
>> + * ftraced, for example. So warn in this case.
> 
> __trace_hcall_[entry|exit] aren't traced anymore since they now have the 
> 'notrace' annotation.

Yes that's true I went back and added the other patch, so I should fix 
this comment.

>> + *
>> + * 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?

Thanks,
Nick
Naveen N. Rao May 4, 2021, 10:25 a.m. UTC | #3 
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.
   */


Thanks,
Naveen
Nicholas Piggin May 4, 2021, 10:45 a.m. UTC | #4 
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.

Thanks,
Nick
Naveen N. Rao May 4, 2021, 4:18 p.m. UTC | #5 
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
diff mbox series

Patch

diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
index 835e7f661a05..a961a7ebeab3 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -1828,8 +1828,11 @@  void hcall_tracepoint_unregfunc(void)
 
 /*
  * Since the tracing code might execute hcalls we need to guard against
- * recursion. H_CONFER from spin locks must be treated separately though
- * and use _notrace plpar_hcall variants, see yield_to_preempted().
+ * recursion, but this always seems risky -- __trace_hcall_entry might be
+ * ftraced, for example. So warn in this case.
+ *
+ * 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;
 
 	(*depth)++;
@@ -1864,7 +1867,7 @@  notrace void __trace_hcall_exit(long opcode, long retval, unsigned long *retbuf)
 
 	depth = this_cpu_ptr(&hcall_trace_depth);
 
-	if (*depth)
+	if (*depth) /* Don't warning again on the way out */
 		goto out;
 
 	(*depth)++;