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

counters ipc: handle single-task dumps correctly #477

Merged
merged 5 commits into from
Apr 17, 2024
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Apr 16, 2024

Currently, humility counters ipc will die if run against a dump that only contains a single task. This is because because it attempts to load the task table from the kernel in order to read task generations/restart counts, and returns an error if the task table can't be loaded.

This branch changes humility counters ipc to handle single-task dumps more gracefully. Now, if we can't load the task table, we just print a warning and continue on with an empty task table. We won't print generations or restart counts in this case, since there's no way to determine that from a dump of a single task, but we will still show the counters, which seems strictly better than the current behavior.

For example:

$ cargo run -- -d tests/cmd/cores/hubris.core.u16-ringbuf counters ipc
   Compiling humility-cmd-counters v0.1.0 (/home/eliza/Code/oxide/humility/cmd/counters)
   Compiling humility v0.11.4 (/home/eliza/Code/oxide/humility)
    Finished dev [unoptimized + debuginfo] target(s) in 6.46s
     Running `target/debug/humility -d tests/cmd/cores/hubris.core.u16-ringbuf counters ipc`
humility: attached to dump
humility: WARNING: failed to load task table: read of 4704 bytes from invalid address: 0x24000490
humility: WARNING: no generations/restart counts will be displayed.
humility: note: this may be a single-task dump.
drv_spi_api::__SPI_CLIENT_COUNTERS
 fn Spi::write() .................................................... 530 calls
    clients:
    task gimlet_seq .................................... = 0 ......... = 530 ok

 fn Spi::exchange() ................................................... 5 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 5 ok

 fn Spi::lock() ....................................................... 4 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 4 ok

 fn Spi::release() .................................................... 1 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 1 ok

drv_stm32xx_sys_api::__SYS_CLIENT_COUNTERS
 fn Sys::gpio_configure_raw() ........................................ 12 calls
    clients:
    task gimlet_seq .................................... = 0 .......... = 12 ok

 fn Sys::gpio_set_reset() ............................................ 12 calls
    clients:
    task gimlet_seq .................................... = 0 .......... = 12 ok

 fn Sys::gpio_read_input() ............................................ 5 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 5 ok

task_jefe_api::__JEFE_CLIENT_COUNTERS
 fn Jefe::set_state() ................................................. 1 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 1 ok

task_packrat_api::__PACKRAT_CLIENT_COUNTERS
 fn Packrat::set_spd_eeprom() ........................................ 32 calls
    clients:
    task gimlet_seq .................................... = 0 .......... = 32 ok

 fn Packrat::set_mac_address_block() .................................. 1 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 1 ok

 fn Packrat::set_identity() ........................................... 1 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 1 ok

This branch depends on @mkeeter's PR #476, because that branch added a failing dump to the test suite, and I wanted to update the test output from that dump.

@hawkw hawkw requested review from cbiffle and mkeeter April 16, 2024 16:52
Base automatically changed from u16-ringbuf to master April 16, 2024 18:52
Currently, `humility counters ipc` will die if run against a dump that
only contains a single task. This is because because it attempts to load
the task table from the kernel in order to read task generations/restart
counts, and returns an error if the task table can't be loaded.

This branch changes `humility counters ipc` to handle single-task dumps
more gracefully. Now, if we can't load the task table, we just print a
warning and continue on with an empty task table. We won't print
generations or restart counts in this case, since there's no way to
determine that from a dump of a single task, but we will still show the
counters, which seems strictly better than the current behavior.

For example:

```console
$ cargo run -- -d tests/cmd/cores/hubris.core.u16-ringbuf counters ipc
   Compiling humility-cmd-counters v0.1.0 (/home/eliza/Code/oxide/humility/cmd/counters)
   Compiling humility v0.11.4 (/home/eliza/Code/oxide/humility)
    Finished dev [unoptimized + debuginfo] target(s) in 6.46s
     Running `target/debug/humility -d tests/cmd/cores/hubris.core.u16-ringbuf counters ipc`
humility: attached to dump
humility: WARNING: failed to load task table: read of 4704 bytes from invalid address: 0x24000490
humility: WARNING: no generations/restart counts will be displayed.
humility: note: this may be a single-task dump.
drv_spi_api::__SPI_CLIENT_COUNTERS
 fn Spi::write() .................................................... 530 calls
    clients:
    task gimlet_seq .................................... = 0 ......... = 530 ok

 fn Spi::exchange() ................................................... 5 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 5 ok

 fn Spi::lock() ....................................................... 4 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 4 ok

 fn Spi::release() .................................................... 1 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 1 ok

drv_stm32xx_sys_api::__SYS_CLIENT_COUNTERS
 fn Sys::gpio_configure_raw() ........................................ 12 calls
    clients:
    task gimlet_seq .................................... = 0 .......... = 12 ok

 fn Sys::gpio_set_reset() ............................................ 12 calls
    clients:
    task gimlet_seq .................................... = 0 .......... = 12 ok

 fn Sys::gpio_read_input() ............................................ 5 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 5 ok

task_jefe_api::__JEFE_CLIENT_COUNTERS
 fn Jefe::set_state() ................................................. 1 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 1 ok

task_packrat_api::__PACKRAT_CLIENT_COUNTERS
 fn Packrat::set_spd_eeprom() ........................................ 32 calls
    clients:
    task gimlet_seq .................................... = 0 .......... = 32 ok

 fn Packrat::set_mac_address_block() .................................. 1 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 1 ok

 fn Packrat::set_identity() ........................................... 1 calls
    clients:
    task gimlet_seq .................................... = 0 ........... = 1 ok

```

This branch depends on @mkeeter's PR #476, because that branch added a
failing dump to the test suite, and I wanted to update the test output
from that dump.
@hawkw hawkw enabled auto-merge (squash) April 16, 2024 19:17
@hawkw hawkw merged commit e94bc0d into master Apr 17, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants