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

Liang, Kan
2023-01-24T15:17:36Z

On 2023-01-24 1:56 a.m., John Stultz wrote:
> On Mon, Jan 23, 2023 at 10:27 AM <kan.liang@linux.intel.com> wrote:
>>
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> 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@linux.intel.com>
>> ---
>
> 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@linux.intel.com/
>>
>> 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