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

[George Dunlap <george.dunlap@xxxxxxxxx>]

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.

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