SYSINIT: add explicit SI_ORDER_LAST
ClosedPublic
Actions

Authored by glebius on Fri, Aug 7, 10:18 PM.

Details

Reviewers

markj
emaste

Group Reviewers

Src Committers
srcmgr
transport

Commits

rG662497d5a741: SYSINIT: add explicit SI_ORDER_LAST

Summary

Working on cleansing use of (SI_SUB_FOO + 1) construct through the kernel
I found a repeating pattern. Often a developer adds a module that depends
on certain subsystem to be fully instantiated and they want to put their
module SYSINIT right at the end of the SI_SUB_FOO. Such module usually
expects that nothing else within this subsystem shall depend on the
module.

The problem with SI_ORDER_ANY which practically was "the last" until this
change is that it is used very widely and people treat it literally as
"any", well, because this is what the name says. This lead to many parts
that could have dependencies later to be added as SI_ORDER_ANY.

So, our developer with the new subsystem that depends on SI_SUB_FOO has
three options:

Use SI_ORDER_ANY, but grep around ther kernel for other SI_SUB_FOO

entries to make sure that no dependencies are set to SI_ORDER_ANY. And in
case they are, shift them up and recheck if dependencies of those
dependencies are met.

Take next subsystem in sysinit list. However, the next one can be

SI_SUB_BAR, that is completely irrelevant from SI_SUB_FOO, and our
developer doesn't want to put his module's SYSINIT into SI_SUB_BAR, cause
it is ugly.

Use the (SI_SUB_FOO + 1) construct that violates -Werror=assign-enum.

The SI_ORDER_LAST solves this hard choice. If you know that nothing is
going to depend on your module within SI_SUB_FOO, but you depend on
SI_SUB_FOO, just use SI_ORDER_LAST.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

glebius created this revision.Fri, Aug 7, 10:18 PM

Herald added a reviewer: transport. · View Herald TranscriptFri, Aug 7, 10:18 PM

Herald added subscribers: linuxkpi, olce, melifaro and 3 others. · View Herald Transcript

glebius requested review of this revision.Fri, Aug 7, 10:18 PM

Harbormaster completed remote builds in B75442: Diff 183640.Fri, Aug 7, 10:18 PM

Herald added a subscriber: obrien. · View Herald TranscriptFri, Aug 7, 10:18 PM

glebius added a child revision: D58710: usb: use SYSINIT(9) order instead of using non-existing subsystem.Fri, Aug 7, 10:18 PM

I somewhat dislike the name SI_ORDER_LAST because it suggests it should be the final one in the list, and seems to be in conflict if two SYSINITS share a SI_SUB_WHATEVER, SI_ORDER_LAST. Would SI_ORDER_END be reasonable?

In D58709#1348174, @emaste wrote:

I somewhat dislike the name SI_ORDER_LAST because it suggests it should be the final one in the list, and seems to be in conflict if two SYSINITS share a SI_SUB_WHATEVER, SI_ORDER_LAST. Would SI_ORDER_END be reasonable?

I think SI_ORDER_LAST is fine, it mirrors SI_ORDER_FIRST and is neither more nor less ambiguous.

Thanks, Dag-Erling. Yes, END would be good if we had BEGIN and we don't have it.

I'd also like to augment the documentation. Right now SYSINIT(9) doesn't document orders at all. I will write something like:

SI_ORDER_FIRST ... SI_ORDER_FIFTH provide explicit order and should be used for those subsystem parts that are being dependencies of parts that are loaded later.
SI_ORDER_ANY is executed after all explicit orders and those parts may depend on parts with explicit ordering. Parts executed at SI_ORDER_ANY shall not be dependencies of other parts, but there are historical violations of that rule.
SI_ORDER_LAST is executed last and parts under this order shall never be dependencies of other parts within the same subsystem.

I'll add documentation to the diff once I see general agreement with this change.

markj accepted this revision as: markj.Tue, Aug 11, 1:45 PM

emaste accepted this revision as: emaste.Tue, Aug 11, 2:01 PM

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Aug 11, 2:41 PM

Closed by commit rG662497d5a741: SYSINIT: add explicit SI_ORDER_LAST (authored by glebius). · Explain Why

This revision was automatically updated to reflect the committed changes.

glebius added a commit: rG662497d5a741: SYSINIT: add explicit SI_ORDER_LAST.

Just a general comment.

This regularization of SI_ORDER and SI_SUB is a very a good improvement.

But based on the descriptions of the changes, it seems that magic numbers (even indirected by magic constants) are not the best way to order dependencies.
Maybe subsystems need the same dependency declaration mechanism that modules have.
Or, perhaps, subsystems can even be defined as some kind of modules.

In D58709#1348972, @avg wrote:

Just a general comment.

This regularization of SI_ORDER and SI_SUB is a very a good improvement.

But based on the descriptions of the changes, it seems that magic numbers (even indirected by magic constants) are not the best way to order dependencies.
Maybe subsystems need the same dependency declaration mechanism that modules have.
Or, perhaps, subsystems can even be defined as some kind of modules.

I was thinking of rcorder(8) for kernel. Of course run at compilation time, not an startup.

I'm not taking this task :) I just want to make kernel -Werror-assing-enum clean.

I was thinking of rcorder(8) for kernel. Of course run at compilation time, not an startup.

I'm not taking this task :) I just want to make kernel -Werror-assing-enum clean.

This is the sort of thing that used to be, or would be a great idea for those old Project Ideas or Kernel Tasks/Wanted Features that used to exist somewhere. A brief with how rcorder works, the code location, how this could done at compile time, similar works, other short ideas on this could get started, what testing would be required, potential pitfalls and whatnot would be great.