-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add builtin numaid to get NUMA Node ID #2177
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if node
is the best name here, it's quite generic. What about numa
, numanode
, or numaid
? Any particular reason why it should be node
?
Also I'd love to see some actual example where this can be useful.
Rename Such as bpftrace script
numa information
Example 1:
Example 2:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this makes sense to me, though I'd like to have a review from at least one another maintainer.
Also please squash the commits, thanks!
0d9d7da
to
afa7801
Compare
Ok, squash done. |
fcb70ad
to
afa7801
Compare
It'd be better if you rebased your one commit onto master, i.e. when checked on Once you have it done, I'll run the tests and help you with potential failures. |
afa7801
to
b56529a
Compare
Thanks a lot. I don't know what's wrong with
|
you can ignore these two if they're consistent with the rest of them. clang-format doesn't work the way we want in every case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! Would it be possible to add a runtime test so we have some kind of coverage for it. Not sure if numactl is possible in the ci
ok, i'll try. |
7b5da59
to
4367c54
Compare
dbd6e5f
to
bc4a12c
Compare
Yeah, ignore this one and keep it consistent with the rest. |
There's another clang-format issue which should be fixed. |
Still:
|
Fix this one (apply the patch proposed by clang-format).
Ignore this one. Thanks! |
Thank you, Is there anything else the code needs to improve? |
Nope, otherwise looks good to me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codegen test LLVM needs to be updated to the new version including the zext
instruction.
Just fix it. the clang-format error still can be ignored. |
One part of the clang-format can be fixed, please do it (see my comment above). Also codegen test is still failing for embedded build. |
clang-format still:
and i don't know how to fix,
On my server there are some errors always with
In the end of
I try
|
I know what happen, see #2240 |
Just apply the first patch that clang-format suggests.
This test is just validating the LLVM IR files (checks that they have correct syntax). The actual comparison of the produced files with expected ones is done in the Check this one: https://github.com/iovisor/bpftrace/runs/6590317804?check_suite_focus=true
These are not test failures when they say "OK".
This is a known issue with LLVM 13, that's why that test is disabled in CI. |
You mean this patch? It's looks like wired. --- a/src/ast/irbuilderbpf.cpp
+++ b/src/ast/irbuilderbpf.cpp
@@ -911,[7](https://github.com/iovisor/bpftrace/runs/6590317810?check_suite_focus=true#step:8:8) +911,[8](https://github.com/iovisor/bpftrace/runs/6590317810?check_suite_focus=true#step:8:9) @@ CallInst *IRBuilderBPF::CreateGetNumaId()
// long bpf_get_numa_node_id(void)
// Return: NUMA Node ID
FunctionType *getnumaid_func_type = FunctionType::get(getInt32Ty(), false);
- PointerType *getnumaid_func_ptr_type = PointerType::get(getnumaid_func_type, 0);
+ PointerType *getnumaid_func_ptr_type = PointerType::get(getnumaid_func_type,
+ 0); Yes, that one. I agree it looks a bit weird, but it follows our coding standard (max 80-char lines). If you don't like it, you can e.g. shorten the variable names so that it fits into one line, but we should be following the rules as much as possible. |
4b5b00e
to
592a116
Compare
Just one clang-format check error left. diff --git a/src/ast/irbuilderbpf.h b/src/ast/irbuilderbpf.h
index f190184..b4ff1[6](https://github.com/iovisor/bpftrace/runs/6594935209?check_suite_focus=true#step:8:7)0 100644
--- a/src/ast/irbuilderbpf.h
+++ b/src/ast/irbuilderbpf.h
@@ -120,[7](https://github.com/iovisor/bpftrace/runs/6594935209?check_suite_focus=true#step:8:8) +[12](https://github.com/iovisor/bpftrace/runs/6594935209?check_suite_focus=true#step:8:13)0,7 @@ public:
CallInst *CreateGetPidTgid();
CallInst *CreateGetCurrentCgroupId();
CallInst *CreateGetUidGid();
- CallInst *CreateGetNumaId();
+ CallInst *CreateGetNumaId();
CallInst *CreateGetCpuId();
CallInst *CreateGetCurrentTask();
CallInst *CreateGetRandom();
Error: Process completed with exit code 1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One last thing - please rebase onto the current master (there's a CI bug fixed).
Example: NUMA node information: $ numactl --hardware available: 2 nodes (0-1) node 0 cpus: 0 1 2 3 4 5 6 7 ... [...] node 1 cpus: 20 21 22 23 24 25 ... [...] node distances: node 0 1 0: 10 21 1: 21 10 bpftrace script code net_action.bt: kprobe:net_tx_action, kprobe:net_rx_action { @[numaid, comm] = count(); } Example1: ------------------------------------------------------------ iperf3 server: $ taskset -c 1 iperf3 -s [...] [ 5] 0.00-1.00 sec 6.04 GBytes 51.9 Gbits/sec [...] iperf3 client: $ taskset -c 2 iperf3 -c 0 [...] [ 5] 0.00-1.00 sec 6.04 GBytes 51.9 Gbits/sec 0 1.19 MBytes [...] net_action.bt output: @[0, iperf3]: 2430857 Because both cpu1 and cpu2 are on the numa node 0. Example2: ------------------------------------------------------------ iperf3 server: $ taskset -c 1 iperf3 -s [...] [ 5] 0.00-1.00 sec 2.79 GBytes 24.0 Gbits/sec [...] iperf3 client: $ taskset -c 21 iperf3 -c 0 [...] [ 5] 0.00-1.00 sec 2.79 GBytes 24.0 Gbits/sec 0 1023 KBytes [...] net_action.bt output: @[0, iperf3]: 456193 @[1, iperf3]: 684853 Because cpu1 on numa node0, cpu21 on numa node1, Processes that need to communicate running on different nodes cause poor performance, It is very easy to get numa with `numaid`.
rebase done. |
@fbs if you could have one more look at this, we can merge then. |
looks good, lets get this in Guess it would be nice to have kernel feature detecting as 5.8 is a relatively new kernel but that doesn't have to block this mr |
Hi, looks like it could merge |
Add node builtin to get NUMA Node ID:
It't useful in multi-node system, like
numactl --hardware
:Add node builtin variable for getting NUMA node ID.