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

kernel: process: try to use memory fixed address #1928

Merged
merged 1 commit into from Aug 6, 2020

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented Jun 12, 2020

Pull Request Overview

This pull request adds logic to process.rs to make it try to find a matching memory address if a specific address was requested by the app. Right now all it tries to do is push the app back in memory (i.e. start the app's memory region at a later address).

This also changes the return values from create(). This fixes the trace process loading debugging messages so that the correct memory region is printed if an app does not start at the beginning of the memory region.

Testing Strategy

Running multiple apps on arty-e21.

TODO or Help Wanted

n/a

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

@bradjc bradjc added the kernel label Jun 12, 2020
Comment on lines 1643 to 1646
// We ran out of memory. Return the original range since (in
// this step) we were unable to meet the process's
// requirements.
(remaining_app_memory, remaining_app_memory_size)
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing. Why would this not return an error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought was that this step is very narrowly defined in the scope of what it can do. However, perhaps the MPU step could figure something better out that would support the application.

Comment on lines 1657 to 1659
// Address already matches, or is earlier in memory, nothing we
// have to do (or can do).
(remaining_app_memory, remaining_app_memory_size)
Copy link
Member

Choose a reason for hiding this comment

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

Same question, why not return an error here?

// Right now, we only support skipping some RAM and leaving a chunk
// unused so that the memory region starts where the process needs it
// to.
let (new_memory_start, new_memory_size) =
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let (new_memory_start, new_memory_size) =
let (requested_memory_start, requested_memory_size) =

"new" is confusing.., whether fixed or not, I think maybe "requested" better expresses that here it's what the loader is requesting.

Now that there are variations of "memory_start", the variables below should probably also change to be something more specific, maybe:

-let (memory_start, memory_size) = match chip.mpu().allocate_app_memory_region(
+let (actual_memory_start, actual_memory_size) = match chip.mpu().allocate_app_memory_region(

@bradjc bradjc added the blocked Waiting on something, like a different PR or a dependency. label Jun 16, 2020
@bradjc
Copy link
Contributor Author

bradjc commented Jun 16, 2020

I'll mark this blocked for now, since it really requires the updates to libtock-c to be usable, and there are other, more pressing, issues to work on in process.rs at the moment.

@bradjc
Copy link
Contributor Author

bradjc commented Jul 14, 2020

I have updated this for after #2022 is merged.

@bradjc bradjc force-pushed the kernel-process-find-memory-address branch from 15ff477 to cd20475 Compare July 23, 2020 19:17
@bradjc bradjc force-pushed the kernel-process-find-memory-address branch from cd20475 to 15ff477 Compare August 3, 2020 17:11
@bradjc bradjc removed the blocked Waiting on something, like a different PR or a dependency. label Aug 3, 2020
@bradjc bradjc force-pushed the kernel-process-find-memory-address branch from 15ff477 to 694021b Compare August 3, 2020 17:18
@bradjc
Copy link
Contributor Author

bradjc commented Aug 3, 2020

Ok! This is ready to go now.

@bradjc bradjc added the release-blocker Issue or PR that must be resolved before the next release label Aug 6, 2020
@bradjc
Copy link
Contributor Author

bradjc commented Aug 6, 2020

@ppannuto This is the last piece (I think) for basic fixed address support to be reasonably usable.

Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

bors r+

I imagine there's more intelligence that will come with time to this given that it's kind of wasteful currently; but it looks good enough for the current state of the universe

@bors bors bot merged commit 4de06c1 into master Aug 6, 2020
@bors bors bot deleted the kernel-process-find-memory-address branch August 6, 2020 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel release-blocker Issue or PR that must be resolved before the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants