Re: [PATCH 3/3] perf/x86/intel/ds: Support monotonic clock for PEBS

[Liang, Kan]

On 2023-01-24 1:56 a.m., John Stultz wrote:
> On Mon, Jan 23, 2023 at 10:27 AM <kan.liang@xxxxxxxxxxxxxxx> wrote:
>>
>> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>>
>> Users try to reconcile user samples with PEBS samples and require a
>> common clock source. However, the current PEBS codes only convert to
>> sched_clock, which is not available from the user space.
>>
>> Only support converting to clock monotonic. Having one common clock
>> source is good enough to fulfill the requirement.
>>
>> Enable the large PEBS for the monotonic clock to reduce the PEBS
>> overhead.
>>
>> There are a few rare cases that may make the conversion fails. For
>> example, TSC overflows. The cycle_last may be changed between samples.
>> The time will fallback to the inaccurate SW times. But the cases are
>> extremely unlikely to happen.
>>
>> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>> ---
> 
> Thanks for sending this out!
> A few minor style issues below and a warning.

Thanks.

> 
>> The patch has to be on top of the below patch
>> https://lore.kernel.org/all/20230123172027.125385-1-kan.liang@xxxxxxxxxxxxxxx/
>>
>>  arch/x86/events/intel/core.c |  2 +-
>>  arch/x86/events/intel/ds.c   | 30 ++++++++++++++++++++++++++----
>>  2 files changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 14f0a746257d..ea194556cc73 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -3777,7 +3777,7 @@ static unsigned long intel_pmu_large_pebs_flags(struct perf_event *event)
>>  {
>>         unsigned long flags = x86_pmu.large_pebs_flags;
>>
>> -       if (event->attr.use_clockid)
>> +       if (event->attr.use_clockid && (event->attr.clockid != CLOCK_MONOTONIC))
>>                 flags &= ~PERF_SAMPLE_TIME;
>>         if (!event->attr.exclude_kernel)
>>                 flags &= ~PERF_SAMPLE_REGS_USER;
>> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
>> index 7980e92dec64..d7f0eaf4405c 100644
>> --- a/arch/x86/events/intel/ds.c
>> +++ b/arch/x86/events/intel/ds.c
>> @@ -1570,13 +1570,33 @@ static u64 get_data_src(struct perf_event *event, u64 aux)
>>         return val;
>>  }
>>
>> +static int pebs_get_synctime(struct system_counterval_t *system,
>> +                            void *ctx)
> 
> Just because the abstract function type taken by
> get_mono_fast_from_given_time is vague, doesn't mean the
> implementation needs to be.
> ctx is really a tsc value, right? So let's call it that to make this a
> bit more readable.

Sure, I will use the tsc to replace ctx.

> 
>> +{
>> +       *system = set_tsc_system_counterval(*(u64 *)ctx);
>> +       return 0;
>> +}
>> +
>> +static inline int pebs_clockid_time(clockid_t clk_id, u64 tsc, u64 *clk_id_time)
> 
> clk_id_time is maybe a bit too fuzzy. It is really a mono_ns value,
> right? Let's keep that explicit here.

Yes. Will make it explicit.

> 
>> +{
>> +       /* Only support converting to clock monotonic */
>> +       if (clk_id != CLOCK_MONOTONIC)
>> +               return -EINVAL;
>> +
>> +       return get_mono_fast_from_given_time(pebs_get_synctime, &tsc, clk_id_time);
>> +}
>> +
>>  static void setup_pebs_time(struct perf_event *event,
>>                             struct perf_sample_data *data,
>>                             u64 tsc)
>>  {
>> -       /* Converting to a user-defined clock is not supported yet. */
>> -       if (event->attr.use_clockid != 0)
>> -               return;
>> +       u64 time;
> 
> Again, "time" is too generic a term without any context here.
> mono_nsec or something would be more clear.

Sure.

> 
>> +
>> +       if (event->attr.use_clockid != 0) {
>> +               if (pebs_clockid_time(event->attr.clockid, tsc, &time))
>> +                       return;
>> +               goto done;
>> +       }
> 
> Apologies for this warning/rant:
> 
> So, I do get the NMI safety of the "fast" time accessors (along with
> the "high performance" sounding name!) is attractive, but as its use
> expands I worry the downsides of this interface isn't made clear
> enough.
> 
> The fast accessors *can* see time discontinuities! Because the logic
> is done without holding the tk_core.seq lock, If you are reading in
> the middle of a ntp adjustment, you may find the current value to be
> larger than the next time you read the time.  These discontinuities
> are likely to be very small, but a negative delta will look very large
> as a u64.  So part of using these "fast *and unsafe*" interfaces is
> you get to keep both pieces when it breaks. Make sure the code here
> that is using these interfaces guards against this (zeroing out
> negative deltas).
> 

Thanks for the warning.
I will add more comments and specially handle it here.

Thanks,
Kan