feat(virtq): add high-level packed virtqueue producer/consumer api#1514
feat(virtq): add high-level packed virtqueue producer/consumer api#1514andreiltd wants to merge 3 commits into
Conversation
650cf31 to
c6ddbad
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a higher-level packed virtqueue API in hyperlight_common::virtq that sits on top of the existing packed-ring primitives, adding buffer management, message framing helpers, concurrency validation (loom), and performance benchmarks.
Changes:
- Add high-level producer/consumer APIs (
VirtqProducer/VirtqConsumer) that manage descriptor-chain lifecycle and notifications. - Add buffer allocation abstractions (
BufferProvider) plus concrete allocators (BufferPool,RecyclePool) and zero-copy segment types. - Add a small wire header (
VirtqMsgHeader), loom-based concurrency tests, and Criterion benchmarks for the new APIs/pools.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hyperlight_common/src/virtq/ring.rs | Exposes mutable access to readable elements to support high-level send-chain writing. |
| src/hyperlight_common/src/virtq/producer.rs | Implements VirtqProducer, chain builder/write APIs, submission/batching, and used-chain reclamation. |
| src/hyperlight_common/src/virtq/consumer.rs | Implements VirtqConsumer, receive+reply types, payload copying, and completion/notification logic. |
| src/hyperlight_common/src/virtq/buffer.rs | Defines BufferProvider, allocator errors/types, Segments, and zero-copy Bytes ownership glue. |
| src/hyperlight_common/src/virtq/pool.rs | Adds BufferPool and RecyclePool implementations for virtqueue buffer allocation. |
| src/hyperlight_common/src/virtq/msg.rs | Adds an 8-byte message header for kind/flags/correlation/payload sizing. |
| src/hyperlight_common/src/virtq/mod.rs | Re-exports new APIs, adds docs/quick-start examples, and defines shared error/notification types. |
| src/hyperlight_common/src/virtq/concurrency.rs | Adds loom-based interleaving tests with a shadow-atomic memory model. |
| src/hyperlight_common/Cargo.toml | Adds dependencies/dev-deps for the new virtq APIs, pools, loom tests, and benches. |
| src/hyperlight_common/benches/buffer_pool.rs | Criterion benchmarks for allocation/free patterns in the pools. |
| src/hyperlight_common/benches/virtq_api.rs | Criterion benchmarks for high-level virtq roundtrips under different allocator strategies. |
| src/hyperlight_common/benches/common/mod.rs | Shared benchmark harness (in-memory MemOps, pair construction, roundtrip drivers). |
| Justfile | Adds a test-loom target to run loom concurrency tests. |
| Cargo.toml | Adds unexpected_cfgs lint config for cfg(loom). |
| Cargo.lock | Locks new crate dependencies introduced by the virtq/pool/bench work. |
| .github/workflows/dep_code_checks.yml | Runs loom concurrency tests in CI. |
11a9c24 to
5b74480
Compare
1b64bd7 to
7fbcf3d
Compare
This patch adds a higher level api on top of the packed ring primitives in hyperlight_common so callers don't manage raw descriptors directly. - Add VirtqProducer/VirtqConsumer in producer.rs and consumer.rs that handle buffer allocation, chain lifecycle, and event suppression/notification decisions; expand virtq/mod.rs with the public API, builders, and docs. - Add buffer management: the BufferProvider trait and shared types in buffer.rs plus two concrete allocators - a two-tier variable-size BufferPool and a fixed-slot RecyclePool. - Add an 8-byte wire message header in msg.rs for message type discrimination and request/response correlation. - Add loom-based concurrency tests in concurrency.rs using shadow atomics - Add criterion benchmarks for the buffer pool and virtq API. Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>
7fbcf3d to
8ae93c2
Compare
Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>
511a737 to
33806cc
Compare
simongdavies
left a comment
There was a problem hiding this comment.
A couple of minor issues, but otherwise LGTM
| /// to move a pool (or a reply `Bytes`) to another thread, e.g. by using these | ||
| /// pools with a producer/consumer on the multi-threaded host. | ||
| #[derive(Debug)] | ||
| pub(super) struct SyncWrap<T>(pub(super) T); |
There was a problem hiding this comment.
If this is only intended to be used for the BufferPool and RecyclePool is there any reason to have the unconstrainted T?
Could we either constraint this to these types:
unsafe impl<const L: usize, const U: usize> Send for SyncWrap<Rc<RefCell<Inner<L, U>>>> {}
unsafe impl<const L: usize, const U: usize> Sync for SyncWrap<Rc<RefCell<Inner<L, U>>>> {}
unsafe impl Send for SyncWrap<Rc<RefCell<RecycleList>>> {}
unsafe impl Sync for SyncWrap<Rc<RefCell<RecycleList>>> {}Or remove it entirely?
pub struct BufferPool<const L: usize = 256, const U: usize = 4096> {
inner: Rc<RefCell<Inner<L, U>>>,
}
pub struct RecyclePool {
inner: Rc<RefCell<RecycleList>>,
}
// SAFETY: these pools hold their state in a non-atomic `Rc<RefCell<..>>`. The
// Send/Sync assertions are sound only for single-threaded (guest-side) use,
// where the pool and every reply `Bytes` derived from it stay on one thread.
// Hyperlight guests are single-threaded. Moving a pool (or a reply `Bytes`) to
// another thread — e.g. a producer/consumer on the multi-threaded host — is unsound.
unsafe impl<const L: usize, const U: usize> Send for BufferPool<L, U> {}
unsafe impl<const L: usize, const U: usize> Sync for BufferPool<L, U> {}
unsafe impl Send for RecyclePool {}
unsafe impl Sync for RecyclePool {}| } | ||
|
|
||
| /// Read the descriptor body (`addr`/`len`/`id`) and combine it with flags | ||
| /// already obtained from [`load_flags_acquire`](Self::load_flags_acquire). |
There was a problem hiding this comment.
this link is broken should be read_flags_acquire
| pub fn complete(&mut self, reply: impl Into<ReplyChain<M>>) -> Result<(), VirtqError> { | ||
| let reply = reply.into(); | ||
| let id = reply.token().id; | ||
| let written = reply.written() as u32; |
There was a problem hiding this comment.
should this use u32::try_from(reply.written()).map_err(|_| VirtqError::ReplyTooLarge)?
|
|
||
| #[inline] | ||
| fn align_up(val: usize, align: usize) -> usize { | ||
| assert!(align > 0); |
There was a problem hiding this comment.
Could we make this return a result?
fn align_up(val: usize, align: usize) -> Result<usize, AllocError> {
if align == 0 { return Err(AllocError::InvalidArg); }
Ok(val.next_multiple_of(align))
}or make the assert a const?
Also there is an align_up in mod.rs (which also panics on align == 0)
There was a problem hiding this comment.
or use https://doc.rust-lang.org/std/primitive.u32.html#method.next_multiple_of. Result might be a bit restrictive
This patch adds a higher level api on top of the packed ring primitives in hyperlight_common so callers don't manage raw descriptors directly.