Paths

Table of Contentst

sched: New scheduler interface definition scheme
ClosedPublic
Actions

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

Details

Reviewers

emaste
kib
andrew
manu
mchoo

Group Reviewers

scheduler

Commits

rG42490d5cd29d: sched: New scheduler interface definition scheme

Summary

Define the scheduler interface once and for all (in 'sys/sys/sched.h')
and remove all code duplication related to it (function signatures, slot
names, dispatch, scheduler instance declaration), making it easier to
modify the interface or to add new schedulers.

This is implemented by defining the interface as X macros, which are
passed a macro (and additional arguments for it) 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 function parameters). The chosen convention eliminates the need for
any specific declaration of functions depending on their number of
arguments.

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 but with the scheduler symbol that was passed to DECLARE_SCHEDULER() added just after the 'sched_' prefix, e.g., for ULE, 'sched_ule_*()') 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 77319
Build 74202: 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.

olce added a parent revision: D60167: ifuncs: Have DEFINE_*IFUNC() macros expand the passed name.Wed, Sep 30, 9:26 AM

Keep implementation namespacing for function implementations.
Produce the name of interface functions from their name without the sched_ prefix.

Herald added a reviewer: andrew. · View Herald TranscriptWed, Sep 30, 9:29 AM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added a reviewer: manu. · View Herald Transcript

Herald added subscribers: riscv, jhibbits. · View Herald Transcript

Harbormaster completed remote builds in B77520: Diff 188167.Wed, Sep 30, 9:29 AM

@kib (switching from inline comments to main comments, makes it easier to follow)

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.

gdb and lldb will put a breakpoint in each function with same name. gdb does report multiple functions with info functions <name>, but does not do so with info symbol <name> (which is understandable given what this command does; but may be surprising). lldb reports all of them with image lookup -s. disass disassembles all functions in lldb, but unfortunately not in gdb (without indicating that there is another function).

So you're fearing that Simics tools could not cope with this situation. I've had an offline discussion with markj@ about kernel debugging, and it appears that, in general, external tools do not deal well with multiple symbols with same name in the same object file. So it seems that having different symbols causes less friction with these tools.

I was willing to evolve ddb so that it at least sets breakpoints to every candidate, and add to it a show scheduler command showing the scheduler instance, and still am, if there is still value to do it.

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.

Yes, but you could do that as well with the show scheduler command evoked above. Anyway, I'm giving up on the idea of having same symbol names for all implementations for now.

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

It appears to me that this point, along with ensuring that compiler sees inline opportunities for interface functions in a scheduler implementation, could still be solvable with some kind of source namespacing. Basically, a header would be generated per implementation that contains lines like #define sched_clock sched_ule_clock and would be included by this implementation only. That way, the non-namespaced names would be used in the source, but the symbols would still be namespaced. Would that work for you? (This would be a separate change.)

I've also added changes to ensure that there's no conflicts with C++ keywords (suffixing struct sched_instance fields with _p, and not producing names of arguments in sched.h (class would be a problem)), and will ask des@ to confirm it's fine once the diff is settled.

sys/kern/sched_4bsd.c
659–661	Since I've rewritten the interface functions' declaration and move the existing documentation accordingly (from elsewhere in the header file), and because there was no documentation at all for `sched_initticks()` there, I just took it from both 4BSD and ULE. As you can see, the text was exactly the same on both cases (already a copy-paste), so it made more sense to put that common text into the interface. I've generally refrained from changing the functions' documentation in this revision, except marginally. I can separate the move here into a separate commit, although I'm not sure that has much value (but for the review itself, it certainly has).

olce marked an inline comment as done.Wed, Sep 30, 2:44 PM

LGTM, there are many small changes that would be in separate commits in normal cases, but since we are refactoring them anyways, I think they all can be squashed into this one commit.

sys/kern/sched_4bsd.c
659–661	Ah, it makes sense. I fine with both approaches.
1013	Do you have any idea why this was named sswtich in the first place? Would it be a typo?

This revision is now accepted and ready to land.Fri, Oct 2, 3:56 PM

Also, do you have plan to MFC this to stable/15? I don't see this being a breaking change to users and KPI/KBI.

kib accepted this revision.Sun, Oct 4, 11:53 AM

kib added inline comments.

sys/sys/sched.h
125	This is very obscure place to put the description of the functions. I would put it in separate comment which would at least also lists the explicit function names (in form sched_NAME_FUNC).

olce added inline comments.Mon, Oct 5, 2:29 PM

sys/sys/sched.h
125	Not completely sure what you mean here. Does `sched_NAME_FUNC()`, with `NAME` in the middle (the scheduler name?), refer to implementation functions? The comments are about the "interface", i.e., what the functions are supposed to do, regardless of the implementation. Having comments in the macros is slightly weird but this way they are close to the actual (high-level) "declarations", as they were before the change with respect to C declaration. Having instead a big comment block with these and interface function names (and also implementation names?) may be easier to the casual reader but is a bit against the direction taken here, where the goal is to avoid repetitions and with them the necessary increased effort to keep things coherent. Could you give a quick example of what you have in mind for one function, or a logical group of functions?

kib added inline comments.Tue, Oct 6, 7:56 AM

sys/sys/sched.h
125	I only mean that comments inside macro are obscure. I do not care what way the functions are named in the comment, only suggested to put the comment listing functions (named somehow) and grouping them by usage, would be put as a dedicated comment.

olce marked 5 inline comments as done.Tue, Oct 6, 1:39 PM

olce added inline comments.

sys/kern/sched_4bsd.c
1013	Answer (well, at least my point of view) at end of commit message.
sys/sys/sched.h
125	Ok. I did some try and that's a bit ugly to have duplicated comments, or comments only in the explicit list and not in the macros themselves. For now, I've opted instead for adding a preamble giving some examples of how to read `FUN()` lines and where functions are declared and documented. I have a separate change adding a list, will post it later, as I'm not exactly sure what you want this list for.