Allows specifying socket/numa node for BESS #946
Conversation
Using "-n" flag a user may now specify the desired socket to allocate huge pages from when using BESS. Also includes a check for worker core existing on the correct socket before trying to launch a worker. If worker is launched on a core on an unused socket GetDefaultPool will crash bessd. Signed-off-by: Tim Rozet <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #946 +/- ##
=======================================
Coverage 55.78% 55.78%
=======================================
Files 9 9
Lines 1149 1149
=======================================
Hits 641 641
Misses 508 508 Continue to review full report at Codecov.
|
Assumptions were being made to use socket 0 when detection for socket id failed. This patch defaults to the socket specified at runtime. Signed-off-by: Tim Rozet <[email protected]>
There was a problem hiding this comment.
Thanks for this! I'm afraid this will break many users who depend on the current default behavior though. To avoid breaking things, could you make the following high-level changes?
- Have
-n(or--nodes?) accept a list of nodes to allocate from and allocate from all nodes when the flag isn't set. For example,-n 0,2,3on a four-node system would allocate hugepages from nodes0,2, and3but not1. - Crashing or disallowing workers on nodes without memory allocated seems a bit extreme to me. Instead, what if we default to the first available node with memory in cases like this and log a warning? @sangjinhan, @shinae-woo does this seem like a reasonable default?
| sid = FLAGS_n; | ||
| LOG(WARNING) << "Unable to detect socket ID, defaulting to: " << sid; | ||
| } else { | ||
| LOG(INFO) << "socket ID in pmd is: " <<sid; |
There was a problem hiding this comment.
same thing about including the port name here
| /* if socket_id is invalid, set to arg */ | ||
| if (sid < 0 || sid > RTE_MAX_NUMA_NODES) { | ||
| sid = FLAGS_n; | ||
| LOG(WARNING) << "Unable to detect socket ID, defaulting to: " << sid; |
There was a problem hiding this comment.
perhaps include the name of PMDPort being configured in this log?
| if (socket_ == SOCKET_ID_ANY) { | ||
| LOG(WARNING) << "rte_socket_id() returned -1 for " << arg->core; | ||
| socket_ = 0; | ||
| if (rte_lcore_to_socket_id((unsigned int) core_) != (unsigned int) socket_) { |
There was a problem hiding this comment.
style guide nit: could you explicitly brace initialize, or perhaps static_cast, here instead of c-style casting?
| @@ -567,6 +567,12 @@ class BESSControlImpl final : public BESSControl::Service { | |||
| if (!is_cpu_present(core)) { | |||
| return return_with_error(response, EINVAL, "Invalid core %d", core); | |||
| } | |||
|
|
|||
| if (rte_lcore_to_socket_id(core) != (unsigned int) FLAGS_n) { | |||
There was a problem hiding this comment.
style guide nit: could you explicitly brace initialize, or perhaps static_cast, here instead of c-style casting?
| } | ||
|
|
||
| LOG(INFO) << "Running worker: " << wid_ << " on core " << core_ << " on socket " << socket_; |
There was a problem hiding this comment.
nit: perhaps demote this to VLOG(1)
Using "-n" flag a user may now specify the desired socket to allocate
huge pages from when using BESS.
Also includes a check for worker core existing on the correct socket
before trying to launch a worker. If worker is launched on a core on an
unused socket GetDefaultPool will crash bessd.
Signed-off-by: Tim Rozet [email protected]