WIP: multi-threaded ccp #78

fcangialosi · 2019-06-21T09:34:43Z

This is a WIP, not ready to be merged yet.

This PR contains a few commits to enable an easier integration with mvfst.

The only change to the interface is an additional typeparameter to run allowing for different backendbuilders to be passed in. The traditional backend and backendbuilder were renamed to singlebackend and singlebackendbuilder.

Once this new interface is in place, it's possible to implement multi-threaded ccp without changing the api or internal run_inner implementation by implementing a multibackend (and multibackendbuilder).

The Other variant of Msg was previously just a wrapper around RawMsg, which must have the same lifetime as receive buf, because it has a direct slice into it. All of the other message variants copy the contents of RawMsg into their own struct, allowing them to own the data, meaning they don't need lifetime specifiers. So the only reason we have a lifetime specifier is for the other type, which itself is only used in tests and benchmarks. We can get rid of this by creating a wrapper Other message type that basically mimics RawMsg, but copies the contents. This is actually more realistic for the benchmark anyway, because it now more closely mimics the behavior of the real message types we use (i.e. it makes its own copy of the message).

akshayknarayan

Might be good to run the IPC benchmark to see if the performance has changed (use of Arc vs Rc among other changes).
I haven't yet looked at split of the overall diff into individual commits, we can look at that once we're happy with the overall structure
Also pls run rustfmt, that is why travis is sad.

akshayknarayan · 2019-06-21T14:45:42Z

src/algs.rs

@@ -101,7 +101,10 @@ pub fn ipc_valid(v: String) -> std::result::Result<(), String> {
 /// }
 ///
 /// fn main() {
-///     portus::start!("unix", None, MyCongestionControlAlgorithm(Default::default()));
+///     let handle = portus::spawn!("unix", None, MyCongestionControlAlgorithm(Default::default()), 4u32);


The documentation for start! should include a use of start!, not spawn!

So before this doctest worked because install datapath programs would fail and so the function would exit. But after I added the code to wait if the datapath is not available, this doctest will hang forever. So I think our only options are (a) don't run the doctest, which is a little unfortunate because it's a nice test of the entire API with a sample congestion control algorithm or (b) use the spawn api and have it start ccp and then kill it with the handle. Do you have any other suggestions for being able to still test start too?

src/algs.rs

akshayknarayan · 2019-06-21T14:46:52Z

src/algs.rs

+    }};
+}
+#[macro_export]
+macro_rules! spawn {


documentation? what is this used for?

added initially because of comment above about not being able to test start because it hangs but wanting to still test everything else -- but it seems like it makes sense to have a spawn equivalent of start! regardless since the interface is pretty much the same

akshayknarayan · 2019-06-21T14:47:34Z

src/bin/ipc_latency.rs

@@ -14,9 +14,10 @@ use std::sync::{atomic, Arc};
 use time::Duration;

 #[derive(Debug)]
-struct TimeMsg(time::Timespec);
+pub struct TimeMsg(time::Timespec);


why does this need to be pub - it's in a binary

needed to be pub because of the separate deserialize_timemsg method

if it's not pub, compiler says TimeMsg decleared as private, can't leak private type

then, why does deserialize_timemsg need to be pub?

akshayknarayan · 2019-06-21T14:49:03Z

src/bin/ipc_latency.rs

+    let sec = LittleEndian::read_i64(&b[0..8]);
+    let nsec = LittleEndian::read_i32(&b[8..12]);
+    Ok(TimeMsg(time::Timespec::new(sec, nsec)))
+}


why has from_raw_msg been moved to this separate function and replaced with unimplemented. Also similarly why is this pub

see commit message

akshayknarayan · 2019-06-21T15:24:02Z

src/ipc/mod.rs

+/// Backend will yield incoming IPC messages forever via `next()`.
+/// It owns the socket; `BackendSender` holds weak references.
+/// The atomic bool is a way to stop iterating.
+pub struct SingleBackend<T: Ipc> {


Moving this up above MultiBackendwould clean up the diff

akshayknarayan · 2019-06-21T15:24:45Z

src/ipc/mod.rs

    fn drop(&mut self) {
-        Rc::get_mut(&mut self.sock)
+        Arc::get_mut(&mut self.sock)


why does this have to be an Arc?

because backend is moved inside the sending thread (around line 128) so it has to be send which means all of its members must be send

akshayknarayan · 2019-06-21T15:26:40Z

src/ipc/unix.rs

    };
 }

 pub struct Socket<T> {
    sk: UnixDatagram,
    dest: String,
    _phantom: PhantomData<T>,
+    _id: u8


please move above _phantom - PhantomData fields are supposed to be last

akshayknarayan · 2019-06-21T15:29:29Z

src/lib.rs

@@ -549,7 +587,7 @@ where
                let f = alg.new_flow(
                    Datapath {
                        sock_id: c.sid,
-                        sender: backend.clone(),
+                        sender: b.sender(), // backend.clone(),


pls remove old comments

akshayknarayan · 2019-06-21T15:30:47Z

src/serialize/other.rs

@@ -0,0 +1,41 @@
+//! Message sent from datapath to CCP when a new flow starts.


why is this called "other" if it's actually a "DatapathInitMsg" or whatever

whoops, looks like i accidentally copied that comment, it's not correct :)

other is just a wrapper around RawMsg that owns the bytes to remove the lifetime specifier

…ltibackend The idea here is to allow CCP to listen on multiple sockets, allowing datapaths (such as mTCP or mvfst) to internally maintain a separate connection with ccp per-thread, without changing the api or main implementation loop much. Aside from adding backend traits to the function signatures, the main run_inner loop does not change

…panic'ing

fcangialosi added 3 commits June 21, 2019 01:38

unix: parameterize by id instead of in/out

d0f4e84

backend: make receive buffer internal to backend

0836f54

fcangialosi requested a review from akshayknarayan June 21, 2019 09:34

akshayknarayan requested changes Jun 21, 2019

View reviewed changes

fcangialosi added 5 commits June 25, 2019 12:20

run: if install program fails, wait to hear from datapath instead of …

f4daa97

…panic'ing

rustfmt

38af7e4

install_programs: broadcast to all backends via new BackendBroadcaster

bac3320

integration_tests: add simple test for multibackend

071296b

fcangialosi force-pushed the multi_backend_clean branch from 417ebce to 071296b Compare June 25, 2019 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: multi-threaded ccp #78

WIP: multi-threaded ccp #78

fcangialosi commented Jun 21, 2019

akshayknarayan left a comment

akshayknarayan Jun 21, 2019

fcangialosi Jun 21, 2019

akshayknarayan Jun 21, 2019

fcangialosi Jun 21, 2019

akshayknarayan Jun 21, 2019

fcangialosi Jun 21, 2019

fcangialosi Jun 21, 2019

akshayknarayan Jun 21, 2019

akshayknarayan Jun 21, 2019

fcangialosi Jun 21, 2019

akshayknarayan Jun 21, 2019

fcangialosi Jun 21, 2019

akshayknarayan Jun 21, 2019

fcangialosi Jun 21, 2019

akshayknarayan Jun 21, 2019

akshayknarayan Jun 21, 2019

akshayknarayan Jun 21, 2019

fcangialosi Jun 21, 2019

		@@ -0,0 +1,41 @@
		//! Message sent from datapath to CCP when a new flow starts.

WIP: multi-threaded ccp #78

Are you sure you want to change the base?

WIP: multi-threaded ccp #78

Conversation

fcangialosi commented Jun 21, 2019

akshayknarayan left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment