On Mon, Mar 18, 2024 at 4:36 PM Andrew Cooper <andrew.coop...@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
