Skip to content
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

core: split out library code #17652

Merged
merged 6 commits into from
Mar 10, 2022
Merged

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Feb 14, 2022

Contribution description

This commit splits core into it's scheduler/IPC part and into other code
that is either used or uses the scheduler through defined APIs.

I consider this a cleanup. Some code in core/ is actually sys/ stuff that we just happen to keep in core/ to ensure changing it is done with care. This code is now moved to core/lib.
Default include paths default modules have been adapted accordingly. Kconfig fixes are probably still necessary.

Why this is useful:

  1. bootloaders don't necessarily use core/, so allowing that to not get build makes some things simpler

  2. There's one prototype Rust reimplementation of core that works almost as drop in, which is made possible by this PR which requires some way to not build the C version. The split from this PR enables that.

Testing procedure

Issues/PRs references

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 14, 2022
@github-actions github-actions bot added Area: boards Area: Board ports Area: build system Area: Build system Area: core Area: RIOT kernel. Handle PRs marked with this with care! Platform: native Platform: This PR/issue effects the native platform labels Feb 14, 2022
@bergzand
Copy link
Member

With the risk of starting bikeshedding, we could also move out the threading and scheduling code into a kernel directory and leave the library-like core bits where they are. What do you think?

@kaspar030 kaspar030 added Process: needs >1 ACK Integration Process: This PR requires more than one ACK Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Feb 14, 2022
@kaspar030
Copy link
Contributor Author

With the risk of starting bikeshedding, we could also move out the threading and scheduling code into a kernel directory and leave the library-like core bits where they are. What do you think?

you mean doing it the other way around? keep what's now in core/lib/ in core/, and move what's now in core/ into core/sched (or whatever?)

@bergzand
Copy link
Member

you mean doing it the other way around? keep what's now in core/lib/ in core/, and move what's now in core/ into core/sched (or whatever?)

Exactly, or move it to /kernel

@kaspar030
Copy link
Contributor Author

Exactly, or move it to /kernel

hm, I'd rather have the actual "core" in core/, and it's auxillary stuff hidden in a subfolder. that makes more sense to me. the "important", "look at me first" code should be positioned more prominently.

what would be the advantage of turning this around in this PR?

(I wish we could have more than one module in a single folder, but not with the current build system)

@benpicco
Copy link
Contributor

benpicco commented Mar 1, 2022

+1 on the general idea, just one question: Would it make sense to move the remainder of what is now in core/ into core/sched/?
The idea would be to make it possible to then add a dummy implementation core/no_sched that can be used by riotboot.

Of course this can also be achieved by disabling the individual modules and providing the dummy implementation directly in the header (edf6019, abc30d6) but it could allow for a cleaner solution.

@kaspar030
Copy link
Contributor Author

Would it make sense to move the remainder of what is now in core/ into core/sched/?

Then core/ would be essentially empty, now.

The idea would be to make it possible to then add a dummy implementation core/no_sched that can be used by riotboot.

what would that no_sched do? dummy implementations for the mutex/msg/thread functions? we've discussed this before (there was the idea of a single-threaded core). that does not make much sense, as, a stripped-down core (to one thread) doesn't really have much overhead here. we're talking almost no memory and a few hundred bytes of code, heavily stripped automatically by the linker. like, if mutexes and msgs are not used, they don't use up code size and negligible amounts of RAM.
riotboot is IMO not yet a proper use case, as it doesn't link/use core currently. And, it is not size constrained, as it usually has two whole flash pages for itself (in order to evenly divide the rest for equal sized slots), and it fits cosily on our platforms.

So, I'd suggest we cross that bridge once someone has sketched an alternative, in-tree implementation that would make this necessary.

@benpicco
Copy link
Contributor

benpicco commented Mar 3, 2022

This is not about size alone: Riotboot does not enter thread mode, so mutex_lock() will not work. What works is busy-waiting on a variable that gets set by an ISR - this still works in Riotboot.

@kaspar030
Copy link
Contributor Author

kaspar030 commented Mar 9, 2022

This is not about size alone: Riotboot does not enter thread mode, so mutex_lock() will not work. What works is busy-waiting on a variable that gets set by an ISR - this still works in Riotboot.

Yeah, what I mean is, as long as riotboot is fine not using mutex_lock(), it can just not use core as it does now. If it does at some point need mutex_lock(), there's not much reason not to use the current scheduler configured to only one thread, vs. a second implementation that does the same but differently. Right?
(I'm looking for in-tree use cases of alternative scheduler implementations that would justify a move to core/sched).

What I do see is that e.g., "kernel_init()" is not really a core_lib candidate. So there could be "core/" containing mostly that and maybe rmutex and scheduler independent stuff that's not pure library code, "core/sched" with the scheduling code, and "core/lib" for the auxillary libs (bitarithm, clist, ...) used by core/sched.

What do you think?

@benpicco
Copy link
Contributor

benpicco commented Mar 9, 2022

The 'bootloader' implementation of mutex_lock() can be really simple.
There is no reason to enter thread mode just for that (and then exit it again at the end of riotboot?).

But I'm not insisting on a separate core implementation, the static inline works fine - I just thought it would be a bit cleaner.

@kaspar030
Copy link
Contributor Author

kaspar030 commented Mar 9, 2022

The 'bootloader' implementation of mutex_lock() can be really simple.
There is no reason to enter thread mode just for that (and then exit it again at the end of riotboot?).

yeah, that's simple.

But I'm not insisting on a separate core implementation, the static inline works fine - I just thought it would be a bit cleaner.

I'm not insisting on not having a seperat core impl. for that. :)

I don't know - how to move forward with this. I think moving this PR's core/ to core/sched, then having core empty, doesn't improve things, but causes a cycle of move code/cleanup commits/test/change and in RIOT-rs an update/cleanup/test which I'd like to avoid/postpone. But I'm ok with doing that if reviewers see a need.

@benpicco
Copy link
Contributor

benpicco commented Mar 9, 2022

Heh I just thought it would be an easy request that could make my life easier down the line - but I didn't even try do pursue that path. I'll just stick to disabling core_mutex for now - please rebase (and squash)

core/lib/init.c Outdated Show resolved Hide resolved
This commit splits core into it's scheduler/IPC part and into other code
that is either used or uses the scheduler, through defined APIs.
Copy link
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack!

@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Mar 10, 2022
@bergzand
Copy link
Member

And go!

@bergzand bergzand merged commit e59a678 into RIOT-OS:master Mar 10, 2022
@bergzand bergzand deleted the split_core_lib branch March 10, 2022 19:17
@kaspar030
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: Kconfig Area: Kconfig integration Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: native Platform: This PR/issue effects the native platform Process: needs >1 ACK Integration Process: This PR requires more than one ACK Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants