Expand ring_buffer API #34

batonius · 2017-08-28T22:29:19Z

Problem: the ring_buffer container lacks several useful methods.

Solution: add the missing methods.

Changes introduced by this pull request:

the with_default, capacity, remove and expand_storage methods.
tests for the new methods.

Other:
This PR is part of #19 effort.

whitequark · 2017-08-29T04:17:52Z

src/storage/ring_buffer.rs

+    ///
+    /// 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,


I'm uncomfortable about this ambiguity, having two separate paths for resetting and creating a default member. How about making Resettable imply Default instead?

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.

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.

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.

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.

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.

Of course it's possible, just use tcp_socket.send(), which directly calls enqueue()...

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 :)

It's only legal for memset to be removed right before deallocation, and we don't have that, so it's not a concern.

whitequark · 2017-08-29T04:19:29Z

src/storage/ring_buffer.rs

@@ -101,6 +119,67 @@ impl<'a, T: 'a> RingBuffer<'a, T> {
            Err(error) => Err(error)
        }
    }
+
+    /// Get capacity of the underlying storage.


Weird to have capacity() but not len().

Style nit: these should go up after full() and empty(), where all the query methods live.

whitequark · 2017-08-29T04:20:01Z

src/storage/ring_buffer.rs

+
+    /// 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<()>


This seems to go completely against the spirit of a ring buffer. Why do you need this?

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.

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.

whitequark · 2017-08-29T04:21:06Z

src/storage/ring_buffer.rs

+    }
+
+    /// Try to expand the underlying storage, panic if the storage cannot be expanded.
+    #[cfg(not(any(feature = "std", feature = "collections")))]


I'd prefer having a single method and optionally compiling in including the ManagedSlice::Owned branch.

whitequark · 2017-08-29T04:23:49Z

src/storage/ring_buffer.rs

+
+    #[cfg(any(feature = "std", feature = "collections"))]
+    pub fn expand_storage(&mut self)
+        where T: Copy + Default {


Do you need Copy? I think mem::swap or even slice::swap is enough to implement the expansion.

I guess that's twice as much work so the Copy bound is fine.

batonius · 2017-08-29T22:22:09Z

Removed remove method and with_default constructor.
Added len method.
RingBuffer now calls Resetable::reset() when enqueuing an element.
UdpPacketBuffer and RawPacketBuffer now memset their payload in Resetable::reset().

whitequark · 2017-08-30T06:08:24Z

Okay, so I think we have some confusion with this PR.

The new expand_storage method is fine but it seems more useful to call it reserve_exact to match the standard containers and let expanding by more than a single element. I'm not sure if it's ever useful to do that, a reasonable strategy is something like adding 50% more buffers once you're 75% full or something.
Let's not add any unsafe code, smoltcp has 0 grep hits for unsafe and I think this is an important advantage.
I think I miscommunicated what I talked about wrt resetting to you. It already works properly. It's not actually necessary to reset elements on every enqueue() call because every such call after the socket is open comes from the same application. The idea is that a TCP/IP host would have a pool of packet (and/or socket) buffers; when a new application on that host asks it for a socket, it creates a new UdpSocket (cheap) and assigns it a socket buffer from a pool (cheap) or if none are available, creates a new one (expensive); in either case the UdpSocket constructor does the reset. After the application is done, the UDP socket is destroyed; if you look at the API, there isn't even a way to close an UDP socket once it's open.

I'm sorry for making you do all this work, I'll try to be more clear from the outset in the future...

batonius · 2017-08-30T21:46:40Z

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 SocketBuffer :) The UDP constructor doesn't actually reset anything, but it consumes instances of SocketBuffer which need to be created for each new socket from ManagedSlices (from a pool?), and this is where the resetting happens. I can't see how these buffers can be returned to the pool right now tho: since ManagedSlice owns a vector, SocketBuffer owns a slice and UdpSocket owns buffers, dropping a socket would free the vectors.

Anyway, I see we don't need resetting in enqueue, but I reckon we still need to actually erase old payload during the resetting.

whitequark · 2017-09-08T17:25:16Z

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.

whitequark requested changes Aug 29, 2017

View reviewed changes

batonius force-pushed the expandable_ring_buffer branch from 18bd3d6 to f9f3ed5 Compare August 29, 2017 22:16

batonius force-pushed the expandable_ring_buffer branch from f9f3ed5 to e346dd9 Compare August 30, 2017 21:43

batonius force-pushed the expandable_ring_buffer branch from e346dd9 to 54aa56a Compare August 30, 2017 21:54

whitequark force-pushed the master branch from 3226770 to 3ab58b0 Compare August 30, 2017 22:26

batonius force-pushed the expandable_ring_buffer branch from 54aa56a to 2605e00 Compare August 30, 2017 22:45

batonius added 2 commits August 31, 2017 10:23

Make RingBuffer expandable.

cf22ba2

Memset payload buffers in UDP/Raw PacketBuffer::reset.

3da43fe

batonius force-pushed the expandable_ring_buffer branch from 2605e00 to 3da43fe Compare August 31, 2017 07:23

batonius mentioned this pull request Sep 8, 2017

[RFC] Packet dispatch #19

Open

whitequark closed this Sep 8, 2017

batonius deleted the expandable_ring_buffer branch September 8, 2017 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand ring_buffer API #34

Expand ring_buffer API #34

batonius commented Aug 28, 2017

whitequark Aug 29, 2017

batonius Aug 29, 2017

whitequark Aug 29, 2017

batonius Aug 29, 2017

whitequark Aug 29, 2017

batonius Aug 29, 2017

whitequark Aug 29, 2017

batonius Aug 29, 2017

whitequark Aug 29, 2017

whitequark Aug 29, 2017

whitequark Aug 29, 2017

batonius Aug 29, 2017

whitequark Aug 29, 2017

batonius Aug 29, 2017

whitequark Aug 29, 2017

whitequark Aug 29, 2017

whitequark Aug 29, 2017

batonius commented Aug 29, 2017

whitequark commented Aug 30, 2017 •

edited

batonius commented Aug 30, 2017

whitequark commented Sep 8, 2017

Expand ring_buffer API #34

Expand ring_buffer API #34

Conversation

batonius commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

batonius commented Aug 29, 2017

whitequark commented Aug 30, 2017 • edited

batonius commented Aug 30, 2017

whitequark commented Sep 8, 2017

whitequark commented Aug 30, 2017 •

edited