sched: New scheduler interface definition and implementation scheme
Needs ReviewPublic
Actions

Authored by olce on Thu, Sep 24, 2:06 PM.

Details

Reviewers

emaste
kib

Group Reviewers

scheduler

Summary

Development of this new scheme was prompted by the following goals:

The scheduler interface should be defined once and for all (in 'sys/sys/sched.h') and all code duplication related to it (function signatures, slot names, dispatch, scheduler instance declaration) should be removed, making it easier to modify the interface or to add new schedulers.
The implementation functions in all schedulers should be renamed back to their original names. Changing these names was not really necessary technically. It introduced unwanted churn and cognitive burden, and inconsistencies with the existing documentation (mostly in the form of code comments) which had not been updated. Additionally, this was in contradiction with the "shim" (transparent, lightweight) intended nature of the dispatch mechanism.
Re-establish inlining opportunities in internal scheduler code without further cluttering the existing code (which changing it to use scheduler-specific names would do).

The chosen implementation brings additional benefits, which are
described below.

Point 1 is implemented by defining the interface as X macros, so that
lists of function declarations, field names in 'struct sched_instance'
and assignments to these fields, as well as the dispatch code, can be
generated simply from the interface description. These X macros are
passed a macro that is "called" with a variable number of arguments
describing a single function of the interface. The convention used for
a function's parameters is that each parameter is represented with two
macro arguments, the first one being the type and the second one being
the name. Helper macros allow to process arguments described in this
convention in order to generate a list of arguments for function
definitions (currently, up to 4 parameters). The chosen convention
eliminates the need for any specific declaration of functions depending
on their number of arguments. It is future-proof in that it also allows
listing the argument names without their types simply. (It also forces
the use of typedefs for complex argument types, if some ever appear.)

Point 2 and 3 are implemented, on one hand, by declaring interface
functions as having external linkage (in 'sys/sys/sched.h'), providing
corresponding dispatching implementations using ifuncs (as in the
original mechanism; in 'sys/kern/sched_shim.c'), and on the other hand,
by declaring functions with same name but internal linkage when
'sys/sys/sched.h' is included by a scheduler implementation (via the
definition of a specific macro prior to inclusion).

DECLARE_SCHEDULER() has been changed to automatically declare and fill
a 'struct sched_instance' structure. Its number of parameters has been
reduced to the minimum, and the symbol part used to produce distinct
structure names can now start with a digit (useful for 4BSD).

The new implementation has the following additional benefits:

The name of implementations for the interface functions is imposed (same name as the interface function) and checked at compile-time.
Missing functions in an implementation are detected at compile-time (and not at runtime with a NULL dereference).
Failures to fill correctly the 'struct sched_instance' object associated to an implementation are eliminated.

Preserve comments/categories of interface functions by moving comments
that were in the old explicit interface declarations to the SCHED_ITF*()
macros, and while here, marginally improve some (and move the common
documentation for sched_initticks() from the implementations to the
interface). Several of them are not exact, but this will be fixed in
a separate commit.

Make the active scheduler instance object internal, as the purpose of
the interface is precisely to hide the actual implementation.

Fixes: ce38acee8d0b ("Add kern/sched_shim.c") (+ some followups)

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 77278
Build 74161: arc lint + arc unit

Event Timeline

olce created this revision.Thu, Sep 24, 2:06 PM

Herald added a subscriber: imp. · View Herald TranscriptThu, Sep 24, 2:06 PM

olce requested review of this revision.Thu, Sep 24, 2:06 PM

Harbormaster completed remote builds in B77278: Diff 187604.Thu, Sep 24, 2:06 PM

kib added inline comments.Fri, Sep 25, 3:43 AM

sys/kern/sched_4bsd.c
680	The very first thing I object to is having more than one function with the same name in kernel. It complicates setting breakpoints. It makes it impossible to usefully read backtraces involving that functions. After all, it adds churn to the source. And if your goal was to avoid having to list SLOTs for each scheduler definition, simply concat the scheduler name into the function name prefix, like `sched_` SCHED_NAME function_name. Then indeed scheduler definitions could be folded into a macro that expands into set of SLOTs.

olce added inline comments.Fri, Sep 25, 7:27 AM

