make btrace interval event to reciprocal of ticks

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

make btrace interval event to reciprocal of ticks

Yuichiro NAITO
Current btrace has `interval:hz:1` probe that makes periodically events.
`interval:hz:1` looks to make events once per second (= 1Hz),
but current implementation makes once per tick (= 100Hz on amd64).
I think the interval should be counted by reciprocal of ticks so that fit to Hz.

Following patch changes to calculate 'dp_maxtick' fit to Hz.
'dtrq_rate' has number of hz. I think it should be allowed same value of 'hz'
so that we can use `interval:hz:100` that equals to once per tick on amd64.

Index: dt_prov_profile.c
===================================================================
RCS file: /cvs/src/sys/dev/dt/dt_prov_profile.c,v
retrieving revision 1.2
diff -u -p -r1.2 dt_prov_profile.c
--- dt_prov_profile.c 25 Mar 2020 14:59:23 -0000 1.2
+++ dt_prov_profile.c 23 Jun 2020 02:02:27 -0000
@@ -75,7 +75,7 @@ dt_prov_profile_alloc(struct dt_probe *d
  KASSERT(TAILQ_EMPTY(plist));
  KASSERT(dtp == dtpp_profile || dtp == dtpp_interval);
 
- if (dtrq->dtrq_rate <= 0 || dtrq->dtrq_rate >= hz)
+ if (dtrq->dtrq_rate <= 0 || dtrq->dtrq_rate > hz)
  return EOPNOTSUPP;
 
  CPU_INFO_FOREACH(cii, ci) {
@@ -88,7 +88,7 @@ dt_prov_profile_alloc(struct dt_probe *d
  return ENOMEM;
  }
 
- dp->dp_maxtick = dtrq->dtrq_rate;
+ dp->dp_maxtick = hz / dtrq->dtrq_rate;
  dp->dp_cpuid = ci->ci_cpuid;
 
  dp->dp_filter = dtrq->dtrq_filter;

--
Yuichiro NAITO
  [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: make btrace interval event to reciprocal of ticks

Martin Pieuchot
On 23/06/20(Tue) 12:04, Yuichiro NAITO wrote:
> Current btrace has `interval:hz:1` probe that makes periodically events.
> `interval:hz:1` looks to make events once per second (= 1Hz),
> but current implementation makes once per tick (= 100Hz on amd64).
> I think the interval should be counted by reciprocal of ticks so that fit to Hz.

Indeed the current implementations assumes it's a number of ticks.  How
does other tracing tool behave?  Is the behavior you suggest similar to
bpftrace(8) or dtrace(1)?

Would it be complicated to add support for other units, like 'ms' or
'sec'?  In that case 'profile:s:10' would mean every 10sec while
'profile:hz:10' would mean every ~6sec, right?

> Following patch changes to calculate 'dp_maxtick' fit to Hz.
> 'dtrq_rate' has number of hz. I think it should be allowed same value of 'hz'
> so that we can use `interval:hz:100` that equals to once per tick on amd64.
>
> Index: dt_prov_profile.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/dt/dt_prov_profile.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 dt_prov_profile.c
> --- dt_prov_profile.c 25 Mar 2020 14:59:23 -0000 1.2
> +++ dt_prov_profile.c 23 Jun 2020 02:02:27 -0000
> @@ -75,7 +75,7 @@ dt_prov_profile_alloc(struct dt_probe *d
>   KASSERT(TAILQ_EMPTY(plist));
>   KASSERT(dtp == dtpp_profile || dtp == dtpp_interval);
>  
> - if (dtrq->dtrq_rate <= 0 || dtrq->dtrq_rate >= hz)
> + if (dtrq->dtrq_rate <= 0 || dtrq->dtrq_rate > hz)
>   return EOPNOTSUPP;
>  
>   CPU_INFO_FOREACH(cii, ci) {
> @@ -88,7 +88,7 @@ dt_prov_profile_alloc(struct dt_probe *d
>   return ENOMEM;
>   }
>  
> - dp->dp_maxtick = dtrq->dtrq_rate;
> + dp->dp_maxtick = hz / dtrq->dtrq_rate;
>   dp->dp_cpuid = ci->ci_cpuid;
>  
>   dp->dp_filter = dtrq->dtrq_filter;
>
> --
> Yuichiro NAITO
>   [hidden email]
>

Reply | Threaded
Open this post in threaded view
|

Re: make btrace interval event to reciprocal of ticks

Lauri Tirkkonen-3
On Thu, Jun 25 2020 11:36:48 +0200, Martin Pieuchot wrote:
> On 23/06/20(Tue) 12:04, Yuichiro NAITO wrote:
> > Current btrace has `interval:hz:1` probe that makes periodically events.
> > `interval:hz:1` looks to make events once per second (= 1Hz),
> > but current implementation makes once per tick (= 100Hz on amd64).
> > I think the interval should be counted by reciprocal of ticks so that fit to Hz.
>
> Indeed the current implementations assumes it's a number of ticks.  How
> does other tracing tool behave?  Is the behavior you suggest similar to
> bpftrace(8) or dtrace(1)?

In DTrace, profile-<n> probes fire at a frequency of <n> hertz, but
apparently also support suffixes.
http://dtrace.org/guide/chp-profile.html

--
Lauri Tirkkonen | lotheac @ IRCnet

Reply | Threaded
Open this post in threaded view
|

Re: make btrace interval event to reciprocal of ticks

Yuichiro NAITO
In reply to this post by Martin Pieuchot
From: Martin Pieuchot <[hidden email]>
Subject: Re: make btrace interval event to reciprocal of ticks
Date: Thu, 25 Jun 2020 11:36:48 +0200

> On 23/06/20(Tue) 12:04, Yuichiro NAITO wrote:
>> Current btrace has `interval:hz:1` probe that makes periodically events.
>> `interval:hz:1` looks to make events once per second (= 1Hz),
>> but current implementation makes once per tick (= 100Hz on amd64).
>> I think the interval should be counted by reciprocal of ticks so that fit to Hz.
>
> Indeed the current implementations assumes it's a number of ticks.  How
> does other tracing tool behave?  Is the behavior you suggest similar to
> bpftrace(8) or dtrace(1)?

I don't know about bpftrace(8).
Dtrace has interval timer in Hz as Lauri says.

> Would it be complicated to add support for other units, like 'ms' or
> 'sec'?  In that case 'profile:s:10' would mean every 10sec while
> 'profile:hz:10' would mean every ~6sec, right?

I feel it's ok to just rename 'interval:hz' to 'interval:ticks' with
no implementation change. (but I hope that 100 is allowed.)
We can know the tick interval from 'hz' in `sysctl -n kern.clockrate`.
So it's easy to understand that 'interval:ticks:N' fires on every N ticks.

And tick resolution is not so high.
In my patch, 51 Hz ~ 100 Hz is calculated to 10 ms (=1 tick) interval.
It might confuse some users.

In my usecase, I want 10ms ~ 1sec intervals.
If N is allowed over 99, 'interval:ticks:N' can be useful to me.

--
Yuichiro NAITO
  [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: make btrace interval event to reciprocal of ticks

Martin Pieuchot
On 25/06/20(Thu) 23:45, Yuichiro NAITO wrote:

> From: Martin Pieuchot <[hidden email]>
> Subject: Re: make btrace interval event to reciprocal of ticks
> Date: Thu, 25 Jun 2020 11:36:48 +0200
>
> > On 23/06/20(Tue) 12:04, Yuichiro NAITO wrote:
> >> Current btrace has `interval:hz:1` probe that makes periodically events.
> >> `interval:hz:1` looks to make events once per second (= 1Hz),
> >> but current implementation makes once per tick (= 100Hz on amd64).
> >> I think the interval should be counted by reciprocal of ticks so that fit to Hz.
> >
> > Indeed the current implementations assumes it's a number of ticks.  How
> > does other tracing tool behave?  Is the behavior you suggest similar to
> > bpftrace(8) or dtrace(1)?
>
> I don't know about bpftrace(8).
> Dtrace has interval timer in Hz as Lauri says.

Seems to be the same, so I'll commit your diff.

> > Would it be complicated to add support for other units, like 'ms' or
> > 'sec'?  In that case 'profile:s:10' would mean every 10sec while
> > 'profile:hz:10' would mean every ~6sec, right?
>
> I feel it's ok to just rename 'interval:hz' to 'interval:ticks' with
> no implementation change. (but I hope that 100 is allowed.)
> We can know the tick interval from 'hz' in `sysctl -n kern.clockrate`.
> So it's easy to understand that 'interval:ticks:N' fires on every N ticks.
>
> And tick resolution is not so high.
> In my patch, 51 Hz ~ 100 Hz is calculated to 10 ms (=1 tick) interval.
> It might confuse some users.
>
> In my usecase, I want 10ms ~ 1sec intervals.
> If N is allowed over 99, 'interval:ticks:N' can be useful to me.

Well ideally we should be able to specify an interval in second or
millisecond.  If you or somebody else wants to implement that, I
suppose it makes sense now to change `dtrq_rate' to be a uint64_t
and encode the interval in nanosecond.