Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
DevinLeamy commented on code in PR #501:
URL: https://github.com/apache/mesos/pull/501#discussion_r1517800854
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,99 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
+ }
+ return result;
+}
+
+
+uint64_t pointer_to_int(const void* pointer)
+{
+ return (uint64_t)(unsigned long)pointer;
+}
+
+} // namespace internal {
+
+ProgramBuilder::ProgramBuilder(enum bpf_prog_type _type) : type(_type) {}
+
+
+void ProgramBuilder::append(std::vector instructions)
+{
+ program.insert(program.end(), instructions.begin(), instructions.end());
+}
+
+
+size_t ProgramBuilder::instructions() const { return program.size(); }
+
+
+Program ProgramBuilder::build()
+{
+ union bpf_attr attribute;
+ std::memset(, 0, sizeof(attribute));
+
+ attribute.prog_type = type;
+ attribute.insn_cnt = instructions();
+ attribute.insns = internal::pointer_to_int(program.data());
+ attribute.license = internal::pointer_to_int("Apache 2.0");
+
+ return Program{.attribute = attribute};
+}
+
+
+Try load(const Program& program, bool logErrors)
+{
+ union bpf_attr attribute = program.attribute;
+ Try fd = 0; // placeholder because Try() DNE
+ string logs = "";
+
+ if (logErrors) {
+const int LOG_SIZE = 8196;
+const int LOG_LEVEL = 1;
+
+char* log_buffer = (char*)malloc(LOG_SIZE);
Review Comment:
Resolved by using a vector.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
DevinLeamy commented on code in PR #501:
URL: https://github.com/apache/mesos/pull/501#discussion_r1517860015
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,99 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
+ }
+ return result;
+}
+
+
+uint64_t pointer_to_int(const void* pointer)
+{
+ return (uint64_t)(unsigned long)pointer;
+}
+
+} // namespace internal {
+
+ProgramBuilder::ProgramBuilder(enum bpf_prog_type _type) : type(_type) {}
+
+
+void ProgramBuilder::append(std::vector instructions)
+{
+ program.insert(program.end(), instructions.begin(), instructions.end());
+}
+
+
+size_t ProgramBuilder::instructions() const { return program.size(); }
+
+
+Program ProgramBuilder::build()
+{
+ union bpf_attr attribute;
+ std::memset(, 0, sizeof(attribute));
+
+ attribute.prog_type = type;
+ attribute.insn_cnt = instructions();
+ attribute.insns = internal::pointer_to_int(program.data());
+ attribute.license = internal::pointer_to_int("Apache 2.0");
+
+ return Program{.attribute = attribute};
+}
+
+
+Try load(const Program& program, bool logErrors)
+{
+ union bpf_attr attribute = program.attribute;
+ Try fd = 0; // placeholder because Try() DNE
+ string logs = "";
+
+ if (logErrors) {
+const int LOG_SIZE = 8196;
+const int LOG_LEVEL = 1;
+
+char* log_buffer = (char*)malloc(LOG_SIZE);
+attribute.log_level = LOG_LEVEL;
+attribute.log_buf = internal::pointer_to_int(log_buffer);
+attribute.log_size = LOG_SIZE;
+fd = internal::syscall_bpf(BPF_PROG_LOAD, , sizeof(attribute));
+logs = ": " + string(log_buffer);
+ } else {
+fd = internal::syscall_bpf(BPF_PROG_LOAD, , sizeof(attribute));
+ }
+
+ if (fd.isError()) {
+return Error("BPF syscall failed" + logs + ": " + fd.error());
+ }
Review Comment:
Updated to work similarly to how you described. We attempt and then retry
with logs if there's a validator error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] [cgroups2] Add missing imports [mesos]
DevinLeamy opened a new pull request, #502: URL: https://github.com/apache/mesos/pull/502 (no comment) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
DevinLeamy commented on code in PR #501: URL: https://github.com/apache/mesos/pull/501#discussion_r1517783917 ## src/linux/ebpf.cpp: ## @@ -14,4 +14,99 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include +#include +#include Review Comment: Kept because the direct syscall is not exposed through ``. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
DevinLeamy commented on code in PR #501:
URL: https://github.com/apache/mesos/pull/501#discussion_r1517858068
##
src/linux/ebpf.hpp:
##
@@ -17,4 +17,136 @@
#ifndef __EBPF_HPP__
#define __EBPF_HPP__
+#include
+
+#include
+#include
+
+#include "stout/nothing.hpp"
+#include "stout/try.hpp"
+
+namespace ebpf {
+
+// Loadable eBPF program. Constructed using an ebpf::ProgramBuilder.
+struct Program
+{
+ // eBPF attribute containing an eBPF program.
+ union bpf_attr attribute;
+};
+
+
+// Builder for eBPF programs.
+class ProgramBuilder
+{
+public:
+ explicit ProgramBuilder(enum bpf_prog_type type);
+
+ // Append instructions to the end of the eBPF program.
+ void append(std::vector instructions);
+
+ // Number of instructions in the eBPF program.
+ size_t instructions() const;
+
+ // Build a loadable eBPF program.
+ Program build();
Review Comment:
I opted to copy the program instructions from ProgramBuilder into Program on
build, and then free the instructions in the Program's destructor.
I saw having the `Builder` as useful for implementing the DeviceController
where there's instructions we want to add to the program instructions before it
becomes a `Program`. By having this separation we can do those additions in the
`build()` step, so the caller doesn't have to remember to do some `finalize()`
step before "building" (AKA creating the bpf_attr). The idea is make sure
`Program`s are _truly_ loadable eBPF programs.
If that's not clear, here's the DeviceProgram I'm preparing for the device
controller (note - the API is slightly different but it should make what I'm
going for more evident):
https://github.com/DevinLeamy/mesos/commit/50e4c0ff7cf806ae02e8dc03488b70d00412583f
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Add missing imports [mesos]
bmahler merged PR #502: URL: https://github.com/apache/mesos/pull/502 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Review Request 74918: [cgroups2] Add utility to list all the available subsystems in a cgroup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74918/ --- Review request for mesos. Repository: mesos Description --- [cgroups2] Add utility to list all the available subsystems in a cgroup. Diffs - src/linux/cgroups2.cpp eff44f11ea5a0223b284d6dcd63eebb809b2a872 Diff: https://reviews.apache.org/r/74918/diff/1/ Testing --- Thanks, Devin Leamy
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
bmahler commented on code in PR #501:
URL: https://github.com/apache/mesos/pull/501#discussion_r1518223153
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,90 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
+#include
+
+#include "stout/check.hpp"
+#include "stout/error.hpp"
+#include "stout/try.hpp"
+
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+Try bpf(int cmd, bpf_attr* attr, size_t size)
+{
+ // We retry the system call `attempts` times. The default is 5, as per
+ // what's done by libbpf:
+ // https://github.com/libbpf/libbpf/blob/master/src/bpf.h#L71-L75
+ int result, attempts = 5;
+ do {
+result = (int)syscall(__NR_bpf, cmd, attr, size);
+ } while (errno == EAGAIN && --attempts > 0);
Review Comment:
this needs to check that result == -1 to check errno
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [cgroups2] Add utility to list all the available subsystems in a cgroup. [mesos]
bmahler closed pull request #503: [cgroups2] Add utility to list all the available subsystems in a cgroup. URL: https://github.com/apache/mesos/pull/503 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Add utility to list all the available subsystems in a cgroup. [mesos]
bmahler commented on code in PR #503:
URL: https://github.com/apache/mesos/pull/503#discussion_r1518213231
##
src/linux/cgroups2.cpp:
##
@@ -72,6 +72,27 @@ const std::string TYPE = "cgroup.type";
} // namespace control {
+namespace controllers {
+
+// Find the available controllers (AKA subsystems) in the provided cgroup.
+Try> available(const string& cgroup)
+{
+ Try subsystems =
+cgroups2::read(cgroup, cgroups2::control::CONTROLLERS);
+ if (subsystems.isError()) {
+return Error(
+ "Failed to read cgroup.controllers in '" + cgroup +
+ "': " + subsystems.error());
+ }
+ vector _subsystems = strings::split(subsystems.get(), " ");
+ set systems(
+std::make_move_iterator(_subsystems.begin()),
+std::make_move_iterator(_subsystems.end()));
+ return systems;
Review Comment:
rather than declaring the variable and immediately returning it, we can
return it directly
##
src/linux/cgroups2.cpp:
##
@@ -72,6 +72,27 @@ const std::string TYPE = "cgroup.type";
} // namespace control {
+namespace controllers {
+
+// Find the available controllers (AKA subsystems) in the provided cgroup.
+Try> available(const string& cgroup)
+{
+ Try subsystems =
Review Comment:
s/subsystems/read/
##
src/linux/cgroups2.cpp:
##
@@ -72,6 +72,27 @@ const std::string TYPE = "cgroup.type";
} // namespace control {
+namespace controllers {
+
+// Find the available controllers (AKA subsystems) in the provided cgroup.
+Try> available(const string& cgroup)
+{
+ Try subsystems =
+cgroups2::read(cgroup, cgroups2::control::CONTROLLERS);
+ if (subsystems.isError()) {
+return Error(
+ "Failed to read cgroup.controllers in '" + cgroup +
+ "': " + subsystems.error());
+ }
+ vector _subsystems = strings::split(subsystems.get(), " ");
Review Comment:
can use * rather than .get() here
s/_//
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
bmahler commented on code in PR #501:
URL: https://github.com/apache/mesos/pull/501#discussion_r1518222165
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,90 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
+#include
+
+#include "stout/check.hpp"
+#include "stout/error.hpp"
+#include "stout/try.hpp"
+
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+Try bpf(int cmd, bpf_attr* attr, size_t size)
+{
+ // We retry the system call `attempts` times. The default is 5, as per
+ // what's done by libbpf:
+ // https://github.com/libbpf/libbpf/blob/master/src/bpf.h#L71-L75
+ int result, attempts = 5;
+ do {
+result = (int)syscall(__NR_bpf, cmd, attr, size);
+ } while (result == EAGAIN && --attempts > 0);
Review Comment:
this needs to check errno
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
DevinLeamy commented on code in PR #501:
URL: https://github.com/apache/mesos/pull/501#discussion_r1518170538
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,126 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
+#include
+
+#include "stout/error.hpp"
+#include "stout/try.hpp"
+
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size);
+
+// Wrapper around the BPF syscall.
+Try bpf(int cmd, bpf_attr* attr, size_t size)
+{
+ int result = syscall_bpf(cmd, attr, size);
+ if (result == -1) {
+ErrnoError error;
+return Error("BPF syscall failed with error '" + error.message + "'");
+ }
+ return result;
+}
+
+
+Program::Program(bpf_prog_type _type) : type(_type) {}
+
+
+void Program::append(vector&& instructions)
+{
+ program.insert(
+program.end(),
+std::make_move_iterator(instructions.begin()),
+std::make_move_iterator(instructions.end()));
+}
+
+
+bpf_attr Program::createAttribute() const
+{
+ bpf_attr attribute;
+ std::memset(, 0, sizeof(attribute));
+
+ attribute.prog_type = type;
+ attribute.insn_cnt = program.size();
+ attribute.insns = reinterpret_cast(program.data());
+ attribute.license = reinterpret_cast("Apache 2.0");
+
+ return attribute;
+}
+
+
+Try load(const Program&& program)
+{
+ bpf_attr attribute = program.createAttribute();
+ string logs = "";
+
+ // The BPF system call is repeated based on whether it succeeds and the
+ // return value. If it succeeds, we don't retry. It it fails with 'EAGAIN'
+ // we retry with the same parameters. If it fails with 'EACCES' - a
+ // verification error - when we retry once with an additional buffer to
+ // capture the verifier error logs.
+ //
+ // We attempt a maximum of 3 times, to avoid an infinite loop if 'EAGAIN' is
+ // returned multiple times.
+ int attempts = 0;
+ bool retry = false;
+ bool retriedWithLogs = false;
+
+ Try fd = 0; // '0' because Try has no default constructor.
Review Comment:
Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
DevinLeamy commented on code in PR #501:
URL: https://github.com/apache/mesos/pull/501#discussion_r1518170872
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,126 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
+#include
+
+#include "stout/error.hpp"
+#include "stout/try.hpp"
+
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size);
+
+// Wrapper around the BPF syscall.
+Try bpf(int cmd, bpf_attr* attr, size_t size)
+{
+ int result = syscall_bpf(cmd, attr, size);
+ if (result == -1) {
+ErrnoError error;
+return Error("BPF syscall failed with error '" + error.message + "'");
+ }
+ return result;
+}
+
+
+Program::Program(bpf_prog_type _type) : type(_type) {}
+
+
+void Program::append(vector&& instructions)
+{
+ program.insert(
+program.end(),
+std::make_move_iterator(instructions.begin()),
+std::make_move_iterator(instructions.end()));
+}
+
+
+bpf_attr Program::createAttribute() const
+{
+ bpf_attr attribute;
+ std::memset(, 0, sizeof(attribute));
+
+ attribute.prog_type = type;
+ attribute.insn_cnt = program.size();
+ attribute.insns = reinterpret_cast(program.data());
+ attribute.license = reinterpret_cast("Apache 2.0");
+
+ return attribute;
+}
+
+
+Try load(const Program&& program)
+{
+ bpf_attr attribute = program.createAttribute();
+ string logs = "";
+
+ // The BPF system call is repeated based on whether it succeeds and the
+ // return value. If it succeeds, we don't retry. It it fails with 'EAGAIN'
+ // we retry with the same parameters. If it fails with 'EACCES' - a
+ // verification error - when we retry once with an additional buffer to
+ // capture the verifier error logs.
+ //
+ // We attempt a maximum of 3 times, to avoid an infinite loop if 'EAGAIN' is
+ // returned multiple times.
+ int attempts = 0;
+ bool retry = false;
+ bool retriedWithLogs = false;
+
+ Try fd = 0; // '0' because Try has no default constructor.
+ do {
Review Comment:
Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
DevinLeamy commented on code in PR #501:
URL: https://github.com/apache/mesos/pull/501#discussion_r1518185616
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,126 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
+#include
+
+#include "stout/error.hpp"
+#include "stout/try.hpp"
+
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size);
+
+// Wrapper around the BPF syscall.
+Try bpf(int cmd, bpf_attr* attr, size_t size)
+{
+ int result = syscall_bpf(cmd, attr, size);
+ if (result == -1) {
+ErrnoError error;
+return Error("BPF syscall failed with error '" + error.message + "'");
Review Comment:
Make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] [cgroups2] Add utility to list all the available subsystems in a cgroup. [mesos]
DevinLeamy opened a new pull request, #503: URL: https://github.com/apache/mesos/pull/503 Interface to the "cgroup.controllers" control file. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
bmahler commented on code in PR #501:
URL: https://github.com/apache/mesos/pull/501#discussion_r1518049989
##
src/linux/ebpf.hpp:
##
@@ -14,7 +14,128 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+// TODO(dleamy): Look into using libbpf: https://github.com/libbpf/libbpf
+// to simplify and or replace the low-level BPF operations.
+
#ifndef __EBPF_HPP__
#define __EBPF_HPP__
+#include
+
+#include
+
+#include "stout/try.hpp"
+
+namespace ebpf {
+
+// eBPF program.
+class Program
+{
+public:
+ explicit Program(bpf_prog_type type);
+
+ // Append instructions to the end of the eBPF program.
+ void append(std::vector&& instructions);
+
+ // Create a eBPF attribute for the program that can be loaded
+ // into the kernel.
+ bpf_attr createAttribute() const;
+
+protected:
+ // Type of eBPF program.
+ const bpf_prog_type type;
+
+ // Instructions of the eBPF program.
+ std::vector program;
+};
+
+
+// Loads the provided eBPF program into the kernel and returns the file
+// descriptor of loaded program.
+Try load(const Program&& program);
Review Comment:
remove const here if you want to take an rvalue reference
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,126 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
+#include
+
+#include "stout/error.hpp"
+#include "stout/try.hpp"
+
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size);
Review Comment:
let's remove this macro, it isn't adding much value at this point
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,126 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
+#include
+
+#include "stout/error.hpp"
+#include "stout/try.hpp"
+
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size);
+
+// Wrapper around the BPF syscall.
+Try bpf(int cmd, bpf_attr* attr, size_t size)
+{
+ int result = syscall_bpf(cmd, attr, size);
+ if (result == -1) {
+ErrnoError error;
+return Error("BPF syscall failed with error '" + error.message + "'");
+ }
+ return result;
+}
Review Comment:
let's point to the lwn article I sent you on why glibc doesn't expose a
function for this syscall, to explain why we need to make the direct syscall
here
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,126 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
+#include
+
+#include "stout/error.hpp"
+#include "stout/try.hpp"
+
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size);
+
+// Wrapper around the BPF syscall.
+Try bpf(int cmd, bpf_attr* attr, size_t size)
+{
+ int result = syscall_bpf(cmd, attr, size);
+ if (result == -1) {
+ErrnoError error;
+return Error("BPF syscall failed with error '" + error.message + "'");
+ }
+ return result;
+}
+
+
+Program::Program(bpf_prog_type _type) : type(_type) {}
+
+
+void Program::append(vector&& instructions)
+{
+ program.insert(
+program.end(),
+std::make_move_iterator(instructions.begin()),
+std::make_move_iterator(instructions.end()));
+}
+
+
+bpf_attr Program::createAttribute() const
+{
+ bpf_attr attribute;
+ std::memset(, 0, sizeof(attribute));
+
+ attribute.prog_type = type;
+ attribute.insn_cnt = program.size();
+ attribute.insns = reinterpret_cast(program.data());
+ attribute.license = reinterpret_cast("Apache 2.0");
+
+ return attribute;
+}
Review Comment:
this is still a dangerous function because the caller *must* keep Program
alive longer than the bpf_attr returned by this function, and that's not
obvious (e.g. this is called create and returns a value, rather than, say, a
const ref) and easy to get wrong and cause a use-after-free bug
we could instead just expose the instructions and the license, and have
`load(const Program& program)` build the bpf_attr struct needed for loading,
this also avoids the need to do any moving since Program clearly outlives load
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,126 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
bmahler commented on code in PR #501:
URL: https://github.com/apache/mesos/pull/501#discussion_r1518226542
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,90 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
+#include
+
+#include "stout/check.hpp"
+#include "stout/error.hpp"
+#include "stout/try.hpp"
+
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+Try bpf(int cmd, bpf_attr* attr, size_t size)
+{
+ // We retry the system call `attempts` times. The default is 5, as per
+ // what's done by libbpf:
+ // https://github.com/libbpf/libbpf/blob/master/src/bpf.h#L71-L75
+ int result, attempts = 5;
+ do {
+result = (int)syscall(__NR_bpf, cmd, attr, size);
+ } while (errno == EAGAIN && --attempts > 0);
+
+ if (result == -1) {
+return ErrnoError();
+ }
+ return result == -1;
Review Comment:
return result;
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,90 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
+#include
+
+#include "stout/check.hpp"
+#include "stout/error.hpp"
+#include "stout/try.hpp"
+
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+Try bpf(int cmd, bpf_attr* attr, size_t size)
+{
+ // We retry the system call `attempts` times. The default is 5, as per
+ // what's done by libbpf:
+ // https://github.com/libbpf/libbpf/blob/master/src/bpf.h#L71-L75
+ int result, attempts = 5;
+ do {
+result = (int)syscall(__NR_bpf, cmd, attr, size);
+ } while (errno == EAGAIN && --attempts > 0);
Review Comment:
```
do {
result = (int)syscall(__NR_bpf, cmd, attr, size);
} while (result == -1 && errno == EAGAIN && --attempts > 0);
```
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,90 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
+#include
+
+#include "stout/check.hpp"
+#include "stout/error.hpp"
+#include "stout/try.hpp"
+
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+Try bpf(int cmd, bpf_attr* attr, size_t size)
+{
+ // We retry the system call `attempts` times. The default is 5, as per
+ // what's done by libbpf:
+ // https://github.com/libbpf/libbpf/blob/master/src/bpf.h#L71-L75
+ int result, attempts = 5;
+ do {
+result = (int)syscall(__NR_bpf, cmd, attr, size);
+ } while (errno == EAGAIN && --attempts > 0);
+
+ if (result == -1) {
+return ErrnoError();
+ }
+ return result == -1;
+}
+
+
+Program::Program(bpf_prog_type _type) : type(_type) {}
+
+
+void Program::append(vector&& instructions)
+{
+ program.insert(
+program.end(),
+std::make_move_iterator(instructions.begin()),
+std::make_move_iterator(instructions.end()));
+}
+
+
+Try load(Program& program)
Review Comment:
const& here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
DevinLeamy commented on code in PR #501: URL: https://github.com/apache/mesos/pull/501#discussion_r1518024442 ## src/linux/ebpf.hpp: ## @@ -17,4 +17,136 @@ #ifndef __EBPF_HPP__ #define __EBPF_HPP__ +#include + +#include +#include + +#include "stout/nothing.hpp" +#include "stout/try.hpp" + Review Comment: Added. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
bmahler commented on code in PR #501:
URL: https://github.com/apache/mesos/pull/501#discussion_r1518162434
##
src/linux/ebpf.cpp:
##
@@ -14,4 +14,126 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-#include "linux/ebpf.hpp"
\ No newline at end of file
+#include "linux/ebpf.hpp"
+
+#include
+#include
+
+#include
+#include
+
+#include "stout/error.hpp"
+#include "stout/try.hpp"
+
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size);
+
+// Wrapper around the BPF syscall.
+Try bpf(int cmd, bpf_attr* attr, size_t size)
+{
+ int result = syscall_bpf(cmd, attr, size);
+ if (result == -1) {
+ErrnoError error;
+return Error("BPF syscall failed with error '" + error.message + "'");
+ }
+ return result;
+}
+
+
+Program::Program(bpf_prog_type _type) : type(_type) {}
+
+
+void Program::append(vector&& instructions)
+{
+ program.insert(
+program.end(),
+std::make_move_iterator(instructions.begin()),
+std::make_move_iterator(instructions.end()));
+}
+
+
+bpf_attr Program::createAttribute() const
+{
+ bpf_attr attribute;
+ std::memset(, 0, sizeof(attribute));
+
+ attribute.prog_type = type;
+ attribute.insn_cnt = program.size();
+ attribute.insns = reinterpret_cast(program.data());
+ attribute.license = reinterpret_cast("Apache 2.0");
+
+ return attribute;
+}
+
+
+Try load(const Program&& program)
+{
+ bpf_attr attribute = program.createAttribute();
+ string logs = "";
+
+ // The BPF system call is repeated based on whether it succeeds and the
+ // return value. If it succeeds, we don't retry. It it fails with 'EAGAIN'
+ // we retry with the same parameters. If it fails with 'EACCES' - a
+ // verification error - when we retry once with an additional buffer to
+ // capture the verifier error logs.
+ //
+ // We attempt a maximum of 3 times, to avoid an infinite loop if 'EAGAIN' is
+ // returned multiple times.
+ int attempts = 0;
+ bool retry = false;
+ bool retriedWithLogs = false;
+
+ Try fd = 0; // '0' because Try has no default constructor.
+ do {
+fd = bpf(BPF_PROG_LOAD, , sizeof(attribute));
+++attempts;
+
+if (fd.isSome()) {
+ break;
+}
+
+ErrnoError error;
+switch (error.code) {
+ case EAGAIN: {
+// Retry with the same parameters.
+retry = true;
+break;
+ }
+ case EACCES: {
+// Retry and capture verifier error logs, if we haven't already.
+if (retriedWithLogs) {
+ retry = false;
+ break;
+}
+
+retry = true;
+vector verifier_log_buffer(8196);
+attribute.log_level = 1;
+attribute.log_buf =
+ reinterpret_cast(verifier_log_buffer.data());
+attribute.log_size = verifier_log_buffer.size();
+fd = bpf(BPF_PROG_LOAD, , sizeof(attribute));
Review Comment:
perhaps add a CHECK here which requires using `Try` as the
bpf return type:
```
CHECK_ERROR(fd);
CHECK_EQ(EACCESS, fd.error().code) << "Expected verifier error again";
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [ebpf][cgroups2] Attaching a eBFP programs to a cgroups. [mesos]
bmahler closed pull request #505: [ebpf][cgroups2] Attaching a eBFP programs to a cgroups. URL: https://github.com/apache/mesos/pull/505 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Rename all 'subsystems' to 'controllers'. [mesos]
bmahler merged PR #506: URL: https://github.com/apache/mesos/pull/506 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [ebpf][cgroups2] Attaching a eBFP programs to a cgroups. [mesos]
DevinLeamy opened a new pull request, #505: URL: https://github.com/apache/mesos/pull/505 Introduces `ebpf::cgroups2::attach()` which allows an `ebpf::Program()` to be attached to a cgroup, given the absolute path to the cgroup and the file descriptor of the loaded program. This will be used by cgroups v2 Device Controller. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [ebpf] Introduced API for loading eBPF programs into the kernel. [mesos]
bmahler merged PR #501: URL: https://github.com/apache/mesos/pull/501 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [cgroups2] Check if a list of subsystems are available to a cgroup. [mesos]
DevinLeamy opened a new pull request, #504: URL: https://github.com/apache/mesos/pull/504 Introduces a utility to check if a list of subsystems can be enabled by a provided cgroup. This is equivalent to checking if the subsystems are listed the cgroup's "cgroup.controllers" file. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [cgroups2] Check if a list of subsystems are available to a cgroup. [mesos]
DevinLeamy closed pull request #504: [cgroups2] Check if a list of subsystems are available to a cgroup. URL: https://github.com/apache/mesos/pull/504 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [ebpf][cgroups2] Attaching a eBFP programs to a cgroups. [mesos]
bmahler commented on code in PR #505:
URL: https://github.com/apache/mesos/pull/505#discussion_r1518354832
##
src/linux/ebpf.cpp:
##
@@ -96,4 +98,51 @@ Try load(const Program& program)
return *fd;
}
+namespace cgroups2 {
+
+Try attach(int fd, const string& cgroup)
+{
+ Try cgroupFd = os::open(cgroup, O_DIRECTORY | O_RDONLY);
Review Comment:
we need a O_CLOEXEC here as well
##
src/linux/ebpf.cpp:
##
@@ -96,4 +98,51 @@ Try load(const Program& program)
return *fd;
}
+namespace cgroups2 {
+
+Try attach(int fd, const string& cgroup)
+{
+ Try cgroupFd = os::open(cgroup, O_DIRECTORY | O_RDONLY);
+ if (cgroupFd.isError()) {
+return Error(
+"Failed to find the file descriptor of '" + cgroup +
Review Comment:
Failed to open '...': ...
##
src/linux/ebpf.cpp:
##
@@ -96,4 +98,51 @@ Try load(const Program& program)
return *fd;
}
+namespace cgroups2 {
+
+Try attach(int fd, const string& cgroup)
+{
+ Try cgroupFd = os::open(cgroup, O_DIRECTORY | O_RDONLY);
+ if (cgroupFd.isError()) {
+return Error(
+"Failed to find the file descriptor of '" + cgroup +
+"': " + cgroupFd.error());
+ }
+
+ bpf_attr attr;
+ memset(, 0, sizeof(attr));
+ attr.attach_type = BPF_CGROUP_DEVICE;
+ attr.target_fd = *cgroupFd;
+ attr.attach_bpf_fd = fd;
+ // BPF_F_ALLOW_MULTI allows multiple eBPF programs to be attached to a single
+ // cgroup and determines the order in which attached programs will run.
+ //
+ // Rules (assuming all cgroups use BPF_F_ALLOW_MULTI):
+ // 1. Programs attached to child cgroups run before programs attached
+ //to their parent.
+ // 2. Within a cgroup, programs attached earlier will run before programs
+ //attached later.
+ //
+ // Order: oldest, ..., newest
+ // cgroup1: A, B
+ // cgroup2: C
+ // cgroup3: D, E
+ // cgroup4: F
+ // cgroup4 run order: F, D, E, A, B
+ // cgroup2 run order: C, A, B
+ //
+ // src:
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/bpf.h#L1090
Review Comment:
let's use a permalink here instead, or it will drift as the linux source
evolves
##
src/linux/ebpf.cpp:
##
@@ -96,4 +98,51 @@ Try load(const Program& program)
return *fd;
}
+namespace cgroups2 {
+
+Try attach(int fd, const string& cgroup)
+{
+ Try cgroupFd = os::open(cgroup, O_DIRECTORY | O_RDONLY);
Review Comment:
we have to close this file descriptor always on exit of this function, or we
will leak open file descriptors
##
src/linux/ebpf.cpp:
##
@@ -96,4 +98,51 @@ Try load(const Program& program)
return *fd;
}
+namespace cgroups2 {
+
+Try attach(int fd, const string& cgroup)
+{
+ Try cgroupFd = os::open(cgroup, O_DIRECTORY | O_RDONLY);
+ if (cgroupFd.isError()) {
+return Error(
+"Failed to find the file descriptor of '" + cgroup +
+"': " + cgroupFd.error());
+ }
+
+ bpf_attr attr;
+ memset(, 0, sizeof(attr));
+ attr.attach_type = BPF_CGROUP_DEVICE;
+ attr.target_fd = *cgroupFd;
+ attr.attach_bpf_fd = fd;
+ // BPF_F_ALLOW_MULTI allows multiple eBPF programs to be attached to a single
+ // cgroup and determines the order in which attached programs will run.
Review Comment:
we don't want multiple programs within a single cgroup, rather we want to
replace them, we can add a TODO to perform replacement for now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
