feat: bidirectional pipe#34
Conversation
Co-authored-by: ruiting-chen <ruiting.chen.soc@gmail.com>
| } | ||
|
|
||
| impl RingHeader { | ||
| /// Bytes available to read. Called by the consumer. |
There was a problem hiding this comment.
The current choices of Ordering depend on that read is only ever modified by the consumer and used_space is only ever invoked by the consumer. This info should be somewhere in the comment
| /// Owns `(ptr, size)` together so call sites don't juggle them. | ||
| /// Wrap-around is handled inside `write_at` / `read_at`. | ||
| pub struct RingData { | ||
| ptr: *mut u8, |
There was a problem hiding this comment.
Why not &mut [u8]? Is there a reason for RingData to need a raw pointer?
|
|
||
| /// Bytes written by this producer that the consumer has not yet read. | ||
| /// Uses Acquire on read_cursor so this can be called cross-thread safely. | ||
| pub fn bytes_pending(&self) -> u64 { |
There was a problem hiding this comment.
How is this different from Ring::free_space()?
| } | ||
|
|
||
| /// True when both ends of this ring are closed. | ||
| pub fn is_closed(&self) -> bool { |
There was a problem hiding this comment.
Maybe consider moving some of these definitions to impl RingData?
| @@ -0,0 +1,66 @@ | |||
| /// Owns a reference to a shared memory region. | |||
There was a problem hiding this comment.
Hmmmm I don't quite understand "owning a reference to a shared memory region". Unless we do something when the reference count hits 0, this comment doesn't really make sense...
| len: u64, | ||
| } | ||
|
|
||
| impl SharedMemoryRegion { |
There was a problem hiding this comment.
...and this is also not quite about shared too...
| /// Caller must ensure the region is zero-initialized before the first side | ||
| /// is constructed, and that exactly one `Side::A` and one `Side::B` are | ||
| /// created per region. | ||
| pub fn new(region: &'a SharedMemoryRegion, ring_size: u64, side: Side) -> Self { |
There was a problem hiding this comment.
Given that the SharedMemoryRegion right now doesn't really "own" the memory, the reference here also don't quite make sense to me...if the concern is that it may have different implementation on the vmm side and the Arca side, we could keep it as a trait here, and let the BidirectionalPipe own it? Then on the vmm-side, this would be mmap::Mmap, on the Arca-side, it holds a raw pointer and size and does nothing on Drop?
| } | ||
|
|
||
| /// Signal that this consumer will read no more bytes. | ||
| pub fn close_reader(&self) { |
There was a problem hiding this comment.
Should this be &mut self since there could only be one reader?
| } | ||
|
|
||
| /// Close this side's outgoing (write) direction. | ||
| pub fn close_write(&self) { |
There was a problem hiding this comment.
Also these functions should also be &mut self? If the user want to be able to handle the consumer and producer side independently, they should split.
| } | ||
|
|
||
| impl<'a> traits::Write for RingProducer<'a> { | ||
| fn write(&mut self, buf: &[u8]) -> Result<usize, PipeError> { |
There was a problem hiding this comment.
I think the logic of "closing the read-end when read EOF" and "closing the write-end when the read-end closed prematurely" should happen at RingProducer and RingConsumer level, or BidirectionalPipe, instead of SyncStream
1 participant
This PR adds a
no_stdbidirectional pipe implementation shared by vmm-side and Arca-side