-
Notifications
You must be signed in to change notification settings - Fork 6.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Init entries: Automatically computing priorities #73836
base: main
Are you sure you want to change the base?
Conversation
5567fd5
to
29a42d2
Compare
Example on how things are working as it should: tests/misc/check_init_priorities now fails because there is no error induced (the test is expecting an error), since the hard-coded priorities are no longer used. |
29a42d2
to
14ed1f0
Compare
This scripts does not have any meaning since priorities are not written by hand anymore. As long as priorities were written by hand, it was indeed required to verify that the ordering was in par with what DTS generated in its dependency tree. It was indeed possible to induce errors by assigning a priority that would break this dependency ordering. Now that all is generated automatically, hand-to-hand with DTS, it can no longer happen. Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@proton.me>
As for DTS generated header, unittest needs to generate an empty fwconfig generated header. Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@proton.me>
f7ebb7d
to
4949b41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't had time for a full review. But a design whine: do we really want this in YAML?
That's literally adding a whole extra source language to produce a binary. It's no longer enough to read the C+dts+kconfig files to understand what the code is going to do, you need to find and read the yaml files too. I think that's sort of a poor trade.[1]
Can't this be done in the existing dts tree? The data structure is robust enough, it supports overlays already (though maybe not at the same granularity as the yaml file locations?). We could have a "mixin" style thing where you could annotate existing nodes with init ordering metadata, etc...
I don't really love DTS either but I do think it would be better to have one bad syntax than two different ones.
You mean something like: #23394 ? ;) The issue is that DTS is only meant to describe hardware and hardware only. I recall getting a big NO about that PR in a meeting at that time due to that rule, precisely. Unless things have changed since? |
did not go deep into this, but here a few thoughts and few concerns:
This pretty much keeps SYS_INIT intact. With a change as fundemnetal as this, do we really want to bandaid the issue like this and keep SYS_INIT the same way if we had the change to actually overhaul the entire thing now? The issue is that SYS_INIT is one of the the most abused macros which contributes to inconsistencies in the boot process already. All board/soc SYS_INIT calls can be replaced with fixed platform hooks (#62595), all kernel SYS_INIT calls can also be replaced with fixed init calls and hooks, those will never change and they are expected to be called the same way, always. using SYS_INIT is wrong (will post a PR to show how we can remove all SYS_INIT usages from kernel/). And it does not end here... If we spend some time on fixing the issue at the right place, we should be left with a small portion of the "problem" which might change how the solution would like at the end.... Also see #73908 which tries to remove usage of SYS_INIT in arch code...
If using a dedicated configuration file is the way to go, did you look into defining this with a file per service? for example
Why do we need to add this as a level? We are trying to clean up the levels. We probably need some more levels to create some meaningful things to depend on and run in and remove some of the hacks we have now (merge PRE_KERNEL_1/_2, remove SMP) etc.... |
See description "Why are the files called fw_config.yaml?".
Resolving dependencies has to be done before compilation. DTS does already that job for all hardware devices, the problem that remained was the system entries (SYS_INIT): without a mean to describe them and the related dependencies, there is no way you can solve the whole dependency tree. YAML is already widely used in Zephyr thus the choice toward that syntax. I never looked at parsing binaries, that's really not the right path for such issue.
That's a question I asked myself as well, if all SYS_INIT entries are described (like hw devices are through DTS), we could apply way more changes then. (we could get rid of all levels for instance).
I have been following these PR yes. (It's actually a nice coincidence it's been tackled now)
I did have something like that in mind at first, but again the number of SYS_INIT everywhere was an issue. So instead I applied the rule of root file + "overlays" that can rewrite or tweak what's in the root. Doing so adds up flexibility but also a bit of complexity since you have to know where to apply a modification.
enabled/disabled, sure why not (it is shorter/simpler actually), but you still need a place where to put the actual init entries. |
Fair enough, though in this case we're right on the boundary. And DTS as currently implemented has absolutely been inching closer to a general software configuration language (c.f. "chosen" phandles, disabled flags, @edersondisouza 's recent PRs dealing with deferred initialization). So maybe that decision might be different.[1] Alternatively, do it in C? Write some macros that emit a init dependency record, then a python script that collates them after an early linker pass, computes the needed magic, and emits it as an ordered SYS_INIT list before the final link? [1] For the record: I absolutely think DTS should be strictly limited to a description of the hardware on which software will be configured to run. But we walked away from that model a while ago. |
Actually thinking about this:
That's not too far from the way it works now, just that the "magic" is some kludgey sorting in a linker script. You could augment SYS_INIT() to emit a more involved descriptor right were it is currently, then the script just emits the legacy SYS_INIT() records for the benefit of the final link. Though again, that sort is sort of a dirty trick and I think python could do it cleaner and better. |
Yes I really prefer sticking with python. Easier to have a human readable description, easier to tweak... Also because it is ran before any compilation/linking happens, so it is possible to detect inconsistencies asap. |
@nashif and it grows almost every week, we are now at 448 ones https://pastebin.com/NBQhHAX1 |
one more of those #74219 |
TLDR; I'd again like to point to Inversion of Control containers from the (web) application programming world as an additional source of inspiration for dependency, lifecycle and configuration management. The most well-known is the Spring IoC container but several others have proven the concept over decades. We recently discussed a similar approach for the configuration of the network subsystem and I see similar conceptual problems in this PR. If we go down this route, then we'll soon have C+dts+kconfig+settings subsys+Zephyr specific yaml for net settings+Zephyr specific yaml for dependency management. The latter two accompanied by custom scripts that are more akin to ad-hoc band-aids than implementing battle-hardened concepts. The big con for any new approach is that we further obfuscate dependencies through one more layer of indirection as has been pointed out. But if we have to go the way of abstraction anyway, then I propose we re-use tried and trusted, well-documented and well-understood concepts. The IoC container concept solves the configuration, dependency and lifecycle/PM management requirements in one approach:
It'll still be a big challenge to adapt the concept to the embedded world, e.g. by doing most of the work before and during build, minimal RAM/ROM footprint, mapping dependency and configuration information to an easy-to-learn file hierarchy (maybe s.th. like This is obviously not a fully thought-through solution proposal. It is meant as a thought-provoking side comment, so please bear with me if I overlooked or mis-represented requirements I'm not aware of. |
one more #74228 |
@fgrandel
How is writing dependencies in a human-readable way obfuscating things even more against arbitrary chosen hard-coded priority number? Quite the contrary, actually.
You are adding way way more requirements to what is being addressed in this PR. I want to fix things incrementally, bits by bits. Now it's the priority, and that is all. Side note, this PR adds a new script but it removes also another one in the process (check_init_priorities.py), so that's not much added in the build after all. |
Right, but the overwhelming majority of those are dependency-free, and don't need any special treatment. They're just lazy drivers and subsystems that don't want to puzzle out the DTS macros, and copied an init level like "POST_KERNEL" from some other code. Seems like a little attention to those would go a long way to avoid the need for all the yaml, which still gives me an ick. |
Like, what if we deprecated the priority and level entirely, documented that no ordering is provided for SYS_INIT() hooks, and focused on removing/augmenting/replacing only the SYS_INIT() calls that are actually trying to do something special, so that they can get their ordering from DTS? |
Yes, thus my remark on the useless 1:1 mapping of those in this pr description. And finally I got CI pass with just a very few nodes in yaml.
Thus bringing more software specific stuff into DTS? Isn't this going against what we stated previously? |
Architecture WG:
|
From the last meeting, now that the overall idea has been detailed and discussed, here if the changes I would already incorporate:
or just dts, it would be shorter, though less self-documenting (but do we really need to be very specific?), i.e:
? Before implementing anything however, I would like to focus the next discussion on these particular elements:
Would that make sense?
|
Architecture WG:
|
@tbursztyka can you post the cocci script you mentioned somewhere? |
I uploaded to you in discord, did you get it? |
Architecture WG:
|
Closing it as this superseded by #79340 |
Introduction
In Zephyr, forcing an initialization order is working along 2 concepts: level and priority.
Where level is a higher order rank made of 6 pre-installed sections and priority a number between 0 and 99 made to fine tune the ordering within the selected section.
For a detailed information on these levels, please refer to https://docs.zephyrproject.org/latest/kernel/drivers/index.html#initialization-levels
Initialization entries exists in 2 types: device and system. Where device represents an instance of a hardware driver, system on the other hand can be interpreted as a software service to be started automatically at boot time.
In the end, each initialization entry gets ordered by its level and priority number as a sub-section of the level section they belong to.
This is, so far, totally fine. Except that all is hard-coded by hand, and orbiting around an abstract number: priority.
Why can this be a problem?
If an ordering issue happens, it will need to be fixed by hand. And since it is an abstract number, it may require a few trial and error.
However, thanks to DTS, we already know most of the device dependency ordering: as long as a device is described via DTS, DTS generates an ordinal representing the position of the device among the others, taking into account its dependencies. Such ordinal is already in use for fine tuning the devices ordering in sections. (see #22545 for more information on how such ordinal came into the picture)
Great, but that leaves out all devices which are not described via DTS and more importantly: system initialization entries which are completely left on the side, with no means to express any actual dependency either against devices or against other system entries. Not to mention the hard coded priority is always there even for DTS based device.
Isn't there Kconfig to help?
Most, if not all, priorities are set via Kconfig generated macros after all (CONFIG_*_PRIORITY).
However, the used CONFIG_ is most of the time a generic one (like CONFIG_KERNEL_INIT_PRIORITY_DEVICE), hoping that it fits the purpose. Getting one per-driver or system entry would just clutter Kconfig. Note as well that the term "priority" is also used for preempt/coop threads and thus all this can become quickly confusing.
And again, there is no way to describe a dependency between a device and a system entry. (There has been an attempt recently to relate DTS and Kconfig to address this issue, and this issue only: see #71470 )
Solution
What is being proposed here is:
All with the necessary flexibility and transparency to address all forseeable use-cases (DTS based devices, non-DTS based devices, system entries, ...)
Obviously at built time, before any compilation happens.
How is the solution implemented
What is going to be introduced is a script which will take advantage of both DTS and Kconfig, add its own information to generate a full dependency tree and from there: to derive the right priority for each initialization entry.
In the end, exactly like what is being found in devicetree_generated.h, zephyr will find the necessary initialization information in a generated header.
On user side, the information that will be at play are nodes described by:
Such node information is stored in YAML format, an already widely used format within Zephyr.
The term "node" has been taken from DTS. At this stage, we are not dealing with initialization entry: these only exist in the code and in the compiled objects, thus why "entry" is not used to describe a YAML object. Each YAML object is thus a node, and such node exists to describe a dependency and/or a level change about an initialization entry.
There is no need, anywhere, to get a 1:1 mapping of initialization entries vs YAML nodes. There are 446 system entries in Zephyr (i.e. SYS_INIT() calls). There is no point in getting these 446 entries a counter part node in YAML.
The end goal is to write in YAML only the relevant information.
For the user, this also means no abstract number such as priority will ever be manipulated: only clear and human readable information about what needs what in order to function properly.
Getting to the details
Information about the initialization nodes is thus written in YAML, following a pre-determined name: fw_config.yaml. It can, as for DTS overlays, exist also board's specific ones: _fw_config.yaml.
At built time, cmake will look for these in that specific order:
Relevantly found YAML files will be used, along with DTS nodes (in EDT pickled object) to recreate a dependency tree. Which tree exist under the form of a graph (the same graph library used in EDT as well). From that graph, we get the ordinal for each node which becomes - in the end - the node priority under the form of a usable macro in a generated header file.
Typically a YAML node will be like:
A realistic example could then be:
enable_shell_uart is a system entry, created in subsys/shell/backends/shell_uart.c line 556. uart0 is a DTS node.
If the goal would be to get a generic description for enable_shell_uart, then there would be an issue: uart0 might be valid for one build, but not for another (could be uart1 ..) Thus, when relevant, it is possible to use DTS aliases and chosen nodes.
So above example would become:
Finally, this is a fully generic description of the dependency between the system entry enable_shell_uart and the device entry it requires.
Let's see a different use-case where 2 nodes are as follow:
By default in the code, their level of initialization has been set to PRE_KERNEL_1. Let's say now that service_boot_phase1 has to be moved on POST_KERNEL just for the current application:
This is entirely feasible. And generates various output:
This level information will also generate relevant macros that will be used then, surperceeding the default hard-coded level in the code. Note however a slight difference in behavior: an explicit level will always replace the default. An implicit level on the other hand will replace the default only if it is a higher level than the default.
Wrong level vs dependencies such as:
will be detected and will generate an error: since service_boot_phase2 needs service_boot_phase1, which one starts at level POST_KERNEL, it is out of question that itself starts at level EARLY.
Since we are dealing with possible errors, dependency loops are also detected:
It is an impossible dependency tree to create and will generate an error.
Optional feature
Kconfig generated options can be interpreted as well to tune level and dependencies in a node. This is a yet-to-be properly formalized, but it is already possible to affect level and dependencies depending on enabled options via Kconfig.
For instance:
Is valid, and will set an explicit PRE_KERNEL_1 level if CONFIG_EARLY_CONSOLE is set. (which is the exact hard-coded behavior in uart_console.c btw).
It could be made mode complicated (currently only one if/then is allowed with a minimalistic unique option treated)
This Kconfig integration feature is, I think, an interesting one but would deserve more thoughts: 2+ if/then possible, more complicated if statements like: (CONFIG_1 and CONFIG_2) or CONFIG_3 ?
Or perhaps it is a rabbit hole...
This feature could be put on the side at first, and reintroduced later if needed.
Addition of a MANUAL level
DTS, since just a short while ago, can force a initialization entry in a dedicated section which will not be treated at boot time. This is to fix an actual need, but doing it through DTS was not exactly the right way since it introduced a zephyr specific software property in DTS. (the DTS property "zephyr,deferred-init"). And DTS is only meant to describe the hardware.
Now, with what is being introduced here, it is simply fixed via adding a "MANUAL" level. It is much more zephyr-ish as it follows the already existing level section naming scheme, no new macros needed such as Z_DEFER_DEVICE_INIT_ENTRY_DEFINE() etc...
And via a fw_config.yaml, such level can be set like for instance:
Rest is as usual. Up to the user to call the device initialization function.
Why are the files called fw_config.yaml?
It is indeed a very generic name and there is a point to it.
At first, fixing this initialization ordering issue was thought to be tackled on its own script, with its own files. Let's face it however: the place where this procedure happens is not only valid for the initialization entries, but for any type of zephyr tweaks where neither Kconfig nor DTS can have a role to play.
There is already things happening like #68127 and perhaps more to come. So instead of multiplying the files, it is more relevant to gather all of this in a central place. Whether the same script should do all - via modules - like what is being proposed here, is open to debate as well. Files could be normalized under one unique naming scheme, but totally different scripts could handle them.
What would need to be done next?
Thank to @carles and @gmarull for reviewing the first prototype and bringing constructive ideas on this.