Re: [RFC PATCH v3 09/58] perf: Add a EVENT_GUEST flag

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2024-10-11 7:41 a.m., Peter Zijlstra wrote:
> On Wed, Aug 21, 2024 at 01:27:17PM +0800, Mi, Dapeng wrote:
>> On 8/1/2024 12:58 PM, Mingwei Zhang wrote:
> 
>>> +static inline u64 __perf_event_time_ctx_now(struct perf_event *event,
>>> +					    struct perf_time_ctx *time,
>>> +					    struct perf_time_ctx *timeguest,
>>> +					    u64 now)
>>> +{
>>> +	/*
>>> +	 * The exclude_guest event time should be calculated from
>>> +	 * the ctx time -  the guest time.
>>> +	 * The ctx time is now + READ_ONCE(time->offset).
>>> +	 * The guest time is now + READ_ONCE(timeguest->offset).
>>> +	 * So the exclude_guest time is
>>> +	 * READ_ONCE(time->offset) - READ_ONCE(timeguest->offset).
>>> +	 */
>>> +	if (event->attr.exclude_guest && __this_cpu_read(perf_in_guest))
>>
>> Hi Kan,
>>
>> we see the following the warning when run perf record command after
>> enabling "CONFIG_DEBUG_PREEMPT" config item.
>>
>> [  166.779208] BUG: using __this_cpu_read() in preemptible [00000000] code:
>> perf/9494
>> [  166.779234] caller is __this_cpu_preempt_check+0x13/0x20
>> [  166.779241] CPU: 56 UID: 0 PID: 9494 Comm: perf Not tainted
>> 6.11.0-rc4-perf-next-mediated-vpmu-v3+ #80
>> [  166.779245] Hardware name: Quanta Cloud Technology Inc. QuantaGrid
>> D54Q-2U/S6Q-MB-MPS, BIOS 3A11.uh 12/02/2022
>> [  166.779248] Call Trace:
>> [  166.779250]  <TASK>
>> [  166.779252]  dump_stack_lvl+0x76/0xa0
>> [  166.779260]  dump_stack+0x10/0x20
>> [  166.779267]  check_preemption_disabled+0xd7/0xf0
>> [  166.779273]  __this_cpu_preempt_check+0x13/0x20
>> [  166.779279]  calc_timer_values+0x193/0x200
>> [  166.779287]  perf_event_update_userpage+0x4b/0x170
>> [  166.779294]  ? ring_buffer_attach+0x14c/0x200
>> [  166.779301]  perf_mmap+0x533/0x5d0
>> [  166.779309]  mmap_region+0x243/0xaa0
>> [  166.779322]  do_mmap+0x35b/0x640
>> [  166.779333]  vm_mmap_pgoff+0xf0/0x1c0
>> [  166.779345]  ksys_mmap_pgoff+0x17a/0x250
>> [  166.779354]  __x64_sys_mmap+0x33/0x70
>> [  166.779362]  x64_sys_call+0x1fa4/0x25f0
>> [  166.779369]  do_syscall_64+0x70/0x130
>>
>> The season that kernel complains this is __perf_event_time_ctx_now() calls
>> __this_cpu_read() in preemption enabled context.
>>
>> To eliminate the warning, we may need to use this_cpu_read() to replace
>> __this_cpu_read().
>>
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index ccd61fd06e8d..1eb628f8b3a0 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -1581,7 +1581,7 @@ static inline u64 __perf_event_time_ctx_now(struct
>> perf_event *event,
>>          * So the exclude_guest time is
>>          * READ_ONCE(time->offset) - READ_ONCE(timeguest->offset).
>>          */
>> -       if (event->attr.exclude_guest && __this_cpu_read(perf_in_guest))
>> +       if (event->attr.exclude_guest && this_cpu_read(perf_in_guest))
>>                 return READ_ONCE(time->offset) - READ_ONCE(timeguest->offset);
>>         else
>>                 return now + READ_ONCE(time->offset);
> 
> The saner fix is moving the preempt_disable() in
> perf_event_update_userpage() up a few lines, no?

Yes, the preempt_disable() is to guarantee consistent time stamps. It
makes sense to include the time calculation part in the preempt_disable().

Since the time-related code was updated recently, I will fold all the
suggestions into the newly re-based code.

Thanks,
Kan