portus api: deduplicate programs across flows #38

deeptir18 · 2018-09-06T20:31:00Z

This commit adds an init_programs function to the CCP API where users
must provide all datapath programs at the beginning, and whenever a flow
should use a particular program, they can use the new "set_program"
function.

I'll split up the commits later, sorry.

akshayknarayan

Overall looks good.
I have a couple code-style comments, and it seems like you haven't cleaned up your commented-out testing code yet.

akshayknarayan · 2018-09-06T20:51:15Z

ccp_generic_cong_avoid/src/lib.rs

@@ -347,6 +252,127 @@ impl<T: Ipc, A: GenericCongAvoidAlg> CongAlg<T> for GenericCongAvoid<T, A> {
        A::name()
    }

+    fn init_programs() -> Option<Vec<(Bin, Scope, String)>> {
+        // install all the datapath programs that can be used in the lifetime of this program
+        let mut vec = vec![];


let mut vec = vec![ (String::from("(def.....)"), String::from("DatapathIntervalProg")), (String::from("(def.....)"), String::from("DatapathIntervalRTTProg")), ...., ];

This is cleaner than vec![] followed by multiple vec.push() calls.
Also, can we call the variable something other than vec? Maybe programs?

akshayknarayan · 2018-09-06T20:55:07Z

ccp_generic_cong_avoid/src/lib.rs

+            let (bin, sc) = portus::compile_program(prog.as_bytes(), None).unwrap();
+            ret_vec.push((bin, sc, prog_name.clone()));
+        }
+        Some(ret_vec)


