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

[George Dunlap <george.dunlap@xxxxxxxxx>]

On Wed, Mar 20, 2024 at 12:19 PM Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 20/03/2024 12:16 pm, George Dunlap wrote:
> > On Mon, Mar 18, 2024 at 4:36 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> > 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