You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ar...@apache.org on 2019/03/15 19:00:44 UTC
[impala] 01/03: IMPALA-8277: support ranges in CPU lists
This is an automated email from the ASF dual-hosted git repository.
arodoni pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit b79a03bf408e32d0a29ead33c788b84e7bf1d439
Author: Tim Armstrong <ta...@cloudera.com>
AuthorDate: Thu Mar 14 14:37:53 2019 -0700
IMPALA-8277: support ranges in CPU lists
Generalise the logic to handle comma-separated ranges.
Testing:
Added unit test for parsing logic to test the various possible
permutations of CPU ranges.
Porting notes:
This is a port of KUDU-2721. There were some minor conflicts
around include ordering in sysinfo.cc that I resolved by
switching more of includes to match Kudu's version. I also
made sure to make sysinfo-test runnable without modifications
to the source file, which is possible with the new unified
backend test infrastructure.
Change-Id: I97311bfbcf70bea069e941b6e7f4f015fb781b3f
Reviewed-on: http://gerrit.cloudera.org:8080/12657
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-on: http://gerrit.cloudera.org:8080/12755
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/CMakeLists.txt | 1 +
be/src/gutil/CMakeLists.txt | 6 ++++
be/src/gutil/sysinfo-test.cc | 71 ++++++++++++++++++++++++++++++++++++++
be/src/gutil/sysinfo.cc | 82 +++++++++++++++++++++++++++++++++++---------
be/src/gutil/sysinfo.h | 14 ++++++++
5 files changed, 157 insertions(+), 17 deletions(-)
diff --git a/be/CMakeLists.txt b/be/CMakeLists.txt
index 204f390..b3f1249 100644
--- a/be/CMakeLists.txt
+++ b/be/CMakeLists.txt
@@ -439,6 +439,7 @@ set (IMPALA_LINK_LIBS
set(WL_WHOLE_ARCHIVE "-Wl,--whole-archive")
set(WL_NO_WHOLE_ARCHIVE "-Wl,--no-whole-archive")
set (UNIFIED_TEST_LIBS
+ GUtilTests
UtilTests
)
set (UNIFIED_TEST_LINK_LIBS
diff --git a/be/src/gutil/CMakeLists.txt b/be/src/gutil/CMakeLists.txt
index f895679..87612da 100644
--- a/be/src/gutil/CMakeLists.txt
+++ b/be/src/gutil/CMakeLists.txt
@@ -71,3 +71,9 @@ ADD_EXPORTABLE_LIBRARY(gutil
COMPILE_FLAGS "-funsigned-char -Wno-deprecated -Wno-char-subscripts")
add_kudu_test(strings/string_util-test)
+
+add_library(GUtilTests
+ sysinfo-test.cc
+)
+add_dependencies(GUtilTests gen-deps gen_ir_descriptions)
+ADD_UNIFIED_BE_LSAN_TEST(sysinfo-test "SysInfoTest.*")
diff --git a/be/src/gutil/sysinfo-test.cc b/be/src/gutil/sysinfo-test.cc
new file mode 100644
index 0000000..a485bb2
--- /dev/null
+++ b/be/src/gutil/sysinfo-test.cc
@@ -0,0 +1,71 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "kudu/gutil/sysinfo.h"
+
+#include <gtest/gtest.h>
+
+namespace kudu {
+
+TEST(SysInfoTest, ReadMaxCpuIndexTest) {
+ using base::ParseMaxCpuIndex;
+ EXPECT_EQ(0, ParseMaxCpuIndex("0\n"));
+ EXPECT_EQ(1, ParseMaxCpuIndex("1\n"));
+ EXPECT_EQ(7, ParseMaxCpuIndex("0-7\n"));
+ EXPECT_EQ(40, ParseMaxCpuIndex("0-7,30-40\n"));
+ EXPECT_EQ(143, ParseMaxCpuIndex("2,4-127,128-143\n"));
+ EXPECT_EQ(44, ParseMaxCpuIndex("44-44\n"));
+
+ // Don't assume that ranges are in ascending order or non-overlapping,
+ // just in case.
+ EXPECT_EQ(8, ParseMaxCpuIndex("0-7,5-8\n"));
+ EXPECT_EQ(7, ParseMaxCpuIndex("0-7,5-6\n"));
+ EXPECT_EQ(3, ParseMaxCpuIndex("2-3,0-1\n"));
+ EXPECT_EQ(3, ParseMaxCpuIndex("2-3,0\n"));
+ EXPECT_EQ(3, ParseMaxCpuIndex("3,0-2\n"));
+
+ // Invalid inputs.
+ EXPECT_EQ(-1, ParseMaxCpuIndex(""));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("\n"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex(" "));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("a\n"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("0\n1\n"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("\n1\n"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("\n1"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("0-\n"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("-2\n"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("1-9qwerty\n"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("1-9,0-\n"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("1,2,3-\n"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("1,2,a-4\n"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("1,2,3@4\n"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("1,2,\n"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("3-2\n"));
+
+ // Overflows in various positions.
+ EXPECT_EQ(-1, ParseMaxCpuIndex("2147483648")); // 2^31
+ EXPECT_EQ(-1, ParseMaxCpuIndex("18446744073709551617")); // 2^64 + 1
+ EXPECT_EQ(-1, ParseMaxCpuIndex("999999999999999999999999999999999999999999999999"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("0-2147483648")); // 2^31
+ EXPECT_EQ(-1, ParseMaxCpuIndex("0-18446744073709551617")); // 2^64 + 1
+ EXPECT_EQ(-1, ParseMaxCpuIndex("0-999999999999999999999999999999999999999999999999"));
+ EXPECT_EQ(-1, ParseMaxCpuIndex("2147483648-1")); // 2^31
+ EXPECT_EQ(-1, ParseMaxCpuIndex("18446744073709551617-1")); // 2^64 + 1
+ EXPECT_EQ(-1, ParseMaxCpuIndex("999999999999999999999999999999999999999999999999-1"));
+}
+
+} // namespace kudu
diff --git a/be/src/gutil/sysinfo.cc b/be/src/gutil/sysinfo.cc
index dddfa74..01f2597 100644
--- a/be/src/gutil/sysinfo.cc
+++ b/be/src/gutil/sysinfo.cc
@@ -33,11 +33,7 @@
#endif
#include <ctype.h> // for isspace()
-#include <stdlib.h> // for getenv()
-#include <stdio.h> // for snprintf(), sscanf()
-#include <string.h> // for memmove(), memchr(), etc.
#include <fcntl.h> // for open()
-#include <errno.h> // for errno
#ifdef HAVE_UNISTD_H
#include <unistd.h> // for read()
#endif
@@ -53,12 +49,25 @@
#include <shlwapi.h> // for SHGetValueA()
#include <tlhelp32.h> // for Module32First()
#endif
+
+#include "gutil/sysinfo.h"
+
+#include <algorithm>
+#include <cerrno> // for errno
+#include <cstdio> // for snprintf(), sscanf()
+#include <cstdlib> // for getenv()
+#include <cstring> // for memmove(), memchr(), etc.
+#include <ctime>
+#include <limits>
+#include <ostream>
+
#include "gutil/dynamic_annotations.h" // for RunningOnValgrind
#include "gutil/macros.h"
-#include "gutil/sysinfo.h"
#include "gutil/walltime.h"
#include <common/logging.h>
+using std::numeric_limits;
+
// This isn't in the 'base' namespace in tcmallc. But, tcmalloc
// exports these functions, so we need to namespace them to avoid
// the conflict.
@@ -154,21 +163,60 @@ static bool ReadIntFromFile(const char *file, int *value) {
static int ReadMaxCPUIndex() {
char buf[1024];
+ // TODO(tarmstrong): KUDU-2730: 'present' doesn't include CPUs that could be hotplugged
+ // in the future. 'possible' does, but using it instead could result in a blow-up in the
+ // number of per-CPU data structures.
CHECK(SlurpSmallTextFile("/sys/devices/system/cpu/present", buf, arraysize(buf)));
+ int max_idx = ParseMaxCpuIndex(buf);
+ CHECK_GE(max_idx, 0) << "unable to parse max CPU index from: " << buf;
+ return max_idx;
+}
- // On a single-core machine, 'buf' will contain the string '0' with a newline.
- if (strcmp(buf, "0\n") == 0) {
- return 0;
- }
-
- // On multi-core, it will have a CPU range like '0-7'.
- CHECK_EQ(0, memcmp(buf, "0-", 2)) << "bad list of possible CPUs: " << buf;
+int ParseMaxCpuIndex(const char* str) {
+ DCHECK(str != nullptr);
+ const char* pos = str;
+ // Initialize max_idx to invalid so we can just return if we find zero ranges.
+ int max_idx = -1;
+
+ while (true) {
+ const char* range_start = pos;
+ const char* dash = nullptr;
+ // Scan forward until we find the separator indicating end of range, which is always a
+ // newline or comma if the input is valid.
+ for (; *pos != ',' && *pos != '\n'; pos++) {
+ // Check for early end of string - bail here to avoid advancing past end.
+ if (*pos == '\0') return -1;
+ if (*pos == '-') {
+ // Multiple dashes in range is invalid.
+ if (dash != nullptr) return -1;
+ dash = pos;
+ } else if (!isdigit(*pos)) {
+ return -1;
+ }
+ }
- char* max_cpu_str = &buf[2];
- char* err;
- int val = strtol(max_cpu_str, &err, 10);
- CHECK(*err == '\n' || *err == '\0') << "unable to parse max CPU index from: " << buf;
- return val;
+ // At this point we found a range [range_start, pos) comprised of digits and an
+ // optional dash.
+ const char* num_start = dash == nullptr ? range_start : dash + 1;
+ // Check for ranges with missing numbers, e.g. "", "3-", "-3".
+ if (num_start == pos || dash == range_start) return -1;
+ // The numbers are comprised only of digits, so it can only fail if it is out of
+ // range of int (the return type of this function).
+ unsigned long start_idx = strtoul(range_start, nullptr, 10);
+ if (start_idx > numeric_limits<int>::max()) return -1;
+ unsigned long end_idx = strtoul(num_start, nullptr, 10);
+ if (end_idx > numeric_limits<int>::max() || start_idx > end_idx) {
+ return -1;
+ }
+ // Keep track of the max index we've seen so far.
+ max_idx = std::max(static_cast<int>(end_idx), max_idx);
+ // End of line, expect no more input.
+ if (*pos == '\n') break;
+ ++pos;
+ }
+ // String must have a single newline at the very end.
+ if (*pos != '\n' || *(pos + 1) != '\0') return -1;
+ return max_idx;
}
#endif
diff --git a/be/src/gutil/sysinfo.h b/be/src/gutil/sysinfo.h
index ec3abe7..d46cfe5 100644
--- a/be/src/gutil/sysinfo.h
+++ b/be/src/gutil/sysinfo.h
@@ -31,6 +31,8 @@
#ifndef _SYSINFO_H_
#define _SYSINFO_H_
+#include <cstdint>
+
namespace base {
// Return the number of online CPUs. This is computed and cached the first time this or
@@ -51,5 +53,17 @@ void SleepForMilliseconds(int64_t milliseconds);
// processor cycles per second of each processor. Thread-safe.
extern double CyclesPerSecond(void);
+// Parse the maximum CPU index from 'str'. The list is in the format of the CPU lists
+// under /sys/devices/system/cpu/, e.g. /sys/devices/system/cpu/present. Returns the
+// index of the max CPU or -1 if the string could not be parsed.
+// Examples of the format and the expected output include:
+// * "0\n" -> 0
+// * "0-8\n" -> 8
+// * "0-15,32-47\n" -> 47
+// * "2,4-127,128-143\n" -> 143
+// Ref: https://www.kernel.org/doc/Documentation/cputopology.txt
+// Exposed for testing.
+extern int ParseMaxCpuIndex(const char* str);
+
} // namespace base
#endif /* #ifndef _SYSINFO_H_ */