-
Notifications
You must be signed in to change notification settings - Fork 447
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
Expand ring_buffer API #34
Conversation
src/storage/ring_buffer.rs
Outdated
/// | ||
/// During creation, every element in `storage` is set to Default::default(). | ||
pub fn with_default<S>(storage: S) -> RingBuffer<'a, T> | ||
where S: Into<ManagedSlice<'a, T>>, T: Default, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm uncomfortable about this ambiguity, having two separate paths for resetting and creating a default member. How about making Resettable
imply Default
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can do it, otherwise, I would have gone with Default
when I refactored RingBuffer
out. Currently, we're using RingBuffer
to store PacketBuffer
for UDP and Raw sockets, and these contain Managed<'a, [u8]>
, meaning we can't just Default::default()
them, so we use Resettable
to reset the other fields. Yet for 90% of the potential use cases Default::default()
would be enough and there's no need to require Resettable
.
Now I think of it, maybe we should just get rid of Resettable
and initialization of elements in the constructors altogether? I can't remember why I even need them, the container itself doesn't care about the values, and we already initialize them in the constructors before inserting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I think of it, maybe we should just get rid of Resettable and initialization of elements in the constructors altogether?
I think you'll get infoleaks when reusing sockets then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only reset elements in the RingBuffer
's constructor tho, try_enqueue
explicitly takes a function to reset the new element, and enqueue
doesn't do it at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. Suppose application A put key material all over the socket. Then, the socket, without being reset, is passed to application B. This application can now call .enqueue()
and read from the buffer what it should have never seen. Ideally we'd have something like &write [u8]
but alas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting point I didn't think about, but I think we should just make RingBuffer
's methods pub(crate) since a user doesn't need to use them directly. It's impossible to get access to the old data via sockets' methods because they all limited by the length of the data in the buffer. Besides, I think memsetting buffers all the time would be too costly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course it's possible, just use tcp_socket.send()
, which directly calls enqueue()
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TcpSocket
doesn't use RingBuffer
right now, but you're right, you can get access to the old data via UdpSocket::send()
. So the current problem is twofold - we don't call reset
on element reuse and we don't memset the buffer in reset
. I guess I'll do it then. This means the Resettable
bound is now container-wide and there's no point in with_default
constructor.
Now I need to find a way to memset a buffer in such a way that the compiler won't optimize it out :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only legal for memset to be removed right before deallocation, and we don't have that, so it's not a concern.
src/storage/ring_buffer.rs
Outdated
@@ -101,6 +119,67 @@ impl<'a, T: 'a> RingBuffer<'a, T> { | |||
Err(error) => Err(error) | |||
} | |||
} | |||
|
|||
/// Get capacity of the underlying storage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird to have capacity()
but not len()
.
Style nit: these should go up after full()
and empty()
, where all the query methods live.
src/storage/ring_buffer.rs
Outdated
|
||
/// Remove the first element equal to `value` from the buffer, | ||
/// reutrn `Err(Error::Exhausted)` if no such element was found. | ||
pub fn remove(&mut self, value: &T) -> Result<()> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to go completely against the spirit of a ring buffer. Why do you need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using RingBuffer
as a queue of dirty sockets and I needed to delete sockets from the queue once they're removed from the main container. I agree its usefulness isn't evident, but it could be handy to have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. But as I've explained in this comment a queue won't work here, you'll need a BTreeMap as well, see my latest commit for more context as to why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok then.
src/storage/ring_buffer.rs
Outdated
} | ||
|
||
/// Try to expand the underlying storage, panic if the storage cannot be expanded. | ||
#[cfg(not(any(feature = "std", feature = "collections")))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer having a single method and optionally compiling in including the ManagedSlice::Owned
branch.
src/storage/ring_buffer.rs
Outdated
|
||
#[cfg(any(feature = "std", feature = "collections"))] | ||
pub fn expand_storage(&mut self) | ||
where T: Copy + Default { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need Copy
? I think mem::swap
or even slice::swap
is enough to implement the expansion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's twice as much work so the Copy bound is fine.
18bd3d6
to
f9f3ed5
Compare
|
Okay, so I think we have some confusion with this PR.
I'm sorry for making you do all this work, I'll try to be more clear from the outset in the future... |
f9f3ed5
to
e346dd9
Compare
No, it's ok, I should have thought it through before making the PR. I think I understand the resetting issue, but it took me some time to realize "socket buffer" doesn't mean Anyway, I see we don't need resetting in |
e346dd9
to
54aa56a
Compare
54aa56a
to
2605e00
Compare
2605e00
to
3da43fe
Compare
Let's address buffer resetting in a coordinated way across the entire codebase when we'll make it possible to return buffers to the pool. The changes here are trivial and insufficient on their own. Regarding expandable ring buffers, I think that was needed for egress handling, but egress is now going to use BTreeMap too, to sort sockets by the earliest time they should be pollwed, so it seems like it's not necessary for now. If it ever is, it's always possible to cherry-pick the commit from here. Thus, closing. |
Problem: the
ring_buffer
container lacks several useful methods.Solution: add the missing methods.
Changes introduced by this pull request:
with_default
,capacity
,remove
andexpand_storage
methods.Other:
This PR is part of #19 effort.