[Features][CustomSched][BPFLib] #29

abrehman94 · 2024-10-17T18:28:04Z

* Custom Scheduling
* Capturing cgroup stats on per invoke level
* BPFLibrary to create a pinned BPFMap of function characteristics
  * used to share data with scx scheduler

aFuerst

I'm not going to look at correctness, etc. on this, will trust you. Just going for code quality, issues like that.

src/Ilúvatar/Makefile

aFuerst · 2024-10-23T14:06:24Z

src/Ilúvatar/Makefile

@@ -55,4 +58,4 @@ format:
 format-check:
 	@cargo fmt --all -- --check
 clippy:
-	@RUSTFLAGS=$(RUST_FLAGS) $(RUST_C) clippy --workspace --examples --benches --no-deps -- -Dclippy::suspicious -Dclippy::correctness -Dclippy::perf -Aclippy::single_match -Aclippy::new_without_default -Aclippy::too_many_arguments -Aclippy::type-complexity -Dclippy::from_over_into -Aclippy::redundant-field-names -Dwarnings
+	@RUSTFLAGS=$(RUST_FLAGS) $(RUST_C) clippy --fix --allow-no-vcs --workspace --examples --benches --no-deps -- -Dclippy::suspicious -Dclippy::correctness -Dclippy::perf -Aclippy::single_match -Aclippy::new_without_default -Aclippy::too_many_arguments -Aclippy::type-complexity -Dclippy::from_over_into -Aclippy::redundant-field-names -Dwarnings


This command is used to check if there are clippy violations, especially in CI, not to correct them. Adding --fix will have the issues "fixed" in the CI build, but those changes aren't actually committed to this branch. The issues would persist and silently creep in code quality problems. Undo this

aFuerst · 2024-10-23T14:09:02Z

src/Ilúvatar/Makefile

@@ -37,6 +37,9 @@ tiny:
 debug:
 	@echo "Building debug"
 	@RUSTFLAGS=$(RUST_FLAGS) $(RUST_C) build $(DEBUG_FLAGS) $(CARGO_ARGS)
+fix:
+	@echo "Fixing lint errors"
+	@RUSTFLAGS=$(RUST_FLAGS) $(RUST_C) fix $(DEBUG_FLAGS) $(CARGO_ARGS)


Combine this with the clippy fix command you changed below? Would be nice to have in the makefile to simplify fixing them locally

aFuerst · 2024-10-23T14:10:57Z

src/Ilúvatar/Cross.toml

