Support ringbufs with `count: u16` #476

mkeeter · 2024-04-16T13:28:52Z

This is a small fix, followed by a zillion lines of new test suite output (because I added a dump with a u16-flavored ringbuf).

Fixes #475; bumps the Humility version to 0.11.4

mkeeter · 2024-04-16T14:03:07Z

@hawkw I'm not sure if the counters ipc tests should be failing on the new archive, e.g.

$ humility -d tests/cmd/cores/hubris.core.u16-ringbuf counters ipc
humility: attached to dump
humility counters failed: read of 4704 bytes from invalid address: 0x24000490

Is that expected for a single-task dump?

mkeeter · 2024-04-16T14:08:02Z

Ah, it looks like we load the full task table:

        let task_table = {
            let (base, task_count) = hubris.task_table(core)?;
            let task_t = hubris.lookup_struct_byname("Task")?.clone();
            humility_doppel::Task::load_tcbs(
                hubris,
                core,
                base,
                task_count as usize,
                &task_t,
            )?
        };

which isn't populated for single-task dumps.

We could make this command more flexible, only loading the task table if available (and using a dummy value for generation otherwise), but that shouldn't block this PR.

hawkw

I was planning on fixing this this morning — thanks for taking care of it for me! I had a couple minor comments.

Regarding support for single-task dumps in counters ipc, good catch! I'll go ahead and fix that separately?

hawkw · 2024-04-16T16:05:11Z

humility-doppel/src/lib.rs

+        match v.as_base()? {
+            Base::U16(v) => Ok(Self(u32::from(*v))),
+            Base::U32(v) => Ok(Self(*v)),
+            _ => bail!("not a u32 or u16: {v:?}"),


Take it or leave it: should we just go ahead and support any integer here, as well? Just in case we start using u8 or u64 counters later?¹

Footnotes

This is probably unlikely, but...be conservative in what you admit, but liberal in what you accept? ↩

Sure, I bumped the internal size up to a u64 and made it accept any unsigned integer (bdd3992)

hawkw · 2024-04-16T16:07:48Z

humility-doppel/src/lib.rs

    pub payload: Value,
 }

+#[derive(Clone, Debug)]
+pub struct RingbufCount(pub u32);


Note that as of oxidecomputer/hubris#1733, ringbuf counts can also be () if de-duplication is disabled. This shouldn't currently occur in the wild yet, as we aren't actually disabling deduplication anywhere, but we ought to start handling the case where there are no counts, too. I'm fine with doing that separately if you prefer not to do it in this PR.

Switched to an Option<u64> (also in bdd3992). We should get a unit test for this at some point, but this is fine for now IMHO

@mkeeter

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 · 2024-04-16T16:53:12Z

Ah, it looks like we load the full task table:

        let task_table = {
            let (base, task_count) = hubris.task_table(core)?;
            let task_t = hubris.lookup_struct_byname("Task")?.clone();
            humility_doppel::Task::load_tcbs(
                hubris,
                core,
                base,
                task_count as usize,
                &task_t,
            )?
        };

which isn't populated for single-task dumps.

We could make this command more flexible, only loading the task table if available (and using a dummy value for generation otherwise), but that shouldn't block this PR.

#477 fixes this; it depends on this branch in order to update the tests for the dump you've added. So, once we get this merged, we can merge the fix for single-task dumps as well.

@mkeeter

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.

@mkeeter

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.

mkeeter added 2 commits April 16, 2024 09:22

Add support for ringbuf with u16 count

61822a3

Add core and trycmd tests; bump version

34890f0

mkeeter requested a review from hawkw April 16, 2024 13:28

Mark some unit tests as failure-expected

6f650be

hawkw approved these changes Apr 16, 2024

View reviewed changes

hawkw mentioned this pull request Apr 16, 2024

counters ipc: handle single-task dumps correctly #477

Merged

Tweaks from review

bdd3992

mkeeter enabled auto-merge (squash) April 16, 2024 18:27

mkeeter merged commit 20e6160 into master Apr 16, 2024
11 checks passed

mkeeter deleted the u16-ringbuf branch April 16, 2024 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ringbufs with `count: u16` #476

Support ringbufs with `count: u16` #476

mkeeter commented Apr 16, 2024 •

edited

Loading

mkeeter commented Apr 16, 2024

mkeeter commented Apr 16, 2024

hawkw left a comment

hawkw Apr 16, 2024

mkeeter Apr 16, 2024

hawkw Apr 16, 2024

mkeeter Apr 16, 2024

hawkw commented Apr 16, 2024

Support ringbufs with count: u16 #476

Support ringbufs with count: u16 #476

Conversation

mkeeter commented Apr 16, 2024 • edited Loading

mkeeter commented Apr 16, 2024

mkeeter commented Apr 16, 2024

hawkw left a comment

Choose a reason for hiding this comment

hawkw Apr 16, 2024

Choose a reason for hiding this comment

Footnotes

mkeeter Apr 16, 2024

Choose a reason for hiding this comment

hawkw Apr 16, 2024

Choose a reason for hiding this comment

mkeeter Apr 16, 2024

Choose a reason for hiding this comment

hawkw commented Apr 16, 2024

Support ringbufs with `count: u16` #476

Support ringbufs with `count: u16` #476

mkeeter commented Apr 16, 2024 •

edited

Loading