sys/kern/sched_4bsd.c
680	It complicates setting breakpoints. How is that? The debugger will just put breakpoints in all functions by the same name. It makes it impossible to usefully read backtraces involving that functions. Not in this case. The only difference is that an external caller will produce a backtrace where the same `sched_()` name appears in two adjacent frames in the call stack, instead of having `sched_NAME_()`. It's not a big deal since only one scheduler is active at a time. Having the scheduler name in functions actually confuses code browsing. Tools like ctags are not able to make a link between `sched_()` and `sched_NAME_()`, so you can't easily list all references (internal references are omitted) nor jump to an implementation. And even for humans it is not straightforward, since dispatchers are burried inside macros (with or without this change). After all, it adds churn to the source. It's amusing that you're using this as an argument against the change whereas your previous change did exactly that. If damage has been done to downstream projects, it was back then, a new change will just be some "icing on the cake", not the "cake" itself. Yes, I approved your change, but in retrospect that was a mistake. And if your goal was to avoid having to list SLOTs for each scheduler definition, simply concat the scheduler name into the function name prefix, like `sched_` SCHED_NAME function_name. Then indeed scheduler definitions could be folded into a macro that expands into set of SLOTs. My goals are very clearly stated at start of the commit message. What you suggest is already done here (except for adding `SCHED_NAME`, obviously).

Hide active_sched.

Harbormaster completed remote builds in B77319: Diff 187668.Fri, Sep 25, 12:06 PM

olce marked an inline comment as done.Fri, Sep 25, 12:06 PM

Syntactically, my preferred way of dealing with this kind of situation is device driver-like declaration where you put required function pointers in a structure and call DECLARE_SCHEDULER. During startup after the scheduler is selected, the kernel can check whether all function pointer members aren't null and panic if there is any null pointer. Although this is weaker than compile-time check, the implementation is clean in a sense of "textbook C".

But I understand why this is needed. I had enough with namespacing scheduler functions which becomes quite annoying when we add new sched function or scheduler. Even for functions that do not overlap with other schedulers (e.g. schedcpu), I always wanted to put sched_ namespacing but also worried it might confuse readers that it belongs to the interface not to a specific scheduler.

So I think documentation is important here. I was going to rewrite scheduler.9 to reflect current scheduler functions and the shim layer anyways. It would be great if you can do it (at least the shim part) as a followup of this revision.

sys/kern/sched_4bsd.c
659–661	Why is this removed? Even if it needs to be, I think it should be a separate review.
sys/kern/sched_ule.c
1658–1660	ditto

kib added inline comments.Fri, Sep 25, 9:59 PM

sys/kern/sched_4bsd.c
680	It complicates setting breakpoints. How is that? The debugger will just put breakpoints in all functions by the same name. Which debugger? gdb and lldb? Might be. ddb not by default. I have no idea and did not tried simics embedded tools for this situation. It makes it impossible to usefully read backtraces involving that functions. Not in this case. The only difference is that an external caller will produce a backtrace where the same `sched_()` name appears in two adjacent frames in the call stack, instead of having `sched_NAME_()`. It's not a big deal since only one scheduler is active at a time. It has nothing to do with adjacent frames. If I see sched_ule_sswitch in the backtrace, I know what scheduler and even more important, what mode of the thread locks was used. If instead I observe sched_sswitch() then I need to interrogate the source of backtrace to see how the kernel was configured. Having the scheduler name in functions actually confuses code browsing. Tools like ctags are not able to make a link between `sched_()` and `sched_NAME_()`, so you can't easily list all references (internal references are omitted) nor jump to an implementation. And even for humans it is not straightforward, since dispatchers are burried inside macros (with or without this change). After all, it adds churn to the source. It's amusing that you're using this as an argument against the change whereas your previous change did exactly that. If damage has been done to downstream projects, it was back then, a new change will just be some "icing on the cake", not the "cake" itself. Yes, I approved your change, but in retrospect that was a mistake. Yes I did introduced the churn for the reason. And adding more churn when there are technical reasons for not doing it is not right. And if your goal was to avoid having to list SLOTs for each scheduler definition, simply concat the scheduler name into the function name prefix, like `sched_` SCHED_NAME function_name. Then indeed scheduler definitions could be folded into a macro that expands into set of SLOTs. My goals are very clearly stated at start of the commit message. What you suggest is already done here (except for adding `SCHED_NAME`, obviously). Well no but I do not see it as a problem, If you want to do it (as part of this review indicates) you can do it. But there is stuff that I object to loudly.