feat: bidirectional pipe#34
Conversation
| } | ||
|
|
||
| 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
There was a problem hiding this comment.
Also see the draft PR for renaming these functions.
| /// 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...
There was a problem hiding this comment.
I think SharedMemoryRegion should be a trait that exposes functions necessary for the creation of the BidirectionalPipe. The trait should also be a Clone.
| 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?
There was a problem hiding this comment.
Here we should make the BidirectionalPipe owns the SharedMemoryRegion. So it would be like impl <T: SharedMemoryRegion> for BidrectionalPipe<T> and the pipe has a real T field.
| } | ||
|
|
||
| /// 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
yhdengh
force-pushed
the
arca-networking-pr
branch
from
7ff4376 to
95a96c7
Compare
Co-authored-by: ruiting-chen <ruiting.chen.soc@gmail.com>
yhdengh
force-pushed
the
arca-networking-pr
branch
from
95a96c7 to
45e8d46
Compare
| } | ||
|
|
||
| /// Split into independent read and write halves (like `TcpStream::split`). | ||
| pub fn split(&mut self) -> (&mut RingConsumer<'a>, &mut RingProducer<'a>) { |
There was a problem hiding this comment.
We should also have a owned version of this, like a split(Self) -> (RingConsumer, RingProducer). We may need to either change the RingConsumer and RingProducer to take a SharedMemoryRegion ownership, or a wrapper struct that wraps around the current RingConsumer and RingProducer.
|
@ruiting-chen @GregoryLi360 I didn't make changes to the |
1 participant
This PR adds a
no_stdbidirectional pipe implementation shared by vmm-side and Arca-side