diff mbox series

[2/7] xen/credit2: Clean up trace handling

Message ID 20240318163552.3808695-3-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series xen/trace: Treewide API cleanup | expand

Commit Message

Andrew Cooper March 18, 2024, 4:35 p.m. UTC 
There is no need for bitfields anywhere - use more sensible types.  There is
also no need to cast 'd' to (unsigned char *) before passing it to a function
taking void *.  Switch to new trace_time() API.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Dario Faggioli <dfaggioli@suse.com>
CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
CC: consulting@bugseng.com <consulting@bugseng.com>

v2:
 * Fix whitespace.
v3:
 * Rebase over core API changes.
---
 xen/common/sched/credit2.c | 301 ++++++++++++++++++-------------------
 1 file changed, 146 insertions(+), 155 deletions(-)

Comments

George Dunlap March 20, 2024, 12:16 p.m. UTC | #1 
On Mon, Mar 18, 2024 at 4:36 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> There is no need for bitfields anywhere - use more sensible types.  There is
> also no need to cast 'd' to (unsigned char *) before passing it to a function
> taking void *.  Switch to new trace_time() API.
>
> No functional change.

Hey Andrew -- overall changes look great, thanks for doing this very
detailed work.

One issue here is that you've changed a number of signed values to
unsigned values; for example:

