From: Martin Pieuchot Subject: Re: btrace: Support additional interval/profile units To: Christian Ludwig Cc: tech@openbsd.org Date: Wed, 22 Jan 2025 12:02:36 +0100 On 06/01/25(Mon) 16:54, Christian Ludwig wrote: > Hi, > > this is an update to cheloha@'s diff at > https://marc.info/?l=openbsd-tech&m=170917477404018 and allows arbitrary > periods for interval/profile probes in btrace(8). It implements > additional units for these probes: s, ms, us. > > Notable changes from the original diff: > > * Kill dtrq_rate, use dtrq_nsecs everywhere. > * Adjust parser errors / debug messages. > * A regression test. > > Tests and feedback welcome. ok mpi@ > --- > regress/usr.sbin/btrace/Makefile | 3 +- > regress/usr.sbin/btrace/interval.bt | 8 ++++ > regress/usr.sbin/btrace/interval.ok | 3 ++ > sys/dev/dt/dt_prov_profile.c | 9 ++-- > sys/dev/dt/dtvar.h | 2 +- > usr.sbin/btrace/bt_parse.y | 67 +++++++++++++++++++++-------- > usr.sbin/btrace/bt_parser.h | 4 +- > usr.sbin/btrace/btrace.c | 65 ++++++++++++++++++---------- > 8 files changed, 114 insertions(+), 47 deletions(-) > create mode 100644 regress/usr.sbin/btrace/interval.bt > create mode 100644 regress/usr.sbin/btrace/interval.ok > > diff --git a/regress/usr.sbin/btrace/Makefile b/regress/usr.sbin/btrace/Makefile > index 9214f221f44e..05a33b933dff 100644 > --- a/regress/usr.sbin/btrace/Makefile > +++ b/regress/usr.sbin/btrace/Makefile > @@ -14,7 +14,8 @@ BT_LANG_SCRIPTS= arithm beginend beginend-argn boolean comments \ > BT_ARG_LANG_SCRIPTS= staticv str > > # scripts that use kernel probes > -BT_KERN_SCRIPTS= argn empty-stmts filters mapoverwrite multiprobe > +BT_KERN_SCRIPTS= argn empty-stmts filters interval mapoverwrite \ > + multiprobe > > REGRESS_EXPECTED_FAILURES= run-maxoperand > > diff --git a/regress/usr.sbin/btrace/interval.bt b/regress/usr.sbin/btrace/interval.bt > new file mode 100644 > index 000000000000..c975d0a3187a > --- /dev/null > +++ b/regress/usr.sbin/btrace/interval.bt > @@ -0,0 +1,8 @@ > +interval:ms:300 { > + @count = @count + 1; > + printf("Probe: %d\n", @count); > +} > + > +profile:s:1 { > + exit(); > +} > diff --git a/regress/usr.sbin/btrace/interval.ok b/regress/usr.sbin/btrace/interval.ok > new file mode 100644 > index 000000000000..a2b653b166d3 > --- /dev/null > +++ b/regress/usr.sbin/btrace/interval.ok > @@ -0,0 +1,3 @@ > +Probe: 1 > +Probe: 2 > +Probe: 3 > diff --git a/sys/dev/dt/dt_prov_profile.c b/sys/dev/dt/dt_prov_profile.c > index 394cda9cd284..68fd258ddef3 100644 > --- a/sys/dev/dt/dt_prov_profile.c > +++ b/sys/dev/dt/dt_prov_profile.c > @@ -67,16 +67,17 @@ int > dt_prov_profile_alloc(struct dt_probe *dtp, struct dt_softc *sc, > struct dt_pcb_list *plist, struct dtioc_req *dtrq) > { > + uint64_t nsecs; > struct dt_pcb *dp; > struct cpu_info *ci; > CPU_INFO_ITERATOR cii; > - extern int hz; > > KASSERT(TAILQ_EMPTY(plist)); > KASSERT(dtp == dtpp_profile || dtp == dtpp_interval); > > - if (dtrq->dtrq_rate <= 0 || dtrq->dtrq_rate > hz) > - return EOPNOTSUPP; > + nsecs = dtrq->dtrq_nsecs; > + if (nsecs < USEC_TO_NSEC(200) || nsecs > SEC_TO_NSEC(UINT32_MAX)) > + return EINVAL; > > CPU_INFO_FOREACH(cii, ci) { > if (!CPU_IS_PRIMARY(ci) && (dtp == dtpp_interval)) > @@ -88,7 +89,7 @@ dt_prov_profile_alloc(struct dt_probe *dtp, struct dt_softc *sc, > return ENOMEM; > } > > - dp->dp_nsecs = SEC_TO_NSEC(1) / dtrq->dtrq_rate; > + dp->dp_nsecs = nsecs; > dp->dp_cpu = ci; > > dp->dp_evtflags = dtrq->dtrq_evtflags & DTEVT_PROV_PROFILE; > diff --git a/sys/dev/dt/dtvar.h b/sys/dev/dt/dtvar.h > index 1ca135d5eb49..00c84fb20510 100644 > --- a/sys/dev/dt/dtvar.h > +++ b/sys/dev/dt/dtvar.h > @@ -109,7 +109,7 @@ struct dtioc_arg { > > struct dtioc_req { > uint32_t dtrq_pbn; /* probe number */ > - uint32_t dtrq_rate; /* number of ticks */ > + uint64_t dtrq_nsecs; /* execution period */ > uint64_t dtrq_evtflags; /* states to record */ > }; > > diff --git a/usr.sbin/btrace/bt_parse.y b/usr.sbin/btrace/bt_parse.y > index 6e23b7bcd2da..33062420b84c 100644 > --- a/usr.sbin/btrace/bt_parse.y > +++ b/usr.sbin/btrace/bt_parse.y > @@ -62,7 +62,7 @@ struct bt_arg g_maxba = BA_INITIALIZER(LONG_MAX, B_AT_LONG); > > struct bt_rule *br_new(struct bt_probe *, struct bt_filter *, > struct bt_stmt *); > -struct bt_probe *bp_new(const char *, const char *, const char *, int32_t); > +struct bt_probe *bp_new(const char *, const char *, const char *, long); > struct bt_arg *ba_append(struct bt_arg *, struct bt_arg *); > struct bt_arg *ba_op(enum bt_argtype, struct bt_arg *, struct bt_arg *); > struct bt_stmt *bs_new(enum bt_action, struct bt_arg *, struct bt_var *); > @@ -119,7 +119,7 @@ static int beflag = 0; /* BEGIN/END parsing context flag */ > %token ERROR ENDFILT > %token OP_EQ OP_NE OP_LE OP_LT OP_GE OP_GT OP_LAND OP_LOR > /* Builtins */ > -%token BUILTIN BEGIN ELSE END HZ IF STR > +%token BUILTIN BEGIN ELSE END IF STR > /* Functions and Map operators */ > %token F_DELETE F_PRINT > %token MFUNC FUNC0 FUNC1 FUNCN OP1 OP2 OP4 MOP0 MOP1 > @@ -155,7 +155,7 @@ probe : { pflag = 1; } pname { $$ = $2; pflag = 0; } > ; > > pname : STRING ':' STRING ':' STRING { $$ = bp_new($1, $3, $5, 0); } > - | STRING ':' HZ ':' NUMBER { $$ = bp_new($1, "hz", NULL, $5); } > + | STRING ':' STRING ':' NUMBER { $$ = bp_new($1, $3, NULL, $5); } > ; > > mentry : GVAR '[' vargs ']' { $$ = bm_find($1, $3); } > @@ -358,18 +358,55 @@ bt_new(struct bt_arg *ba, struct bt_stmt *condbs, struct bt_stmt *elsebs) > return bs_new(B_AC_TEST, bop, (struct bt_var *)bc); > } > > +/* > + * interval and profile support the same units. > + */ > +static uint64_t > +bp_unit_to_nsec(const char *unit, long value) > +{ > + static const struct { > + const char *name; > + enum { UNIT_HZ, UNIT_US, UNIT_MS, UNIT_S } id; > + long long max; > + } units[] = { > + { .name = "hz", .id = UNIT_HZ, .max = 1000000LL }, > + { .name = "us", .id = UNIT_US, .max = LLONG_MAX / 1000 }, > + { .name = "ms", .id = UNIT_MS, .max = LLONG_MAX / 1000000 }, > + { .name = "s", .id = UNIT_S, .max = LLONG_MAX / 1000000000 }, > + }; > + size_t i; > + > + for (i = 0; i < nitems(units); i++) { > + if (strcmp(units[i].name, unit) == 0) { > + if (value < 1) > + yyerror("Number is invalid: %ld", value); > + if (value > units[i].max) > + yyerror("Number is too large: %ld", value); > + switch (units[i].id) { > + case UNIT_HZ: > + return (1000000000LLU / value); > + case UNIT_US: > + return (value * 1000LLU); > + case UNIT_MS: > + return (value * 1000000LLU); > + case UNIT_S: > + return (value * 1000000000LLU); > + } > + } > + } > + yyerror("Invalid unit: %s", unit); > + return 0; > +} > + > /* Create a new probe */ > struct bt_probe * > -bp_new(const char *prov, const char *func, const char *name, int32_t rate) > +bp_new(const char *prov, const char *func, const char *name, long number) > { > struct bt_probe *bp; > enum bt_ptype ptype; > > - if (rate < 0 || rate > INT32_MAX) > - errx(1, "only positive values permitted"); > - > if (prov == NULL && func == NULL && name == NULL) > - ptype = rate; /* BEGIN or END */ > + ptype = number; /* BEGIN or END */ > else > ptype = B_PT_PROBE; > > @@ -379,7 +416,8 @@ bp_new(const char *prov, const char *func, const char *name, int32_t rate) > bp->bp_prov = prov; > bp->bp_func = func; > bp->bp_name = name; > - bp->bp_rate = rate; > + if (ptype == B_PT_PROBE && name == NULL) > + bp->bp_nsecs = bp_unit_to_nsec(func, number); > bp->bp_type = ptype; > > return bp; > @@ -726,7 +764,6 @@ lookup(char *s) > { "else", ELSE, 0 }, > { "exit", FUNC0, B_AC_EXIT }, > { "hist", OP1, 0 }, > - { "hz", HZ, 0 }, > { "if", IF, 0 }, > { "kstack", BUILTIN, B_AT_BI_KSTACK }, > { "lhist", OP4, 0 }, > @@ -1077,14 +1114,10 @@ again: > /* > * Probe lexer backdoor, interpret the token as a string > * rather than a keyword. Otherwise, reserved keywords > - * would conflict with syscall names. The exception to > - * this is 'hz', which hopefully will never be a > - * syscall. > + * would conflict with syscall names. > */ > - if (kwp->token != HZ) { > - yylval.v.string = kwp->word; > - return STRING; > - } > + yylval.v.string = kwp->word; > + return STRING; > } else if (beflag) { > /* Interpret tokens in a BEGIN/END context. */ > if (kwp->type >= B_AT_BI_ARG0 && > diff --git a/usr.sbin/btrace/bt_parser.h b/usr.sbin/btrace/bt_parser.h > index 081c0045c2f7..579ae314a181 100644 > --- a/usr.sbin/btrace/bt_parser.h > +++ b/usr.sbin/btrace/bt_parser.h > @@ -31,7 +31,7 @@ > * > * "provider:function:name" > * or > - * "provider:time_unit:rate" > + * "provider:time_unit:number" > * > * Multiple probes can be associated to the same action. > */ > @@ -40,7 +40,7 @@ struct bt_probe { > const char *bp_prov; /* provider */ > const char *bp_func; /* function or time unit */ > const char *bp_name; > - uint32_t bp_rate; > + uint64_t bp_nsecs; > #define bp_unit bp_func > enum bt_ptype { > B_PT_BEGIN = 1, > diff --git a/usr.sbin/btrace/btrace.c b/usr.sbin/btrace/btrace.c > index 7c2e3dcdc22c..8cbc8ce9eadd 100644 > --- a/usr.sbin/btrace/btrace.c > +++ b/usr.sbin/btrace/btrace.c > @@ -363,12 +363,6 @@ dtpi_func(struct dtioc_probe_info *dtpi) > return syscallnames[idx]; > } > > -int > -dtpi_is_unit(const char *unit) > -{ > - return !strncmp("hz", unit, sizeof("hz")); > -} > - > struct dtioc_probe_info * > dtpi_get_by_value(const char *prov, const char *func, const char *name) > { > @@ -377,20 +371,16 @@ dtpi_get_by_value(const char *prov, const char *func, const char *name) > > dtpi = dt_dtpis; > for (i = 0; i < dt_ndtpi; i++, dtpi++) { > - if (prov != NULL && > - strncmp(prov, dtpi->dtpi_prov, DTNAMESIZE)) > - continue; > - > - if (func != NULL) { > - if (dtpi_is_unit(func)) > - return dtpi; > - > - if (strncmp(func, dtpi_func(dtpi), DTNAMESIZE)) > + if (prov != NULL) { > + if (strncmp(prov, dtpi->dtpi_prov, DTNAMESIZE)) > + continue; > + } > + if (func != NULL && name != NULL) { > + if (strncmp(func, dtpi_func(dtpi), DTNAMESIZE)) > + continue; > + if (strncmp(name, dtpi->dtpi_name, DTNAMESIZE)) > continue; > } > - > - if (strncmp(name, dtpi->dtpi_name, DTNAMESIZE)) > - continue; > > debug("matched probe %s:%s:%s\n", dtpi->dtpi_prov, > dtpi_func(dtpi), dtpi->dtpi_name); > @@ -400,6 +390,37 @@ dtpi_get_by_value(const char *prov, const char *func, const char *name) > return NULL; > } > > +static uint64_t > +bp_nsecs_to_unit(struct bt_probe *bp) > +{ > + static const struct { > + const char *name; > + enum { UNIT_HZ, UNIT_US, UNIT_MS, UNIT_S } id; > + } units[] = { > + { .name = "hz", .id = UNIT_HZ }, > + { .name = "us", .id = UNIT_US }, > + { .name = "ms", .id = UNIT_MS }, > + { .name = "s", .id = UNIT_S }, > + }; > + size_t i; > + > + for (i = 0; i < nitems(units); i++) { > + if (strcmp(units[i].name, bp->bp_unit) == 0) { > + switch (units[i].id) { > + case UNIT_HZ: > + return (1000000000LLU / bp->bp_nsecs); > + case UNIT_US: > + return (bp->bp_nsecs / 1000LLU); > + case UNIT_MS: > + return (bp->bp_nsecs / 1000000LLU); > + case UNIT_S: > + return (bp->bp_nsecs / 1000000000LLU); > + } > + } > + } > + return 0; > +} > + > void > probe_bail(struct bt_probe *bp) > { > @@ -420,9 +441,9 @@ probe_name(struct bt_probe *bp) > > assert(bp->bp_type == B_PT_PROBE); > > - if (bp->bp_rate) { > - snprintf(buf, sizeof(buf), "%s:%s:%u", bp->bp_prov, > - bp->bp_unit, bp->bp_rate); > + if (bp->bp_nsecs) { > + snprintf(buf, sizeof(buf), "%s:%s:%llu", bp->bp_prov, > + bp->bp_unit, bp_nsecs_to_unit(bp)); > } else { > snprintf(buf, sizeof(buf), "%s:%s:%s", bp->bp_prov, > bp->bp_unit, bp->bp_name); > @@ -582,7 +603,7 @@ rules_setup(int fd) > > bp->bp_pbn = dtpi->dtpi_pbn; > dtrq->dtrq_pbn = dtpi->dtpi_pbn; > - dtrq->dtrq_rate = bp->bp_rate; > + dtrq->dtrq_nsecs = bp->bp_nsecs; > dtrq->dtrq_evtflags = evtflags; > if (dtrq->dtrq_evtflags & DTEVT_KSTACK) > dokstack = 1; > -- > 2.34.1 >