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.
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
