Skip to content

ereport quality of life improvements#2225

Merged
hawkw merged 3 commits into
masterfrom
eliza/ereport-qol
Sep 15, 2025
Merged

ereport quality of life improvements#2225
hawkw merged 3 commits into
masterfrom
eliza/ereport-qol

Conversation

@hawkw

@hawkw hawkw commented Sep 15, 2025

Copy link
Copy Markdown
Member

This branch makes several improvements to the packrat ereport APIs based on experience using them. It consists of three commits that make independent changes but which depend on each other. I'd like to rebase-merge this, and the three commits can be reviewed independently in series. The changes here are:

  1. packrat: return an error if an ereport was lost (183d155)

    This commit changes task-packrat-api's deliver_ereport IPC to return
    an error if the ereport did not fit in the buffer, rather than being
    infallible. In the future, this may allow some tasks to attempt
    re-delivery periodically until there's space.

  2. packrat: Add helper API for serde ereports (49710b0)

    In practice, so far, most ereport payloads have been defined as structs
    implementing serde::Serialize and encoded using the minicbor-serde
    adapter crate. This requires a bit of boilerplate code, so it seems
    worthwhile to provide a method that just does that. This commit adds one
    in task-packrat-api. It's feature-flagged so we can avoid compiling
    minicbor, minicbor-serde, and serde in tasks that don't submit
    ereports using this.

  3. cpu_seq: Encode ereports into static buffers (dc47a60)

    In this comment, @cbiffle reminded me that we can use
    ClaimOnceCell to use a static to create byte buffers for encoding
    ereports. This way, the buffer is never on the stack, reducing the risk
    of stack overflows when recording an ereport.

    This commit changes drv_gimlet_seq_server and drv_cosmo_seq_server
    to use that technique for their ereports.

hawkw added a commit that referenced this pull request Sep 15, 2025
In practice, so far, most ereport payloads have been defined as structs
implementing `serde::Serialize` and encoded using the `minicbor-serde`
adapter crate. This requires a bit of boilerplate code, so it seems
worthwhile to provide a method that just does that. This commit adds one
in `task-packrat-api`. It's feature-flagged so we can avoid compiling
`minicbor`, `minicbor-serde`, and `serde` in tasks that don't submit
ereports using this.
hawkw added a commit that referenced this pull request Sep 15, 2025
In [this comment][1], @cbiffle reminded me that we can use
`ClaimOnceCell` to use a `static` to create byte buffers for encoding
ereports. This way, the buffer is never on the stack, reducing the risk
of stack overflows when recording an ereport.

This commit changes `drv_gimlet_seq_server` and `drv_cosmo_seq_server`
to use that technique for their ereports.
hawkw added a commit that referenced this pull request Sep 15, 2025
This commit changes `task-packrat-api`'s `deliver_ereport` IPC to return
an error if the ereport did not fit in the buffer, rather than being
infallible. In the future, this may allow some tasks to attempt
re-delivery periodically until there's space.
hawkw added a commit that referenced this pull request Sep 15, 2025
In practice, so far, most ereport payloads have been defined as structs
implementing `serde::Serialize` and encoded using the `minicbor-serde`
adapter crate. This requires a bit of boilerplate code, so it seems
worthwhile to provide a method that just does that. This commit adds one
in `task-packrat-api`. It's feature-flagged so we can avoid compiling
`minicbor`, `minicbor-serde`, and `serde` in tasks that don't submit
ereports using this.
hawkw added a commit that referenced this pull request Sep 15, 2025
In [this comment][1], @cbiffle reminded me that we can use
`ClaimOnceCell` to use a `static` to create byte buffers for encoding
ereports. This way, the buffer is never on the stack, reducing the risk
of stack overflows when recording an ereport.

This commit changes `drv_gimlet_seq_server` and `drv_cosmo_seq_server`
to use that technique for their ereports.
hawkw added a commit that referenced this pull request Sep 15, 2025
In [this comment][1], @cbiffle reminded me that we can use
`ClaimOnceCell` to use a `static` to create byte buffers for encoding
ereports. This way, the buffer is never on the stack, reducing the risk
of stack overflows when recording an ereport.

This commit changes `drv_gimlet_seq_server` and `drv_cosmo_seq_server`
to use that technique for their ereports.