Some( vec.iter() .map(|prog, prog_name| { let (bin, sc) = portus::compile_program(prog.as_bytes(), None).unwrap(); (bin, sc, prog_name.clone()) }) .collect::<Vec<(_,_,_)>>()

Instead of the for loop and the new vec

akshayknarayan · 2018-09-06T20:56:22Z

integration_tests/libccp_integration/Makefile

@@ -1,7 +1,7 @@
 CC = gcc # C compiler
 CFLAGS = -fPIC -Wall -Wextra -O2 -g # C flags
 CFLAGS += -D__USRLIB__ # Only use this makefile for compiling user-space library
-ifeq ($(DEBUG), 1)
+ifeq ($(DEBUG), y)


different commit for this?

akshayknarayan · 2018-09-06T20:57:42Z

integration_tests/libccp_integration/test_datapath.c

    int err = 0;
    if ((send_sock = socket(AF_UNIX, SOCK_DGRAM, 0)) == -1) {
        printf("Could not setup sending socket\n");
        exit(-1);
    }

-    send_sockaddr.sun_family = AF_UNIX;
+    /*send_sockaddr.sun_family = AF_UNIX;


akshayknarayan · 2018-09-06T20:58:21Z

src/lib.rs

 use ipc::Ipc;
 use ipc::{BackendSender, BackendBuilder};
 use serialize::Msg;
 use std::sync::{Arc, atomic};
 use std::thread;

+/*pub struct ScopeMap {


What is ScopeMap?

akshayknarayan · 2018-09-06T21:00:28Z

src/lib.rs

+                self.sender.send_msg(&buf[..])?;
+                Ok(sc.clone())
+            },
+            _ => Err(Error(


indentation here

wait what should this look like

oh I was just thinking:

_ => Err(Error( format!("Map....", program_name), )),

akshayknarayan · 2018-09-06T21:01:10Z

src/serialize/changeprog.rs

+pub struct Msg {
+    pub sid: u32,
+    pub program_uid: u32,
+    pub num_fields: u32, // question: does this get turned into a u32?


during serialization

akshayknarayan · 2018-09-06T21:13:00Z

Also @deeptir18 since this is a breaking API change, please bump the portus version in Cargo.toml to 0.3.0

fcangialosi

no comments aside from what Akshay already mentioned

akshayknarayan

Version number should be 0.3.0 :)

akshayknarayan · 2018-09-07T02:34:02Z

Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "portus"
-version = "0.2.3"
-authors = ["Akshay Narayan <akshayn@csail.mit.edu>", "Frank Cangialosi <frankc@csail.mit.edu", "Deepti Raghavan <deeptir@stanford.edu"]
+version = "0.2.4"


0.3.0: because it's a breaking change, we want to make sure downstream crates which write portus = "0.2" as their SemVer dependency don't pull in this change and break.

akshayknarayan · 2018-09-07T02:34:51Z

integration_tests/libccp_integration/test_datapath.c

-    struct sockaddr_un send_sockaddr;
-    int path_len = 0;
+    //struct sockaddr_un send_sockaddr;
+    //int path_len = 0;


why is this commented?

akshayknarayan · 2018-09-07T02:35:41Z

integration_tests/libccp_integration/test_datapath.c

@@ -277,11 +274,13 @@ int main(__attribute__((unused))int argc, __attribute__((unused))char **argv) {
    int recv_sock = setup_listening_thread();

    // initialize a fake connection; sends create messages
+	// sleep to make sure portus has started
+	sleep(2);


I don't think this is necessary, but let's not worry about it now, since my PR #37 will fix it anyway.

akshayknarayan · 2018-09-07T02:36:50Z

src/lib.rs

+                self.sender.send_msg(&buf[..])?;
+                Ok(sc.clone())
+            },
+            _ => Err(Error(


oh I was just thinking:

_ => Err(Error( format!("Map....", program_name), )),

akshayknarayan · 2018-09-07T02:38:04Z

ccp_generic_cong_avoid/src/lib.rs

+
+            "), String::from("SSUpdateProg"))];
+        Some(
+        programs.iter()


indentation here

This commit modifies the CCP API to expect users to install all programs in the new init_programs function, when portus starts running. To make a flow use a particular program, there is a new set_program function; when installing, users also specify a name for each program. In the datapath, now, programs need not be duplicated across flows. This also bumps the libccp submodule version to match the corresponding changes in libccp.

deeptir18 requested review from akshayknarayan and fcangialosi September 6, 2018 20:31

akshayknarayan reviewed Sep 6, 2018

View reviewed changes

fcangialosi approved these changes Sep 6, 2018

View reviewed changes

deeptir18 force-pushed the deduplicate branch 2 times, most recently from 965d228 to 4735ee8 Compare September 7, 2018 01:20

akshayknarayan reviewed Sep 7, 2018

View reviewed changes

deeptir18 force-pushed the deduplicate branch from 4735ee8 to 7260eff Compare September 7, 2018 04:47

fcangialosi merged commit 073ae6b into master Sep 7, 2018

fcangialosi deleted the deduplicate branch September 7, 2018 04:54

This was referenced Sep 8, 2018

Datapath program de-duplication #32

Closed

Python bindings don't compile #40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

portus api: deduplicate programs across flows #38

portus api: deduplicate programs across flows #38

deeptir18 commented Sep 6, 2018

akshayknarayan left a comment

akshayknarayan Sep 6, 2018

akshayknarayan Sep 6, 2018

akshayknarayan Sep 6, 2018

akshayknarayan Sep 6, 2018

akshayknarayan Sep 6, 2018

akshayknarayan Sep 6, 2018

akshayknarayan Sep 6, 2018

akshayknarayan Sep 6, 2018

deeptir18 Sep 7, 2018

akshayknarayan Sep 7, 2018

akshayknarayan Sep 6, 2018

akshayknarayan commented Sep 6, 2018

fcangialosi left a comment

akshayknarayan left a comment

akshayknarayan Sep 7, 2018

akshayknarayan Sep 7, 2018

akshayknarayan Sep 7, 2018

akshayknarayan Sep 7, 2018

akshayknarayan Sep 7, 2018

portus api: deduplicate programs across flows #38

portus api: deduplicate programs across flows #38

Conversation

deeptir18 commented Sep 6, 2018

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

akshayknarayan commented Sep 6, 2018

fcangialosi left a comment

Choose a reason for hiding this comment

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