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

ringbuf: rewrite entry to have no panics #1624

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Conversation

cbiffle
Copy link
Collaborator

@cbiffle cbiffle commented Feb 21, 2024

This knocks about 120 bytes off of each ringbuf in a task (because the Ringbuf::entry routine gets specialized for each ringbuf, and this makes that routine smaller).

Reduces Gimlet flash image size by 3.4%.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This is lovely, and I really appreciate the comments (as usual!)

lib/ringbuf/src/lib.rs Show resolved Hide resolved
@hawkw hawkw mentioned this pull request Feb 22, 2024
2 tasks
@hawkw
Copy link
Member

hawkw commented Feb 22, 2024

Hmm, I have some bad news about this. After rebasing my branch from #1621 to use this code for Ringbuf::entry rather than the code on master, all my ringbufs appear to be empty:

eliza@cadbury ~ $ pfexec humility -t c71 ringbuf
humility: attached to 0483:3754:001100184741500820383733 via ST-Link V3
humility: ring buffer drv_gimlet_seq_server::__RINGBUF in gimlet_seq:
humility: ringbuf dump failed: member missing from struct: cell
humility: ring buffer drv_i2c_devices::adm1272::__RINGBUF in power:
humility: ring buffer drv_i2c_devices::max31790::__RINGBUF in thermal:
humility: ring buffer drv_i2c_devices::sbrmi::__RINGBUF in sbrmi:
humility: ring buffer drv_oxide_vpd::__RINGBUF in gimlet_seq:
humility: ring buffer drv_oxide_vpd::__RINGBUF in host_sp_comms:
humility: ring buffer drv_packrat_vpd_loader::__RINGBUF in gimlet_seq:
humility: ring buffer drv_sbrmi::__RINGBUF in sbrmi:
humility: ring buffer drv_stm32h7_spi_server_core::__RINGBUF in spi2_driver:
humility: ring buffer drv_stm32h7_spi_server_core::__RINGBUF in sprot:
humility: ring buffer drv_stm32h7_sprot_server::__RINGBUF in sprot:
humility: ring buffer drv_stm32xx_i2c::__RINGBUF in i2c_driver:
humility: ring buffer drv_stm32xx_i2c::__RINGBUF in spd:
humility: ring buffer drv_stm32xx_i2c_server::__RINGBUF in i2c_driver:
humility: ring buffer ksz8463::__RINGBUF in net:
humility: ring buffer ksz8463::__RINGBUF in host_sp_comms:
humility: ring buffer stm32h7_update_server::__RINGBUF in update_server:
humility: ring buffer task_control_plane_agent::CRITICAL in control_plane_agent:
humility: ring buffer task_control_plane_agent::__RINGBUF in control_plane_agent:
humility: ring buffer task_control_plane_agent::update::rot::__RINGBUF in control_plane_agent:
humility: ring buffer task_dump_agent::__RINGBUF in dump_agent:
humility: ring buffer task_dump_agent::udp::__RINGBUF in dump_agent:
humility: ring buffer task_hiffy::stm32h7::__RINGBUF in hiffy:
humility: ring buffer task_host_sp_comms::__RINGBUF in host_sp_comms:
humility: ring buffer task_jefe::dump::__RINGBUF in jefe:
humility: ring buffer task_jefe::external::__RINGBUF in jefe:
humility: ring buffer task_net::mgmt::__RINGBUF in net:
humility: ring buffer task_packrat::__RINGBUF in packrat:
humility: ring buffer task_power::__RINGBUF in power:
humility: ring buffer task_power::bsp::__RINGBUF in power:
humility: ring buffer task_spd::__RINGBUF in spd:
humility: ring buffer task_thermal::__RINGBUF in thermal:
humility: ring buffer task_validate::__RINGBUF in validate:
humility: ring buffer vsc85xx::__RINGBUF in net:

After changing my PR back to use the previous code, the ringbufs contain entries as expected:

eliza@cadbury ~ $ pfexec humility -t c71 ringbuf
humility: attached to 0483:3754:001100184741500820383733 via ST-Link V3
humility: ring buffer drv_gimlet_seq_server::__RINGBUF in gimlet_seq:
humility: ringbuf dump failed: member missing from struct: cell
humility: ring buffer drv_i2c_devices::adm1272::__RINGBUF in power:
 NDX LINE      GEN    COUNT PAYLOAD
   0  106        1        1 Config(CommandData(0x3f3f))
   1  159        1        1 Coefficients(Coefficients { m: 0xfde, b: 0x0, R: 0xfe })
   2  177        1        1 Coefficients(Coefficients { m: 0x297, b: 0x5000, R: 0xff })
   3  205        1        1 Coefficients(Coefficients { m: 0x2927, b: 0x0, R: 0xfd })
   4  106        1        1 Config(CommandData(0x3f3f))
   5  159        1        1 Coefficients(Coefficients { m: 0xfde, b: 0x0, R: 0xfe })
   6  177        1        1 Coefficients(Coefficients { m: 0x52e, b: 0x5000, R: 0xff })
   7  205        1        1 Coefficients(Coefficients { m: 0x524e, b: 0x0, R: 0xfd })