[1]: #2214 (comment)
@hawkw
hawkw marked this pull request as ready for review September 15, 2025 18:23
@hawkw hawkw changed the title [wip] ereport quality of life improvements ereport quality of life improvements Sep 15, 2025
@hawkw
hawkw requested review from cbiffle and mkeeter September 15, 2025 18:23
hawkw added a commit that referenced this pull request Sep 15, 2025
In [this comment][1], @cbiffle reminded me that we can use
`ClaimOnceCell` to use a `static` to create byte buffers for encoding
ereports. This way, the buffer is never on the stack, reducing the risk
of stack overflows when recording an ereport.

This commit changes `drv_gimlet_seq_server` and `drv_cosmo_seq_server`
to use that technique for their ereports.

[1]: #2214 (comment)
Comment thread drv/gimlet-seq-server/src/main.rs Outdated
Comment thread idl/packrat.idol Outdated

@mkeeter mkeeter left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A few nits, but looks reasonable!

This commit changes `task-packrat-api`'s `deliver_ereport` IPC to return
an error if the ereport did not fit in the buffer, rather than being
infallible. In the future, this may allow some tasks to attempt
re-delivery periodically until there's space.
In practice, so far, most ereport payloads have been defined as structs
implementing `serde::Serialize` and encoded using the `minicbor-serde`
adapter crate. This requires a bit of boilerplate code, so it seems
worthwhile to provide a method that just does that. This commit adds one
in `task-packrat-api`. It's feature-flagged so we can avoid compiling
`minicbor`, `minicbor-serde`, and `serde` in tasks that don't submit
ereports using this.
In [this comment][1], @cbiffle reminded me that we can use
`ClaimOnceCell` to use a `static` to create byte buffers for encoding
ereports. This way, the buffer is never on the stack, reducing the risk
of stack overflows when recording an ereport.

This commit changes `drv_gimlet_seq_server` and `drv_cosmo_seq_server`
to use that technique for their ereports.

[1]: #2214 (comment)
@hawkw
hawkw enabled auto-merge (rebase) September 15, 2025 19:19
@hawkw
hawkw merged commit 3acefa6 into master Sep 15, 2025
135 checks passed
@hawkw
hawkw deleted the eliza/ereport-qol branch September 15, 2025 19:29
hawkw added a commit that referenced this pull request Sep 15, 2025
This commit changes `task-packrat-api`'s `deliver_ereport` IPC to return
an error if the ereport did not fit in the buffer, rather than being
infallible. In the future, this may allow some tasks to attempt
re-delivery periodically until there's space.
hawkw added a commit that referenced this pull request Sep 15, 2025
In practice, so far, most ereport payloads have been defined as structs
implementing `serde::Serialize` and encoded using the `minicbor-serde`
adapter crate. This requires a bit of boilerplate code, so it seems
worthwhile to provide a method that just does that. This commit adds one
in `task-packrat-api`. It's feature-flagged so we can avoid compiling
`minicbor`, `minicbor-serde`, and `serde` in tasks that don't submit
ereports using this.
hawkw added a commit that referenced this pull request Sep 15, 2025
clockdomain pushed a commit to clockdomain/hubris that referenced this pull request Sep 26, 2025
This commit changes `task-packrat-api`'s `deliver_ereport` IPC to return
an error if the ereport did not fit in the buffer, rather than being
infallible. In the future, this may allow some tasks to attempt
re-delivery periodically until there's space.
clockdomain pushed a commit to clockdomain/hubris that referenced this pull request Sep 26, 2025
In practice, so far, most ereport payloads have been defined as structs
implementing `serde::Serialize` and encoded using the `minicbor-serde`
adapter crate. This requires a bit of boilerplate code, so it seems
worthwhile to provide a method that just does that. This commit adds one
in `task-packrat-api`. It's feature-flagged so we can avoid compiling
`minicbor`, `minicbor-serde`, and `serde` in tasks that don't submit
ereports using this.
clockdomain pushed a commit to clockdomain/hubris that referenced this pull request Sep 26, 2025
In [this comment][1], @cbiffle reminded me that we can use
`ClaimOnceCell` to use a `static` to create byte buffers for encoding
ereports. This way, the buffer is never on the stack, reducing the risk
of stack overflows when recording an ereport.

This commit changes `drv_gimlet_seq_server` and `drv_cosmo_seq_server`
to use that technique for their ereports.

[1]: oxidecomputer#2214 (comment)
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