On Wed, Mar 20, 2024 at 1:13 PM Jan Beulich <jbeul...@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.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.
> >
> > 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