humility: ring buffer drv_i2c_devices::max31790::__RINGBUF in thermal:
humility: ring buffer drv_i2c_devices::sbrmi::__RINGBUF in sbrmi:
humility: ring buffer drv_oxide_vpd::__RINGBUF in gimlet_seq:
humility: ring buffer drv_oxide_vpd::__RINGBUF in host_sp_comms:
humility: ring buffer drv_packrat_vpd_loader::__RINGBUF in gimlet_seq:
humility: ring buffer drv_sbrmi::__RINGBUF in sbrmi:
humility: ring buffer drv_stm32h7_spi_server_core::__RINGBUF in spi2_driver:
 NDX LINE      GEN    COUNT PAYLOAD
  52  494     5450        1 WaitISR(0x32017)
  53  464     5450        1 Rx(0x0)
  54  464     5450        1 Rx(0x7)
  55  494     5450        1 WaitISR(0x10012)
  56  464     5450        1 Rx(0x6)
  57  322     5450        1 Start(exchange, (0x2, 0x4))

 < ... REDACTED FOR BREVITY ... >

(n.b. that the gimlet_seq ringbuf parse failure is because i'm using that ringbuf for testing my event counts change and should be expected).

I haven't tested just this branch yet, so it's possible that it's an interaction with my code, but I intend to shortly --- @cbiffle, have you actually tried this out yet?

Edit: Disregard all this, I messed up my rebase and ate the line that actually updates the ringbuf's last element. Whoopsie.

@cbiffle
Copy link
Collaborator Author

cbiffle commented Feb 22, 2024

Ringbufs with this change check out locally; issue appears to have been a conflict between our changes.

This knocks about 120 bytes off of each ringbuf in a task (because the
Ringbuf::entry routine gets specialized for each ringbuf, and this makes
that routine smaller).

Reduces Gimlet flash image size by 3.4%.
@cbiffle cbiffle enabled auto-merge (rebase) February 22, 2024 22:48
@cbiffle cbiffle merged commit 534855a into master Feb 22, 2024
83 checks passed
// 1. None of our target platforms currently have hardware modulus,
// and many of them don't even have hardware divide, making
// remainder quite expensive.
// 2. The code as written here correctly turns usize::MAX into 0 for
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the comment! I'd remove this bullet point, though, as this code would be correct regardless. Specifically, one presumes that with modulus, this code would be written as let ndx = last.wrapping_add(1) % self.buffer.len();. Since usize::MAX.wrapping_add(1) is 0, and 0 (mod N) is 0 for N != 0, this would still (correctly) be 0.

dancrossnyc added a commit that referenced this pull request Feb 23, 2024
The change that introduced this comment was
automerged, and thus didn't address the comment at
#1624 (comment).
This change just massages the comment a bit to address
that.
dancrossnyc added a commit that referenced this pull request Feb 23, 2024
The change that introduced this comment was
automerged, and thus didn't address the comment at
#1624 (comment).
This change just massages the comment a bit to address
that.
hawkw added a commit that referenced this pull request Feb 26, 2024
Ringbufs provide a fixed-size buffer for storing diagnostic events,
which may include data. This is very useful for debugging. However,
because the ring buffer has a fixed size, only the last `N` events are
visible at any given time. If a ringbuf has a capacity of, say, 16
events, and a given event has occurred 10,000 times over the lifespan of
the task, there's currently no way to know that the event has happened
more than 16 times. Furthermore, there's no way to use the ringbuf to
determine whether a given event has _ever_ occurred during the task's
lifetime: if a given variant is not _currently_ present in the ringbuf,
it may still have occurred previously and been "drowned out" by
subsequent events. Finally, some targets disable the ringbuf entirely,
for space reasons, and on those targets, it may still be desirable to be
able to get some less granular diagnostic data out of the ringbuf events
a task records.

Therefore, this commit extends the `ringbuf` crate to support generating
counts of the number of times a particular event variant has been
recorded, in addition to a fixed-size buffer of events. These counters
provide less detailed data than inspecting the ringbuf contents, because
they don't record the values of any _fields_ on that event. However,
they allow recording very large numbers of events in a fixed amount of
space, and provide historical data for events that may have occurred in
the past and then "fallen off" the end of the ringbuf as new events were
recorded. By inspecting these counters, we can determine if a given
variant has *ever* been recorded, even if it isn't currently in the
buffer, and we can see the total number of times it has been recorded
over the task's entire lifetime.

Event counters are implemented using a new `CountedRingbuf` type, which
marries a ring buffer with a type which can count occurances of entry
variants. `CountedRingbuf`s may be declared using the `counted_ringbuf!`
macro, and entries can be recorded using the existing `ringbuf_entry!`
macro.

`CountedRingbuf` requires that the entry type implement a new `Count`
trait, which defines the counter type, an initializer for creating new
instances of the counter type, and a method to increment the counter
type with a given instance of the entry type. An implementation of
`ringbuf::Count` can be generated for an entry type using the
`#[derive(ringbuf::Count)]` attribute, which generates a `struct` with
an `AtomicU32` field for each of the deriving `enum`'s variants. This
`struct` can then be loaded by Humility to provide a view of the event
counters.

Because recording event counters requires only `4 * <N_VARIANTS>` bytes,
counters are currently recorded even when the `ringbuf` crate has the
`disabled` feature set. This way, we can still record some diagnostic
data on targets that don't have the space to store the ringbuf itself.
If it becomes necessary to also disable counters, we could add a
separate `disable-counters` feature as well, so that a target can pick
and choose between recording ringbuf entries, counts, or both.

As a proof of concept, I've also updated `gimlet-seq-server` to use the
new event counters for its `Trace` ringbuf. Subsequent commits will roll
it out elsewhere.

Future work will include:

- [ ] Actually adopting event counters in existing ringbufs
- [ ] Updating `humility ringbuf` to also dump ringbuf event counts (see
  oxidecomputer/humility#449).

Depends on #1624, both to reduce merge conflicts and to mitigate an
increase in size due to the changes in this branch.
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.

None yet

3 participants