Changeset View
Changeset View
Standalone View
Standalone View
sys/kern/sched_ule.c
| Show First 20 Lines • Show All 286 Lines • ▼ Show 20 Lines | struct tdq { | |||||||||||
| u_char tdq_ts_off; /* (t) TS insertion offset. */ | u_char tdq_ts_off; /* (t) TS insertion offset. */ | |||||||||||
| u_char tdq_ts_deq_off; /* (t) TS dequeue offset. */ | u_char tdq_ts_deq_off; /* (t) TS dequeue offset. */ | |||||||||||
| /* | /* | |||||||||||
| * (t) Number of (stathz) ticks since last offset incrementation | * (t) Number of (stathz) ticks since last offset incrementation | |||||||||||
| * correction. | * correction. | |||||||||||
| */ | */ | |||||||||||
| u_char tdq_ts_ticks; | u_char tdq_ts_ticks; | |||||||||||
| int tdq_id; /* (c) cpuid. */ | int tdq_id; /* (c) cpuid. */ | |||||||||||
| int tdq_do_idle; | ||||||||||||
| struct runq tdq_runq; /* (t) Run queue. */ | struct runq tdq_runq; /* (t) Run queue. */ | |||||||||||
| char tdq_name[TDQ_NAME_LEN]; | char tdq_name[TDQ_NAME_LEN]; | |||||||||||
| #ifdef KTR | #ifdef KTR | |||||||||||
| char tdq_loadname[TDQ_LOADNAME_LEN]; | char tdq_loadname[TDQ_LOADNAME_LEN]; | |||||||||||
| #endif | #endif | |||||||||||
| }; | }; | |||||||||||
| /* Idle thread states and config. */ | /* Idle thread states and config. */ | |||||||||||
| ▲ Show 20 Lines • Show All 1,190 Lines • ▼ Show 20 Lines | llc: | |||||||||||
| /* | /* | |||||||||||
| * Try hard to keep interrupts within found LLC. Search the LLC for | * Try hard to keep interrupts within found LLC. Search the LLC for | |||||||||||
| * the least loaded CPU we can run now. For NUMA systems it should | * the least loaded CPU we can run now. For NUMA systems it should | |||||||||||
| * be within target domain, and it also reduces scheduling overhead. | * be within target domain, and it also reduces scheduling overhead. | |||||||||||
| */ | */ | |||||||||||
| if (ccg != NULL && intr) { | if (ccg != NULL && intr) { | |||||||||||
| cpu = sched_lowest(ccg, mask, pri, INT_MAX, ts->ts_cpu, r); | cpu = sched_lowest(ccg, mask, pri, INT_MAX, ts->ts_cpu, r); | |||||||||||
| if (cpu >= 0) | if (cpu >= 0) | |||||||||||
| SCHED_STAT_INC(pickcpu_intrbind); | SCHED_STAT_INC(pickcpu_intrbind); | |||||||||||
| } else | } else if (ccg != NULL) { | |||||||||||
| /* Search the LLC for the least loaded idle CPU we can run now. */ | /* | |||||||||||
| if (ccg != NULL) { | * Search the LLC for the least loaded idle CPU we can run now. | |||||||||||
| */ | ||||||||||||
olce: Strange that a language server complains about that... I don't see what is misleading with the… | ||||||||||||
Not Done Inline ActionsIt's actually complaining about the next if over (the global search for least loaded CPU) being indented too deeply, which I don't quite understand either. Possibly a bug in the LS.
Counterpoint is that if you read the subsequent "if" its not immediately obvious that its an else if and looks like it stands on its own.
I do like this option. What would you like to change in the comment's content though? I'm not claiming to understand this code entirely but it seems like the body of the if (ccg != NULL) is doing exactly what the comment describes, i.e. looking for the least loaded CPU in the LLC CPU group.
yup :) obiwac: It's actually complaining about the next if over (the global search for least loaded CPU) being… | ||||||||||||
| cpu = sched_lowest(ccg, mask, max(pri, PRI_MAX_TIMESHARE), | cpu = sched_lowest(ccg, mask, max(pri, PRI_MAX_TIMESHARE), | |||||||||||
| INT_MAX, ts->ts_cpu, r); | INT_MAX, ts->ts_cpu, r); | |||||||||||
| if (cpu >= 0) | if (cpu >= 0) | |||||||||||
| SCHED_STAT_INC(pickcpu_affinity); | SCHED_STAT_INC(pickcpu_affinity); | |||||||||||
| } | } | |||||||||||
| /* Search globally for the least loaded CPU we can run now. */ | /* Search globally for the least loaded CPU we can run now. */ | |||||||||||
| if (cpu < 0) { | if (cpu < 0) { | |||||||||||
| cpu = sched_lowest(cpu_top, mask, pri, INT_MAX, ts->ts_cpu, r); | cpu = sched_lowest(cpu_top, mask, pri, INT_MAX, ts->ts_cpu, r); | |||||||||||
| ▲ Show 20 Lines • Show All 57 Lines • ▼ Show 20 Lines | ||||||||||||
| * Pick the highest priority task we have and return it. | * Pick the highest priority task we have and return it. | |||||||||||
| */ | */ | |||||||||||
| static struct thread * | static struct thread * | |||||||||||
| tdq_choose(struct tdq *tdq) | tdq_choose(struct tdq *tdq) | |||||||||||
| { | { | |||||||||||
| struct thread *td; | struct thread *td; | |||||||||||
| TDQ_LOCK_ASSERT(tdq, MA_OWNED); | TDQ_LOCK_ASSERT(tdq, MA_OWNED); | |||||||||||
| if (__predict_false(tdq->tdq_do_idle)) | ||||||||||||
| return (NULL); /* Return NULL for idle thread */ | ||||||||||||
| td = runq_choose_realtime(&tdq->tdq_runq); | td = runq_choose_realtime(&tdq->tdq_runq); | |||||||||||
| if (td != NULL) | if (td != NULL) | |||||||||||
| return (td); | return (td); | |||||||||||
| td = runq_choose_timeshare(&tdq->tdq_runq, tdq->tdq_ts_deq_off); | td = runq_choose_timeshare(&tdq->tdq_runq, tdq->tdq_ts_deq_off); | |||||||||||
| if (td != NULL) { | if (td != NULL) { | |||||||||||
| KASSERT(td->td_priority >= PRI_MIN_BATCH, | KASSERT(td->td_priority >= PRI_MIN_BATCH, | |||||||||||
| ("tdq_choose: Invalid priority on timeshare queue %d", | ("tdq_choose: Invalid priority on timeshare queue %d", | |||||||||||
| td->td_priority)); | td->td_priority)); | |||||||||||
| ▲ Show 20 Lines • Show All 1,069 Lines • ▼ Show 20 Lines | sched_exit_thread(struct thread *td, struct thread *child) | |||||||||||
| */ | */ | |||||||||||
| thread_lock(td); | thread_lock(td); | |||||||||||
| td_get_sched(td)->ts_runtime += td_get_sched(child)->ts_runtime; | td_get_sched(td)->ts_runtime += td_get_sched(child)->ts_runtime; | |||||||||||
| sched_interact_update(td); | sched_interact_update(td); | |||||||||||
| sched_priority(td); | sched_priority(td); | |||||||||||
| thread_unlock(td); | thread_unlock(td); | |||||||||||
| } | } | |||||||||||
| void | static void | |||||||||||
| sched_preempt(struct thread *td) | sched_preempt_locked(struct tdq *tdq, struct thread *td) | |||||||||||
Done Inline Actions
Seems much simpler to continue not returning anything, and unlock as needed in the function. olce: Seems much simpler to continue not returning anything, and unlock as needed in the function. | ||||||||||||
Done Inline ActionsFeels kind of weird to mix responsibilities like this (i.e. having the caller lock and the callee unlock), but i suppose there's no real better way so long as mi_switch() can unlock anyways. obiwac: Feels kind of weird to mix responsibilities like this (i.e. having the caller lock and the… | ||||||||||||
| { | { | |||||||||||
| struct tdq *tdq; | ||||||||||||
| int flags; | int flags; | |||||||||||
| SDT_PROBE2(sched, , , surrender, td, td->td_proc); | ||||||||||||
| thread_lock(td); | ||||||||||||
| tdq = TDQ_SELF(); | ||||||||||||
| TDQ_LOCK_ASSERT(tdq, MA_OWNED); | TDQ_LOCK_ASSERT(tdq, MA_OWNED); | |||||||||||
| if (td->td_priority > tdq->tdq_lowpri) { | if (td->td_priority > tdq->tdq_lowpri) { | |||||||||||
| if (td->td_critnest == 1) { | if (td->td_critnest == 1) { | |||||||||||
| flags = SW_INVOL | SW_PREEMPT; | flags = SW_INVOL | SW_PREEMPT; | |||||||||||
| flags |= TD_IS_IDLETHREAD(td) ? SWT_REMOTEWAKEIDLE : | flags |= TD_IS_IDLETHREAD(td) ? SWT_REMOTEWAKEIDLE : | |||||||||||
| SWT_REMOTEPREEMPT; | SWT_REMOTEPREEMPT; | |||||||||||
| mi_switch(flags); | mi_switch(flags); | |||||||||||
| /* Switch dropped thread lock. */ | /* Switch dropped thread lock. */ | |||||||||||
| return; | return; | |||||||||||
Done Inline Actions
See previous inline comment. olce: See previous inline comment. | ||||||||||||
| } | } | |||||||||||
| td->td_owepreempt = 1; | td->td_owepreempt = 1; | |||||||||||
| } else { | } else | |||||||||||
| tdq->tdq_owepreempt = 0; | tdq->tdq_owepreempt = 0; | |||||||||||
| } | ||||||||||||
| thread_unlock(td); | thread_unlock(td); | |||||||||||
Done Inline Actions
See previous inline comment. olce: See previous inline comment. | ||||||||||||
| } | } | |||||||||||
| void | ||||||||||||
| sched_preempt(struct thread *td) | ||||||||||||
| { | ||||||||||||
| struct tdq *tdq; | ||||||||||||
| SDT_PROBE2(sched, , , surrender, td, td->td_proc); | ||||||||||||
| thread_lock(td); | ||||||||||||
| tdq = TDQ_SELF(); | ||||||||||||
| sched_preempt_locked(tdq, td); | ||||||||||||
| } | ||||||||||||
Done Inline Actions
olce: | ||||||||||||
| void | ||||||||||||
| sched_do_idle(struct thread *td, bool do_idle) | ||||||||||||
| { | ||||||||||||
| struct tdq *tdq; | ||||||||||||
| thread_lock(td); | ||||||||||||
| tdq = TDQ_SELF(); | ||||||||||||
| TDQ_LOCK_ASSERT(tdq, MA_OWNED); | ||||||||||||
| tdq->tdq_do_idle = do_idle; | ||||||||||||
| sched_preempt_locked(tdq, td); | ||||||||||||
| } | ||||||||||||
Done Inline Actions
olce: | ||||||||||||
| /* | /* | |||||||||||
| * Fix priorities on return to user-space. Priorities may be elevated due | * Fix priorities on return to user-space. Priorities may be elevated due | |||||||||||
| * to static priorities in msleep() or similar. | * to static priorities in msleep() or similar. | |||||||||||
| */ | */ | |||||||||||
| void | void | |||||||||||
| sched_userret_slowpath(struct thread *td) | sched_userret_slowpath(struct thread *td) | |||||||||||
| { | { | |||||||||||
| ▲ Show 20 Lines • Show All 137 Lines • ▼ Show 20 Lines | ||||||||||||
| struct thread * | struct thread * | |||||||||||
| sched_choose(void) | sched_choose(void) | |||||||||||
| { | { | |||||||||||
| struct thread *td; | struct thread *td; | |||||||||||
| struct tdq *tdq; | struct tdq *tdq; | |||||||||||
| tdq = TDQ_SELF(); | tdq = TDQ_SELF(); | |||||||||||
| TDQ_LOCK_ASSERT(tdq, MA_OWNED); | TDQ_LOCK_ASSERT(tdq, MA_OWNED); | |||||||||||
| td = tdq_choose(tdq); | td = tdq_choose(tdq); | |||||||||||
| KASSERT(td != PCPU_GET(idlethread), ("sched_choose: idle thread " | ||||||||||||
| "explicitly returned; tdq_choose should return NULL instead")); | ||||||||||||
| if (td != NULL) { | if (td != NULL) { | |||||||||||
Not Done Inline ActionsI'd move that at the end of tdq_choose() instead of here, even if this function currently has the only call to tdq_choose(). olce: I'd move that at the end of `tdq_choose()` instead of here, even if this function currently has… | ||||||||||||
Not Done Inline ActionsI guess that makes things a little more complicated as tdq_choose() doesn't have a single path where it returns the thread. I guess I could just do this check on the return after runq_choose_idle() though? obiwac: I guess that makes things a little more complicated as `tdq_choose()` doesn't have a single… | ||||||||||||
| tdq_runq_rem(tdq, td); | tdq_runq_rem(tdq, td); | |||||||||||
| tdq->tdq_lowpri = td->td_priority; | tdq->tdq_lowpri = td->td_priority; | |||||||||||
| } else { | } else { | |||||||||||
| tdq->tdq_lowpri = PRI_MAX_IDLE; | tdq->tdq_lowpri = PRI_MAX_IDLE; | |||||||||||
| td = PCPU_GET(idlethread); | td = PCPU_GET(idlethread); | |||||||||||
| } | } | |||||||||||
| tdq->tdq_curthread = td; | tdq->tdq_curthread = td; | |||||||||||
| return (td); | return (td); | |||||||||||
| ▲ Show 20 Lines • Show All 657 Lines • Show Last 20 Lines | ||||||||||||
Strange that a language server complains about that... I don't see what is misleading with the original form, except for indentation, where one can argue it doesn't matter here.
The new form is not style(9) compliant, and that I find really misleading :-) (a very quick glance does not see that the if has an else branch).
A way out which is imperfect but that I often use: Put the herald comment inside the block guarded by the if instead of before, possible modify the comment's content due to the new location, and format as prescribed by style(9).
In the end, keeping the original code is probably the best option. If doing some change here, it should anyway be a separate commit.