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: add get_addrs(), get_sizes() #2822

Merged
merged 1 commit into from Oct 13, 2021
Merged

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented Sep 14, 2021

Pull Request Overview

It seems like we might want to separate process printing from the actual process implementation. One challenge in doing that is getting the internal state of the process necessary to display information about the process. This provides a consolidated way to get insight about how memory is used by the process.

Rather than having individual getters for each item, this starts by grouping the values into "addresses" and "sizes". This approach seems more scalable than having individual functions, as any new values can be added to the relevant struct.

This is part of a larger effort to create a "process printer" trait and implementation separate from ProcessStandard. However, it is a standalone change, and can be discussed independently.

Testing Strategy

todo

TODO or Help Wanted

Comments?

We may want to remove the duplicated getters as part of this PR, or we may not. I wasn't sure, so I left them for now.

Documentation Updated

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

Formatting

  • Ran make prepush.

These two functions provide all debugging sizes and addresses in structs
rather than individual getters.
@hudson-ayers
Copy link
Contributor

Because these methods are part of the process trait, adding this adds 175 bytes to the kernel even if the methods are not called. That is not a ton, so I don't feel super strongly about it, but wanted to provide the data point. Given progress on ProcessPrinter has somewhat-stalled, do you still think these are worth adding?

@bradjc
Copy link
Contributor Author

bradjc commented Sep 27, 2021

Did you remove all or most of the redundant member functions?

And ProcessPrinter works for the memory map (#2826), and we decided that the trait based version was better than hardcoding it in process_standard.rs.

@hudson-ayers
Copy link
Contributor

Did you remove all or most of the redundant member functions?

And ProcessPrinter works for the memory map (#2826), and we decided that the trait based version was better than hardcoding it in process_standard.rs.

Nope, I just looked at the benchmarks on this PR as-is, so you're right the overhead is probably less. Ok, that makes sense.

@hudson-ayers
Copy link
Contributor

Should we remove the redundant member functions before merging this, or should that be done as part of #2826 ?

@bradjc
Copy link
Contributor Author

bradjc commented Oct 4, 2021

I have a start here: https://github.com/tock/tock/compare/process-remove-dup-addr-fns?expand=1

The kernel crate compiles, but the capsules does not because of the process console usage. However, I think many of those will go away with #2826. So maybe the linked branch can serve as a down payment, and we can handle the rest after the process printer changes settle.

@hudson-ayers
Copy link
Contributor

SGTM

@bradjc
Copy link
Contributor Author

bradjc commented Oct 7, 2021

Because these methods are part of the process trait, adding this adds 175 bytes to the kernel even if the methods are not called. That is not a ton, so I don't feel super strongly about it, but wanted to provide the data point. Given progress on ProcessPrinter has somewhat-stalled, do you still think these are worth adding?

On hail, I'm seeing a 112 byte reduction after removing the redundant process getters.

@hudson-ayers
Copy link
Contributor

Marking last-call

@hudson-ayers hudson-ayers added the last-call Final review period for a pull request. label Oct 7, 2021
@hudson-ayers
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 13, 2021

@bors bors bot merged commit ccccdf7 into master Oct 13, 2021
@bors bors bot deleted the process-get-addrs-sizes branch October 13, 2021 16:28
bors bot added a commit that referenced this pull request Oct 13, 2021
2862: kernel: process: remove duplicate address functions r=ppannuto a=bradjc



### Pull Request Overview

No longer need individual functions since `get_addresses()` provides them all.

This is on top of #2826 and #2822.


### Testing Strategy

travis


### TODO or Help Wanted

n/a


### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make prepush`.


Co-authored-by: Brad Campbell <bradjc5@gmail.com>
@granaghan granaghan mentioned this pull request Nov 1, 2021
2 tasks
bors bot added a commit that referenced this pull request Nov 23, 2021
2885: Stored state r=hudson-ayers a=granaghan

### Pull Request Overview

This pull request adds/changes/fixes...
Adds the ability to to serialize StoredState. Along with PR #2822, this enables crash information to be exported and fetched on a later boot.

### Testing Strategy

This pull request was tested by...
Tested export, import, and print of RISC-V StoredState.


### TODO or Help Wanted

This pull request still needs...


### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.
N/A?

### Formatting

- [x] Ran `make prepush`.


Co-authored-by: Brian Granaghan <granaghan@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel last-call Final review period for a pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants