init: Make entry init-function less and introduce service objects #90014
Conversation
tbursztyka
added
RFC
github-actions
bot
requested review from
andyross,
ceolin,
cfriedt,
dcpleung,
nashif,
npitre,
peter-mitsis and
TaiJuWu
tbursztyka
force-pushed
the
init_fn_out
branch
2 times, most recently
from
75c4116 to
5d04385
Compare
| static inline bool is_entry_about_service(const void *obj) | ||
| { | ||
| return (obj >= (void *)_service_list_start && | ||
| obj < (void *)_service_list_end); |
There was a problem hiding this comment.
This per-item test seems needless. You're sorting them in the linker already as a "sub-section" of an existing contiguous level. No need to check every item, just walk the prefix that is "devices" and the suffix which is "services", no?
There was a problem hiding this comment.
I am afraid I don't see what you are seeing. Service and device presence in init entries is shuffled, so one after another an init entry might point to a service or a device. What am I missing?
There was a problem hiding this comment.
It might be possible to do what Andy is suggesting, but I think it would require initializing devices either before or after services. Given that some services likely do depend on devices being initialized and some do not depend on devices being initialized, it might also require multiple passes through the service area to look only for certain priorities before, and then only certain priorities after.
The way it's done in this PR is probably the lesser evil of the two but it does add a few extra instructions to be executed before every init routine. So there is maybe a small price to pay here in terms of speed and the trade-off is to gain some amount of space back.
So @tbursztyka - this PR is kind of a space-optimization, unless I'm misunderstanding.
If you quantize the space savings, it is is the size of 1 void * per device, is that right?
So on 32-bit system with 16 devices, we would be 64 bytes in .rodata and gaining maybe 32 bytes in .text?
Of course the growth in .text is O(1) whereas the savings in .rodata is O(n), in terms of number of devices, so in the end, the space savings should be worth it.
Is the space savings worth it at the expense of additional time needed for init?
Maybe I'm looking into it too much 😅
What do you think @peter-mitsis ?
There was a problem hiding this comment.
Is the space savings worth it at the expense of additional time needed for init?
.text size depends on the architecture, so on x86 it's a mere 12bytes added, on arm cortex_m0 it's 20bytes.
I don't think timing is going to be badly impacted by such check. (it's on if statement added, and a new indirection for services). We are probably talking about µs of difference in overall.
It's a small gain, one could argue it's negligible but there are numerous place where rom/ram size is loosely taken into account (it always surprises me that ppl developing for embedded systems are sometimes not aligning structures for instance, nor using bits instead of bytes whenever it makes sense to pack things up etc...). So such places here + there + also there etc... we end up consuming quite much data that could be useful for something else. Take #90009 for instance and add it to this current one. Continuing your example it's already 128 bytes gained on devices only. Little by little, it starts to make a difference.
Now from a design point of view, I believe my proposal makes more sense as well. init_entry is only about sorting out objects, which objects then provide the means to be initialized etc...
On further changes, in the future, we could even generalize the init operation, thus removing altogether the if statement I added to discriminate object type. (that would mean however that service init fn would take a const void * parameter as well etc...)
Since the addition of deinit operation in device, init and deinit have been within each device, rendering their init entry's init function useless. In order to save ROM space, let's remove the init function from init entry altogether, and introduce a new object called "service" which owns an init function to go along with SYS_INIT/SYS_INIT_NAMED. Signed-off-by: Tomasz Bursztyka <tobu@bang-olufsen.dk>
As the pointer has been removed from struct init_entry, there is no point to look for it nor to display any information about it (obviously). Fixing the test script as well. Signed-off-by: Tomasz Bursztyka <tobu@bang-olufsen.dk>
check_init_priorities.py does no longer output any information about any initialization function pointer as such pointer has been removed from struct init_entry. Signed-off-by: Tomasz Bursztyka <tobu@bang-olufsen.dk>
struct init_entry may point to a struct device or a struct service, each ones belonging to device or service iterable section. Service section being newly added, handling it in cmake relevantly. Signed-off-by: Tomasz Bursztyka <tobu@bang-olufsen.dk>
|
Up for a discussion. But I am still planning on removing the priority from the device driver model (and generate it automatically), following what I started here #79340, and service is going to be used also then. (I foresee that we could even have more complicated 'service' that can be stopped/restarted at run-time etc... all with device dependencies, thus being able to react on relevant PM events and so on.)