> @@ -1563,16 +1559,16 @@ static s_time_t tickle_score(const struct scheduler *ops, s_time_t now,
>      if ( unlikely(tb_init_done) )
>      {
>          struct {
> -            unsigned unit:16, dom:16;
> -            int credit, score;
> -        } d;
> -        d.dom = cur->unit->domain->domain_id;
> -        d.unit = cur->unit->unit_id;
> -        d.credit = cur->credit;
> -        d.score = score;
> -        __trace_var(TRC_CSCHED2_TICKLE_CHECK, 1,
> -                    sizeof(d),
> -                    (unsigned char *)&d);
> +            uint16_t unit, dom;
> +            uint32_t credit, score;

...here you change `int` to `unit32_t`; but `credit` and `score` are
both signed values, which may be negative.  There are a number of
other similar instances.  In general, if there's a signed value, it
was meant.

I don't want to delay this another 3 years, so if there's not a strong
argument in favor of converting the signed types into uint32_t, I can
make the change myself and re-submit the series.

(I'll probably end up making a change to xenalyze.c as well, to update
the structure definitions there to match what's in Xen, so it wouldn't
be any extra effort.)

 -George
Andrew Cooper March 20, 2024, 12:19 p.m. UTC | #2 
On 20/03/2024 12:16 pm, George Dunlap wrote:
> On Mon, Mar 18, 2024 at 4:36 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> There is no need for bitfields anywhere - use more sensible types.  There is
>> also no need to cast 'd' to (unsigned char *) before passing it to a function
>> taking void *.  Switch to new trace_time() API.
>>
>> No functional change.
> Hey Andrew -- overall changes look great, thanks for doing this very
> detailed work.
>
> One issue here is that you've changed a number of signed values to
> unsigned values; for example:
>
>> @@ -1563,16 +1559,16 @@ static s_time_t tickle_score(const struct scheduler *ops, s_time_t now,
>>      if ( unlikely(tb_init_done) )
>>      {
>>          struct {
>> -            unsigned unit:16, dom:16;
>> -            int credit, score;
>> -        } d;
>> -        d.dom = cur->unit->domain->domain_id;
>> -        d.unit = cur->unit->unit_id;
>> -        d.credit = cur->credit;
>> -        d.score = score;
>> -        __trace_var(TRC_CSCHED2_TICKLE_CHECK, 1,
>> -                    sizeof(d),
>> -                    (unsigned char *)&d);
>> +            uint16_t unit, dom;
>> +            uint32_t credit, score;
> ...here you change `int` to `unit32_t`; but `credit` and `score` are
> both signed values, which may be negative.  There are a number of
> other similar instances.  In general, if there's a signed value, it
> was meant.

Oh - this is a consequence of being reviewed that way in earlier iterations.

If they really can hold negative numbers, they can become int32_t's. 
What's important is that they have a clearly-specified width.

~Andrew
George Dunlap March 20, 2024, 1:04 p.m. UTC | #3 
On Wed, Mar 20, 2024 at 12:19 PM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 20/03/2024 12:16 pm, George Dunlap wrote:
> > On Mon, Mar 18, 2024 at 4:36 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >> There is no need for bitfields anywhere - use more sensible types.  There is
> >> also no need to cast 'd' to (unsigned char *) before passing it to a function
> >> taking void *.  Switch to new trace_time() API.
> >>
> >> No functional change.
> > Hey Andrew -- overall changes look great, thanks for doing this very
> > detailed work.
> >
> > One issue here is that you've changed a number of signed values to
> > unsigned values; for example:
> >
> >> @@ -1563,16 +1559,16 @@ static s_time_t tickle_score(const struct scheduler *ops, s_time_t now,
> >>      if ( unlikely(tb_init_done) )
> >>      {
> >>          struct {
> >> -            unsigned unit:16, dom:16;
> >> -            int credit, score;
> >> -        } d;
> >> -        d.dom = cur->unit->domain->domain_id;
> >> -        d.unit = cur->unit->unit_id;
> >> -        d.credit = cur->credit;
> >> -        d.score = score;
> >> -        __trace_var(TRC_CSCHED2_TICKLE_CHECK, 1,
> >> -                    sizeof(d),
> >> -                    (unsigned char *)&d);
> >> +            uint16_t unit, dom;
> >> +            uint32_t credit, score;
> > ...here you change `int` to `unit32_t`; but `credit` and `score` are
> > both signed values, which may be negative.  There are a number of
> > other similar instances.  In general, if there's a signed value, it
> > was meant.
>
> Oh - this is a consequence of being reviewed that way in earlier iterations.
>
> If they really can hold negative numbers, they can become int32_t's.
> What's important is that they have a clearly-specified width.

Oh yes, sorry if that wasn't clear -- I was suggesting int32_t, unless
there was some compelling reason to do otherwise.  I just realize that
the last time this series had some review comments, it took 3 years to
come back around, and it would be a shame if that happened again; so
I'm happy to go through and do the change.

 -George
Jan Beulich March 20, 2024, 1:13 p.m. UTC | #4 
On 20.03.2024 13:19, Andrew Cooper wrote:
> On 20/03/2024 12:16 pm, George Dunlap wrote:
>> On Mon, Mar 18, 2024 at 4:36 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> There is no need for bitfields anywhere - use more sensible types.  There is
>>> also no need to cast 'd' to (unsigned char *) before passing it to a function
>>> taking void *.  Switch to new trace_time() API.
>>>
>>> No functional change.
>> Hey Andrew -- overall changes look great, thanks for doing this very
>> detailed work.
>>
>> One issue here is that you've changed a number of signed values to
>> unsigned values; for example:
>>
>>> @@ -1563,16 +1559,16 @@ static s_time_t tickle_score(const struct scheduler *ops, s_time_t now,
>>>      if ( unlikely(tb_init_done) )
>>>      {
>>>          struct {
>>> -            unsigned unit:16, dom:16;
>>> -            int credit, score;
>>> -        } d;
>>> -        d.dom = cur->unit->domain->domain_id;
>>> -        d.unit = cur->unit->unit_id;
>>> -        d.credit = cur->credit;
>>> -        d.score = score;
>>> -        __trace_var(TRC_CSCHED2_TICKLE_CHECK, 1,
>>> -                    sizeof(d),
>>> -                    (unsigned char *)&d);
>>> +            uint16_t unit, dom;
>>> +            uint32_t credit, score;
>> ...here you change `int` to `unit32_t`; but `credit` and `score` are
>> both signed values, which may be negative.  There are a number of
>> other similar instances.  In general, if there's a signed value, it
>> was meant.
> 
> Oh - this is a consequence of being reviewed that way in earlier iterations.

Which in turn is a result of us still having way to many uses of plain
int when signed quantities aren't meant. Plus my suggestion to make
this explicit by saying "signed int" was rejected.

> If they really can hold negative numbers, they can become int32_t's. 
> What's important is that they have a clearly-specified width.

And please feel free to retain my R-b with any such adjustments.

Jan
George Dunlap March 20, 2024, 1:20 p.m. UTC | #5 
On Wed, Mar 20, 2024 at 1:13 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 20.03.2024 13:19, Andrew Cooper wrote:
> > On 20/03/2024 12:16 pm, George Dunlap wrote:
> >> On Mon, Mar 18, 2024 at 4:36 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >>> There is no need for bitfields anywhere - use more sensible types.  There is
> >>> also no need to cast 'd' to (unsigned char *) before passing it to a function
> >>> taking void *.  Switch to new trace_time() API.
> >>>
> >>> No functional change.
> >> Hey Andrew -- overall changes look great, thanks for doing this very
> >> detailed work.
> >>
> >> One issue here is that you've changed a number of signed values to
> >> unsigned values; for example:
> >>
> >>> @@ -1563,16 +1559,16 @@ static s_time_t tickle_score(const struct scheduler *ops, s_time_t now,
> >>>      if ( unlikely(tb_init_done) )
> >>>      {
> >>>          struct {
> >>> -            unsigned unit:16, dom:16;
> >>> -            int credit, score;
> >>> -        } d;
> >>> -        d.dom = cur->unit->domain->domain_id;
> >>> -        d.unit = cur->unit->unit_id;
> >>> -        d.credit = cur->credit;
> >>> -        d.score = score;
> >>> -        __trace_var(TRC_CSCHED2_TICKLE_CHECK, 1,
> >>> -                    sizeof(d),
> >>> -                    (unsigned char *)&d);
> >>> +            uint16_t unit, dom;
> >>> +            uint32_t credit, score;
> >> ...here you change `int` to `unit32_t`; but `credit` and `score` are
> >> both signed values, which may be negative.  There are a number of
> >> other similar instances.  In general, if there's a signed value, it
> >> was meant.
> >
> > Oh - this is a consequence of being reviewed that way in earlier iterations.
>
> Which in turn is a result of us still having way to many uses of plain
> int when signed quantities aren't meant. Plus my suggestion to make
> this explicit by saying "signed int" was rejected.

Not complaining, but just pointing out to maybe save some time in the
future:  The vast majority of fields in the trace records in this file
are unsigned; in fact, two of the fields in this structure are
unsigned.  That should have been a hint that being signed was likely
to be intentional in this code, and to look at the context before
removing it.  (In this case, for example just above here there's an
"if ( score < 0 )" showing that score might be negative.)

 -George
George Dunlap March 20, 2024, 3:05 p.m. UTC | #6 
On Wed, Mar 20, 2024 at 12:16 PM George Dunlap <george.dunlap@cloud.com> wrote:
>
> On Mon, Mar 18, 2024 at 4:36 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >
> > There is no need for bitfields anywhere - use more sensible types.  There is
> > also no need to cast 'd' to (unsigned char *) before passing it to a function
> > taking void *.  Switch to new trace_time() API.
> >
> > No functional change.
>
> Hey Andrew -- overall changes look great, thanks for doing this very
> detailed work.

For my own posterity later:

I've done a close review of the rest of the patch and didn't see any
issues other than the loss of sign in a handful of cases.

 -George
diff mbox series

Patch

diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
index c76330d79d3a..b7df9f2a9111 100644
--- a/xen/common/sched/credit2.c
+++ b/xen/common/sched/credit2.c
@@ -1085,13 +1085,13 @@  static void update_max_weight(struct csched2_runqueue_data *rqd, int new_weight,
     if ( unlikely(tb_init_done) )
     {
         struct {
-            unsigned rqi:16, max_weight:16;
-        } d;
-        d.rqi = rqd->id;
-        d.max_weight = rqd->max_weight;
-        __trace_var(TRC_CSCHED2_RUNQ_MAX_WEIGHT, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+            uint16_t rqi, max_weight;
+        } d = {
+            .rqi        = rqd->id,
+            .max_weight = rqd->max_weight,
+        };
+
+        trace_time(TRC_CSCHED2_RUNQ_MAX_WEIGHT, sizeof(d), &d);
     }
 }
 
@@ -1119,9 +1119,7 @@  _runq_assign(struct csched2_unit *svc, struct csched2_runqueue_data *rqd)
             .rqi  = rqd->id,
         };
 
-        __trace_var(TRC_CSCHED2_RUNQ_ASSIGN, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+        trace_time(TRC_CSCHED2_RUNQ_ASSIGN, sizeof(d), &d);
     }
 
 }
@@ -1354,9 +1352,7 @@  update_runq_load(const struct scheduler *ops,
             .shift      = P,
         };
 
-        __trace_var(TRC_CSCHED2_UPDATE_RUNQ_LOAD, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+        trace_time(TRC_CSCHED2_UPDATE_RUNQ_LOAD, sizeof(d), &d);
     }
 }
 
@@ -1406,16 +1402,16 @@  update_svc_load(const struct scheduler *ops,
     {
         struct {
             uint64_t v_avgload;
-            unsigned unit:16, dom:16;
-            unsigned shift;
-        } d;
-        d.dom = svc->unit->domain->domain_id;
-        d.unit = svc->unit->unit_id;
-        d.v_avgload = svc->avgload;
-        d.shift = P;
-        __trace_var(TRC_CSCHED2_UPDATE_UNIT_LOAD, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+            uint16_t unit, dom;
+            uint32_t shift;
+        } d = {
+            .v_avgload = svc->avgload,
+            .unit      = svc->unit->unit_id,
+            .dom       = svc->unit->domain->domain_id,
+            .shift     = P,
+        };
+
+        trace_time(TRC_CSCHED2_UPDATE_UNIT_LOAD, sizeof(d), &d);
     }
 }
 
@@ -1424,7 +1420,7 @@  update_load(const struct scheduler *ops,
             struct csched2_runqueue_data *rqd,
             struct csched2_unit *svc, int change, s_time_t now)
 {
-    trace_var(TRC_CSCHED2_UPDATE_LOAD, 1, 0,  NULL);
+    TRACE_TIME(TRC_CSCHED2_UPDATE_LOAD);
 
     update_runq_load(ops, rqd, change, now);
     if ( svc )
@@ -1462,15 +1458,15 @@  static void runq_insert(struct csched2_unit *svc)
     if ( unlikely(tb_init_done) )
     {
         struct {
-            unsigned unit:16, dom:16;
-            unsigned pos;
-        } d;
-        d.dom = svc->unit->domain->domain_id;
-        d.unit = svc->unit->unit_id;
-        d.pos = pos;
-        __trace_var(TRC_CSCHED2_RUNQ_POS, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+            uint16_t unit, dom;
+            uint32_t pos;
+        } d = {
+            .unit = svc->unit->unit_id,
+            .dom  = svc->unit->domain->domain_id,
+            .pos  = pos,
+        };
+
+        trace_time(TRC_CSCHED2_RUNQ_POS, sizeof(d), &d);
     }
 }
 
@@ -1563,16 +1559,16 @@  static s_time_t tickle_score(const struct scheduler *ops, s_time_t now,
     if ( unlikely(tb_init_done) )
     {
         struct {
-            unsigned unit:16, dom:16;
-            int credit, score;
-        } d;
-        d.dom = cur->unit->domain->domain_id;
-        d.unit = cur->unit->unit_id;
-        d.credit = cur->credit;
-        d.score = score;
-        __trace_var(TRC_CSCHED2_TICKLE_CHECK, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+            uint16_t unit, dom;
+            uint32_t credit, score;
+        } d = {
+            .unit   = cur->unit->unit_id,
+            .dom    = cur->unit->domain->domain_id,
+            .credit = cur->credit,
+            .score  = score,
+        };
+
+        trace_time(TRC_CSCHED2_TICKLE_CHECK, sizeof(d), &d);
     }
 
     return score;
@@ -1610,17 +1606,16 @@  runq_tickle(const struct scheduler *ops, struct csched2_unit *new, s_time_t now)
     if ( unlikely(tb_init_done) )
     {
         struct {
-            unsigned unit:16, dom:16;
-            unsigned processor;
-            int credit;
-        } d;
-        d.dom = unit->domain->domain_id;
-        d.unit = unit->unit_id;
-        d.processor = cpu;
-        d.credit = new->credit;
-        __trace_var(TRC_CSCHED2_TICKLE_NEW, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+            uint16_t unit, dom;
+            uint32_t processor, credit;
+        } d = {
+            .dom       = unit->domain->domain_id,
+            .unit      = unit->unit_id,
+            .processor = cpu,
+            .credit    = new->credit,
+        };
+
+        trace_time(TRC_CSCHED2_TICKLE_NEW, sizeof(d), &d);
     }
 
     /*
@@ -1759,12 +1754,12 @@  runq_tickle(const struct scheduler *ops, struct csched2_unit *new, s_time_t now)
     if ( unlikely(tb_init_done) )
     {
         struct {
-            unsigned cpu:16, pad:16;
-        } d;
-        d.cpu = ipid; d.pad = 0;
-        __trace_var(TRC_CSCHED2_TICKLE, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+            uint16_t cpu, _pad;
+        } d = {
+            .cpu = ipid,
+        };
+
+        trace_time(TRC_CSCHED2_TICKLE, sizeof(d), &d);
     }
 
     tickle_cpu(ipid, rqd);
@@ -1840,16 +1835,16 @@  static void reset_credit(int cpu, s_time_t now, struct csched2_unit *snext)
         if ( unlikely(tb_init_done) )
         {
             struct {
-                unsigned unit:16, dom:16;
-                int credit_start, credit_end;
-            } d;
-            d.dom = svc->unit->domain->domain_id;
-            d.unit = svc->unit->unit_id;
-            d.credit_start = start_credit;
-            d.credit_end = svc->credit;
-            __trace_var(TRC_CSCHED2_CREDIT_RESET, 1,
-                        sizeof(d),
-                        (unsigned char *)&d);
+                uint16_t unit, dom;
+                uint32_t credit_start, credit_end;
+            } d = {
+                .unit         = svc->unit->unit_id,
+                .dom          = svc->unit->domain->domain_id,
+                .credit_start = start_credit,
+                .credit_end   = svc->credit,
+            };
+
+            trace_time(TRC_CSCHED2_CREDIT_RESET, sizeof(d), &d);
         }
     }
 
@@ -1895,18 +1890,17 @@  void burn_credits(struct csched2_runqueue_data *rqd,
     if ( unlikely(tb_init_done) )
     {
         struct {
-            unsigned unit:16, dom:16;
-            int credit, budget;
-            int delta;
-        } d;
-        d.dom = svc->unit->domain->domain_id;
-        d.unit = svc->unit->unit_id;
-        d.credit = svc->credit;
-        d.budget = has_cap(svc) ?  svc->budget : INT_MIN;
-        d.delta = delta;
-        __trace_var(TRC_CSCHED2_CREDIT_BURN, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+            uint16_t unit, dom;
+            uint32_t credit, budget, delta;
+        } d = {
+            .unit   = svc->unit->unit_id,
+            .dom    = svc->unit->domain->domain_id,
+            .credit = svc->credit,
+            .budget = has_cap(svc) ? svc->budget : INT_MIN,
+            .delta  = delta,
+        };
+
+        trace_time(TRC_CSCHED2_CREDIT_BURN, sizeof(d), &d);
     }
 }
 
@@ -2551,17 +2545,17 @@  csched2_res_pick(const struct scheduler *ops, const struct sched_unit *unit)
     {
         struct {
             uint64_t b_avgload;
-            unsigned unit:16, dom:16;
-            unsigned rq_id:16, new_cpu:16;
-        } d;
-        d.dom = unit->domain->domain_id;
-        d.unit = unit->unit_id;
-        d.rq_id = min_rqd ? min_rqd->id : -1;
-        d.b_avgload = min_avgload;
-        d.new_cpu = new_cpu;
-        __trace_var(TRC_CSCHED2_PICKED_CPU, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+            uint16_t unit, dom;
+            uint16_t rq_id, new_cpu;
+        } d = {
+            .b_avgload = min_avgload,
+            .unit      = unit->unit_id,
+            .dom       = unit->domain->domain_id,
+            .rq_id     = min_rqd ? min_rqd->id : -1,
+            .new_cpu   = new_cpu,
+        };
+
+        trace_time(TRC_CSCHED2_PICKED_CPU, sizeof(d), &d);
     }
 
     return get_sched_res(new_cpu);
@@ -2622,16 +2616,16 @@  static void migrate(const struct scheduler *ops,
     if ( unlikely(tb_init_done) )
     {
         struct {
-            unsigned unit:16, dom:16;
-            unsigned rqi:16, trqi:16;
-        } d;
-        d.dom = unit->domain->domain_id;
-        d.unit = unit->unit_id;
-        d.rqi = svc->rqd->id;
-        d.trqi = trqd->id;
-        __trace_var(TRC_CSCHED2_MIGRATE, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+            uint16_t unit, dom;
+            uint16_t rqi, trqi;
+        } d = {
+            .unit = unit->unit_id,
+            .dom  = unit->domain->domain_id,
+            .rqi  = svc->rqd->id,
+            .trqi = trqd->id,
+        };
+
+        trace_time(TRC_CSCHED2_MIGRATE, sizeof(d), &d);
     }
 
     if ( svc->flags & CSFLAG_scheduled )
@@ -2768,15 +2762,15 @@  static void balance_load(const struct scheduler *ops, int cpu, s_time_t now)
         if ( unlikely(tb_init_done) )
         {
             struct {
-                unsigned lrq_id:16, orq_id:16;
-                unsigned load_delta;
-            } d;
-            d.lrq_id = st.lrqd->id;
-            d.orq_id = st.orqd->id;
-            d.load_delta = st.load_delta;
-            __trace_var(TRC_CSCHED2_LOAD_CHECK, 1,
-                        sizeof(d),
-                        (unsigned char *)&d);
+                uint16_t lrq_id, orq_id;
+                uint32_t load_delta;
+            } d = {
+                .lrq_id     = st.lrqd->id,
+                .orq_id     = st.orqd->id,
+                .load_delta = st.load_delta,
+            };
+
+            trace_time(TRC_CSCHED2_LOAD_CHECK, sizeof(d), &d);
         }
 
         /*
@@ -2820,9 +2814,7 @@  static void balance_load(const struct scheduler *ops, int cpu, s_time_t now)
             .orq_id     = st.orqd->id,
         };
 
-        __trace_var(TRC_CSCHED2_LOAD_BALANCE, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+        trace_time(TRC_CSCHED2_LOAD_BALANCE, sizeof(d), &d);
     }
 
     SCHED_STAT_CRANK(acct_load_balance);
@@ -3407,15 +3399,15 @@  runq_candidate(struct csched2_runqueue_data *rqd,
         if ( unlikely(tb_init_done) )
         {
             struct {
-                unsigned unit:16, dom:16;
-                unsigned runtime;
-            } d;
-            d.dom = scurr->unit->domain->domain_id;
-            d.unit = scurr->unit->unit_id;
-            d.runtime = now - scurr->unit->state_entry_time;
-            __trace_var(TRC_CSCHED2_RATELIMIT, 1,
-                        sizeof(d),
-                        (unsigned char *)&d);
+                uint16_t unit, dom;
+                uint32_t runtime;
+            } d = {
+                .unit    = scurr->unit->unit_id,
+                .dom     = scurr->unit->domain->domain_id,
+                .runtime = now - scurr->unit->state_entry_time,
+            };
+
+            trace_time(TRC_CSCHED2_RATELIMIT, sizeof(d), &d);
         }
         return scurr;
     }
@@ -3468,13 +3460,13 @@  runq_candidate(struct csched2_runqueue_data *rqd,
         if ( unlikely(tb_init_done) )
         {
             struct {
-                unsigned unit:16, dom:16;
-            } d;
-            d.dom = svc->unit->domain->domain_id;
-            d.unit = svc->unit->unit_id;
-            __trace_var(TRC_CSCHED2_RUNQ_CAND_CHECK, 1,
-                        sizeof(d),
-                        (unsigned char *)&d);
+                uint16_t unit, dom;
+            } d = {
+                .unit = svc->unit->unit_id,
+                .dom  = svc->unit->domain->domain_id,
+            };
+
+            trace_time(TRC_CSCHED2_RUNQ_CAND_CHECK, sizeof(d), &d);
         }
 
         /*
@@ -3542,17 +3534,16 @@  runq_candidate(struct csched2_runqueue_data *rqd,
     if ( unlikely(tb_init_done) )
     {
         struct {
-            unsigned unit:16, dom:16;
-            unsigned tickled_cpu;
-            int credit;
-        } d;
-        d.dom = snext->unit->domain->domain_id;
-        d.unit = snext->unit->unit_id;
-        d.credit = snext->credit;
-        d.tickled_cpu = snext->tickled_cpu;
-        __trace_var(TRC_CSCHED2_RUNQ_CANDIDATE, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+            uint16_t unit, dom;
+            uint32_t tickled_cpu, credit;
+        } d = {
+            .unit        = snext->unit->unit_id,
+            .dom         = snext->unit->domain->domain_id,
+            .tickled_cpu = snext->tickled_cpu,
+            .credit      = snext->credit,
+        };
+
+        trace_time(TRC_CSCHED2_RUNQ_CANDIDATE, sizeof(d), &d);
     }
 
     if ( unlikely(snext->tickled_cpu != -1 && snext->tickled_cpu != cpu) )
@@ -3608,18 +3599,18 @@  static void cf_check csched2_schedule(
     if ( unlikely(tb_init_done) )
     {
         struct {
-            unsigned cpu:16, rq_id:16;
-            unsigned tasklet:8, idle:8, smt_idle:8, tickled:8;
-        } d;
-        d.cpu = cur_cpu;
-        d.rq_id = c2r(sched_cpu);
-        d.tasklet = tasklet_work_scheduled;
-        d.idle = is_idle_unit(currunit);
-        d.smt_idle = cpumask_test_cpu(sched_cpu, &rqd->smt_idle);
-        d.tickled = tickled;
-        __trace_var(TRC_CSCHED2_SCHEDULE, 1,
-                    sizeof(d),
-                    (unsigned char *)&d);
+            uint16_t cpu, rq_id;
+            uint8_t tasklet, idle, smt_idle, tickled;
+        } d = {
+            .cpu      = cur_cpu,
+            .rq_id    = c2r(sched_cpu),
+            .tasklet  = tasklet_work_scheduled,
+            .idle     = is_idle_unit(currunit),
+            .smt_idle = cpumask_test_cpu(sched_cpu, &rqd->smt_idle),
+            .tickled  = tickled,
+        };
+
+        trace_time(TRC_CSCHED2_SCHEDULE, sizeof(d), &d);
     }
 
     /* Update credits (and budget, if necessary). */
@@ -3654,7 +3645,7 @@  static void cf_check csched2_schedule(
     if ( tasklet_work_scheduled )
     {
         __clear_bit(__CSFLAG_unit_yield, &scurr->flags);
-        trace_var(TRC_CSCHED2_SCHED_TASKLET, 1, 0, NULL);
+        TRACE_TIME(TRC_CSCHED2_SCHED_TASKLET);
         snext = csched2_unit(sched_idle_unit(sched_cpu));
     }
     else