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

  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>
---

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(-)
  

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.

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

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

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

> +
> +       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
-john
  

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