@@ -5,5 +5,9 @@ passthrough = ["ARCH=amd64", "GO_VERSION=1.22.0", "CNI_VERSION=v1.1.1", "GOPATH=
 default-target = "x86_64-unknown-linux-gnu"    # use this target if none is explicitly provided
 pre-build = [                                  # additional commands to run prior to building the package
    "dpkg --add-architecture $CROSS_DEB_ARCH", 
-    "apt-get update && apt-get --assume-yes install protobuf-compiler iproute2 wget curl runc bridge-utils iptables net-tools sysstat"
+    "apt-get update && apt-get --assume-yes install protobuf-compiler iproute2 wget curl runc bridge-utils iptables net-tools sysstat libelf-dev lsb-release wget software-properties-common gnupg",
+    "wget https://apt.llvm.org/llvm.sh && chmod +x llvm.sh &&  ./llvm.sh  all &&  ln -s /usr/bin/ld.lld-18 /usr/bin/ld.lld &&  ln -s /usr/bin/clang-18 /usr/bin/clang &&  ln -s /usr/bin/clang++-18 /usr/bin/clang++ && rm llvm.sh"


Can you add these to the setup docs as new dependencies? I assume they're required for the bpf stuff

These are required at the build time. From what I understand, cross builds in a container environment so these dependencies need to be installed there. It does not need to be on the host. Setup docs are for the host right?

aFuerst · 2024-10-23T14:13:08Z

src/Ilúvatar/fs_policy_tsksz/src/bpf.rs

+        let rb_map = &mut skel.maps.queued_stats;
+        let mut builder = libbpf_rs::RingBufferBuilder::new();
+        builder.add(rb_map, callback_stats).unwrap();
+        let queued_stats = builder.build().unwrap();


We don't want upwrap and panic! because they can cause hard-to-debug issues and the error is only sent to stderr and easily lost

fs_policy_tsksz is launched as a separate process and the stderr is being logged to a separate log file. In case of a panic log file can be interpreted offline.

aFuerst · 2024-10-23T14:15:02Z

src/Ilúvatar/fs_policy_tsksz/src/main.rs

+                }
+            }
+            for (k, v) in &self.characteristics {
+                println!("{}: {:?}", k, v);


Idk how this will run, but can it use the same logging framework we have? For consistency

It's running as a separate process. I am logging it's stdout and stderr to separate files. We can use same tracing framework but it would just change the logging format in those files.

Not sure if you meant logging to same file or just logging using this framework for consistent format.

aFuerst · 2024-10-23T14:17:33Z

src/Ilúvatar/iluvatar_library/src/cgroup_interaction.rs

Is there am existing crate to make cgroup interaction easier on us and avoid duplication? I trust it's doing what you want, don't know what's out there.

Closest I found was this https://crates.io/crates/cgroups-fs. It does not support cgroup v2.

aFuerst · 2024-10-23T14:17:49Z

src/Ilúvatar/iluvatar_library/src/cgroup_interaction.rs

+                procs: vec![],
+                cpustats: [
+                    ("nr_periods".to_string(), 0),
+                    ("nr_throttled".to_string(), 0),


These strings should be constants

aFuerst · 2024-10-23T14:19:03Z

src/Ilúvatar/iluvatar_library/src/cgroup_interaction.rs

+        let cgroup_id = "89e979a2b0e9fd3".to_string();
+        let res = read_cgroup(cgroup_id);
+        println!("{:?}", res);
+        assert!(false);


Get these tests working, and remove print statements in them please.

aFuerst · 2024-10-23T14:22:44Z

src/Ilúvatar/iluvatar_library/src/utils/mod.rs

+        .arg("-c")
+        .arg(cmd)
+        .output()
+        .expect("failed to execute process");


There are wrappers for executing commands, iluvatar_library::utils::execute_cmd and others

It was experimental code. I have removed it.

aFuerst · 2024-10-25T13:35:55Z

src/Ilúvatar/iluvatar_worker_library/src/services/containers/container_pool.rs

@@ -111,6 +121,8 @@ impl ContainerPool {
                Some(c) => {
                    debug!(tid=%tid, container_id=%c.container_id(), name=%self.pool_name, pool_type=?PoolType::Idle, "Removing random container from pool");
                    self.add_container(c.clone(), &self.running_pool, tid, PoolType::Running);
+                    // pool is the best place to maintain a tid to cgroup id mapping


Is this the best place? Presumably the cgroup on a container doesn't change and could just be stored in the container struct. This would just make maintaining the pool and cgroups more convoluted

There was no mapping for tid to container. It creates this map in the container pool.

I don't think during a transaction tid a cgroup-id can change. Correct me if I am wrong.

Cgroup-id is part of the container.

aFuerst · 2024-10-25T13:37:47Z

src/Ilúvatar/iluvatar_worker_library/src/services/invocation/cpu_q_invoke.rs

@@ -333,6 +333,8 @@ impl CpuQueueingInvoker {
            EventualItem::Now(n) => n?,
        };
        self.running.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+        // since the acquire has been called - tid-cgroup id should be there
+        self.cmap.start_invoke(&reg.fqdn, tid);


Put these in invoke_on_container_2, one combined place that already has the cmap in it, much less code threading you have to do

That is a very nice suggestion! moved it to invoke_on_container_2.

aFuerst · 2024-10-25T13:40:01Z

src/Ilúvatar/iluvatar_worker_library/src/worker_api/mod.rs

+    pub tx_pids: IpcSender<PidsPacket>,
+}
+
+fn launch_scheduler(worker_config: &WorkerConfig) {


All this should be in a separate file. It is much too specific to be in the startup code file

Moved to a separate file.

aFuerst · 2024-11-07T13:44:19Z

How much did you change with that push? You re-wrote the history so I can't really tell. And please don't do that again...

Is there more that needs done to this?

abrehman94 · 2024-11-07T14:27:39Z

I was trying to keep the history clean. Specific changes made by the force push can be viewed using compare button.

Last commit does not fix everything. I have removed unnecessary code and fixed warnings only.

Right now I am on vacation, I will get to it in a week.

abrehman94 · 2024-11-25T16:42:48Z

Change BPFlib to a feature.

* Custom Scheduling * Capturing cgroup stats on per invoke level * BPFLibrary to create a pinned BPFMap of function characteristics * used to share data with scx scheduler

abrehman94 · 2024-11-27T21:49:48Z

@aFuerst Please have a look. I think I have addressed all the review comments. Sorry about force pushing the branch. It's just too much code, if I would have done separate commits, it would pollute the git history.

abrehman94 requested review from prateek-s and aFuerst October 17, 2024 18:28

abrehman94 force-pushed the finescheduling branch 3 times, most recently from e215d44 to 939b86b Compare October 17, 2024 21:41

aFuerst requested changes Oct 23, 2024

View reviewed changes

aFuerst reviewed Oct 25, 2024

View reviewed changes

abrehman94 force-pushed the finescheduling branch from 939b86b to 03aa34f Compare October 25, 2024 16:54

abrehman94 force-pushed the finescheduling branch 5 times, most recently from a60e641 to 2cdae55 Compare November 21, 2024 04:30

abrehman94 force-pushed the finescheduling branch from 2cdae55 to de5cc34 Compare November 27, 2024 20:10

[Features][CustomSched][BPFLib]

e1170df

* Custom Scheduling * Capturing cgroup stats on per invoke level * BPFLibrary to create a pinned BPFMap of function characteristics * used to share data with scx scheduler

abrehman94 force-pushed the finescheduling branch from de5cc34 to e1170df Compare November 27, 2024 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Features][CustomSched][BPFLib] #29

[Features][CustomSched][BPFLib] #29

abrehman94 commented Oct 17, 2024

aFuerst left a comment

aFuerst Oct 23, 2024

abrehman94 Nov 17, 2024

aFuerst Oct 23, 2024

abrehman94 Nov 17, 2024

aFuerst Oct 23, 2024

abrehman94 Nov 17, 2024

aFuerst Oct 23, 2024

abrehman94 Nov 17, 2024

aFuerst Oct 23, 2024

abrehman94 Nov 17, 2024

aFuerst Oct 23, 2024

abrehman94 Nov 18, 2024

aFuerst Oct 23, 2024

abrehman94 Nov 18, 2024

aFuerst Oct 23, 2024

abrehman94 Nov 18, 2024

aFuerst Oct 23, 2024

abrehman94 Nov 18, 2024

aFuerst Oct 25, 2024

abrehman94 Nov 18, 2024

aFuerst Oct 25, 2024

abrehman94 Nov 18, 2024

aFuerst Oct 25, 2024

abrehman94 Nov 18, 2024

aFuerst commented Nov 7, 2024

abrehman94 commented Nov 7, 2024

abrehman94 commented Nov 25, 2024

abrehman94 commented Nov 27, 2024

[Features][CustomSched][BPFLib] #29

Are you sure you want to change the base?

[Features][CustomSched][BPFLib] #29

Conversation

abrehman94 commented Oct 17, 2024

aFuerst 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

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

aFuerst commented Nov 7, 2024

abrehman94 commented Nov 7, 2024

abrehman94 commented Nov 25, 2024

abrehman94 commented Nov 27, 2024