Bug 210093 - Redundant sq initialization in kern/subr_sleepqueue
Summary: Redundant sq initialization in kern/subr_sleepqueue
Status: Closed Works As Intended
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: freebsd-threads (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-07 10:55 UTC by Bulat
Modified: 2016-08-04 19:40 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Bulat 2016-06-07 10:55:33 UTC
File: kern/subr_sleepqueue.
File revision: r300109
Routine: sleepq_add.

Repeated initialization of sq to td->td_sleepqueue in 327 and 347 lines with no other intervening assignments of other sleepqueue to sq. Probably, compiler optimizes this out.
Comment 1 Bulat 2016-06-07 11:09:32 UTC
td_sleepqueue in thread structure marked as (k) - accessed only by curthread, thus no need to repeatedly reassign same value to sq.
Comment 2 Konstantin Belousov freebsd_committer 2016-06-13 15:53:32 UTC
(In reply to Bulat from comment #1)
Earlier assignment is performed only under invariants, i.e. in the debugging kernel.  Later one is for the normal control flow.

So yes, for the invariants-enabled kernel the second assignment is excessive, but there is no point of optimizing for INVARIANTS, which performs lot of algorithmically unneeded work to make more robust checks.
Comment 3 John Baldwin freebsd_committer freebsd_triage 2016-08-04 19:40:44 UTC
If the INVARIANTS block didn't have to define a variable we probably wouldn't do it twice.  We could fix it but it would have to be a bit uglier:

#ifdef INVARIANTS
   int i;
#endif

   sq = td->td_sleepqueue;
#ifdef INVARIANTS
   for (i = 0; ...) {
   }
   ...
   sq->sq_lock = lock;
#endif
   ...

If our style permitted C99-type 'for (int i' that would be a simple way to
solve it, but it doesn't.