You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "cmcfarlen (via GitHub)" <gi...@apache.org> on 2023/03/01 16:33:00 UTC
[GitHub] [trafficserver] cmcfarlen opened a new pull request, #9479: Hugepage config cleanup
cmcfarlen opened a new pull request, #9479:
URL: https://github.com/apache/trafficserver/pull/9479
This removes the following settings:
- proxy.config.cache.dir.enable_hugepages
- proxy.config.allocator.iobuf_use_hugepages
Instead, these features will just use the `proxy.config.allocator.hugepages` setting. This is to reduce the config required to get huge pages going.
This PR also adds back in the hopefully more user friendly parsing of the iobuffer buckets. This is related because this was part of the prior hugepage work but was left out because libswoc was not linked to the binaries in the build system.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] cmcfarlen commented on a diff in pull request #9479: Hugepage config cleanup
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #9479:
URL: https://github.com/apache/trafficserver/pull/9479#discussion_r1122092165
##########
iocore/eventsystem/IOBuffer.cc:
##########
@@ -73,6 +74,80 @@ init_buffer_allocators(int iobuffer_advice)
init_buffer_allocators(iobuffer_advice, chunk_sizes, false);
}
+auto
+make_buffer_size_parser()
+{
+ return [l = swoc::Lexicon<int>{
+ {{0, {"128"}},
+ {1, {"256"}},
+ {2, {"512"}},
+ {3, {"1k", "1024"}},
+ {4, {"2k", "2048"}},
+ {5, {"4k", "4096"}},
+ {6, {"8k", "8192"}},
+ {7, {"16k"}},
+ {8, {"32k"}},
+ {9, {"64k"}},
+ {10, {"128k"}},
+ {11, {"256k"}},
+ {12, {"512k"}},
+ {13, {"1M", "1024k"}},
+ {14, {"2M", "2048k"}}},
+ -1
+ }](ts::TextView esize) -> std::optional<int> {
+ int result = l[esize];
+ if (result == -1) {
+ return std::nullopt;
+ }
+ return result;
+ };
+}
+
+bool
+parse_buffer_chunk_sizes(const char *chunk_sizes_string, int chunk_sizes[DEFAULT_BUFFER_SIZES])
+{
+ const ts::TextView delimiters(", ");
+ if (chunk_sizes_string != nullptr) {
+ ts::TextView src(chunk_sizes_string, ts::TextView::npos);
+ auto parser = make_buffer_size_parser();
+ int n = 0;
+ while (!src.empty()) {
+ ts::TextView token{src.take_prefix_at(delimiters)};
+
+ ts::TextView esize = token.take_prefix_at(':');
+ if (!token.empty()) {
+ auto parsed = parser(esize);
+ if (parsed) {
+ n = *parsed;
+ } else {
+ Error("Failed to parse size for %.*s", static_cast<int>(esize.size()), esize.data());
+ return false;
+ }
+ } else {
+ // element didn't have a colon so its just a chunk size
+ token = esize;
+ }
+
+ // Check if n goes out of bounds
+ if (n >= DEFAULT_BUFFER_SIZES) {
+ Error("Invalid IO buffer chunk sizes string");
+ return false;
+ }
+
+ auto x = ts::svto_radix<10>(token);
+ if (token.empty() && x <= std::numeric_limits<int>::max()) {
Review Comment:
I do wish `svto_radix` returned `std::optional` or something!
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #9479: Hugepage config cleanup
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #9479:
URL: https://github.com/apache/trafficserver/pull/9479#discussion_r1122079847
##########
iocore/eventsystem/IOBuffer.cc:
##########
@@ -73,6 +74,80 @@ init_buffer_allocators(int iobuffer_advice)
init_buffer_allocators(iobuffer_advice, chunk_sizes, false);
}
+auto
+make_buffer_size_parser()
+{
+ return [l = swoc::Lexicon<int>{
+ {{0, {"128"}},
+ {1, {"256"}},
+ {2, {"512"}},
+ {3, {"1k", "1024"}},
+ {4, {"2k", "2048"}},
+ {5, {"4k", "4096"}},
+ {6, {"8k", "8192"}},
+ {7, {"16k"}},
+ {8, {"32k"}},
+ {9, {"64k"}},
+ {10, {"128k"}},
+ {11, {"256k"}},
+ {12, {"512k"}},
+ {13, {"1M", "1024k"}},
+ {14, {"2M", "2048k"}}},
+ -1
+ }](ts::TextView esize) -> std::optional<int> {
+ int result = l[esize];
+ if (result == -1) {
+ return std::nullopt;
+ }
+ return result;
+ };
+}
+
+bool
+parse_buffer_chunk_sizes(const char *chunk_sizes_string, int chunk_sizes[DEFAULT_BUFFER_SIZES])
+{
+ const ts::TextView delimiters(", ");
+ if (chunk_sizes_string != nullptr) {
+ ts::TextView src(chunk_sizes_string, ts::TextView::npos);
+ auto parser = make_buffer_size_parser();
+ int n = 0;
+ while (!src.empty()) {
+ ts::TextView token{src.take_prefix_at(delimiters)};
+
+ ts::TextView esize = token.take_prefix_at(':');
+ if (!token.empty()) {
+ auto parsed = parser(esize);
+ if (parsed) {
+ n = *parsed;
+ } else {
+ Error("Failed to parse size for %.*s", static_cast<int>(esize.size()), esize.data());
+ return false;
+ }
+ } else {
+ // element didn't have a colon so its just a chunk size
+ token = esize;
+ }
+
+ // Check if n goes out of bounds
+ if (n >= DEFAULT_BUFFER_SIZES) {
+ Error("Invalid IO buffer chunk sizes string");
+ return false;
+ }
+
+ auto x = ts::svto_radix<10>(token);
+ if (token.empty() && x <= std::numeric_limits<int>::max()) {
Review Comment:
This may break in ubuntu ` x <= std::numeric_limits<int>::max()`, you fixed this already ;)
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] cmcfarlen commented on a diff in pull request #9479: Hugepage config cleanup
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #9479:
URL: https://github.com/apache/trafficserver/pull/9479#discussion_r1134245846
##########
iocore/eventsystem/EventSystem.cc:
##########
@@ -45,24 +45,12 @@ ink_event_system_init(ts::ModuleVersion v)
int chunk_sizes[DEFAULT_BUFFER_SIZES] = {0};
char *chunk_sizes_string = REC_ConfigReadString("proxy.config.allocator.iobuf_chunk_sizes");
- if (chunk_sizes_string != nullptr) {
- ts::TextView src(chunk_sizes_string, ts::TextView::npos);
- int n = 0;
- while (n < DEFAULT_BUFFER_SIZES && !src.empty()) {
- ts::TextView token{src.take_prefix_at(' ')};
- auto x = ts::svto_radix<10>(token);
- if (token.empty() && x != std::numeric_limits<decltype(x)>::max()) {
- chunk_sizes[n++] = x;
- } else {
- break;
- }
- }
- ats_free(chunk_sizes_string);
+ if (chunk_sizes_string && !parse_buffer_chunk_sizes(chunk_sizes_string, chunk_sizes)) {
+ // If we can't parse the string then we can't be sure of the chunk sizes so just exit
+ ::exit(1);
Review Comment:
Great question! Fixed
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] cmcfarlen commented on a diff in pull request #9479: Hugepage config cleanup
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #9479:
URL: https://github.com/apache/trafficserver/pull/9479#discussion_r1122090641
##########
iocore/eventsystem/IOBuffer.cc:
##########
@@ -73,6 +74,80 @@ init_buffer_allocators(int iobuffer_advice)
init_buffer_allocators(iobuffer_advice, chunk_sizes, false);
}
+auto
+make_buffer_size_parser()
+{
+ return [l = swoc::Lexicon<int>{
+ {{0, {"128"}},
+ {1, {"256"}},
+ {2, {"512"}},
+ {3, {"1k", "1024"}},
+ {4, {"2k", "2048"}},
+ {5, {"4k", "4096"}},
+ {6, {"8k", "8192"}},
+ {7, {"16k"}},
+ {8, {"32k"}},
+ {9, {"64k"}},
+ {10, {"128k"}},
+ {11, {"256k"}},
+ {12, {"512k"}},
+ {13, {"1M", "1024k"}},
+ {14, {"2M", "2048k"}}},
+ -1
+ }](ts::TextView esize) -> std::optional<int> {
+ int result = l[esize];
+ if (result == -1) {
+ return std::nullopt;
+ }
+ return result;
+ };
+}
+
+bool
+parse_buffer_chunk_sizes(const char *chunk_sizes_string, int chunk_sizes[DEFAULT_BUFFER_SIZES])
+{
+ const ts::TextView delimiters(", ");
+ if (chunk_sizes_string != nullptr) {
+ ts::TextView src(chunk_sizes_string, ts::TextView::npos);
+ auto parser = make_buffer_size_parser();
+ int n = 0;
+ while (!src.empty()) {
+ ts::TextView token{src.take_prefix_at(delimiters)};
+
+ ts::TextView esize = token.take_prefix_at(':');
+ if (!token.empty()) {
+ auto parsed = parser(esize);
+ if (parsed) {
+ n = *parsed;
+ } else {
+ Error("Failed to parse size for %.*s", static_cast<int>(esize.size()), esize.data());
+ return false;
+ }
+ } else {
+ // element didn't have a colon so its just a chunk size
+ token = esize;
+ }
+
+ // Check if n goes out of bounds
+ if (n >= DEFAULT_BUFFER_SIZES) {
+ Error("Invalid IO buffer chunk sizes string");
+ return false;
+ }
+
+ auto x = ts::svto_radix<10>(token);
+ if (token.empty() && x <= std::numeric_limits<int>::max()) {
Review Comment:
oops, unfixed! Thanks!
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] cmcfarlen commented on pull request #9479: Hugepage config cleanup
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #9479:
URL: https://github.com/apache/trafficserver/pull/9479#issuecomment-1450945302
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9479: Hugepage config cleanup
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9479:
URL: https://github.com/apache/trafficserver/pull/9479#discussion_r1130299368
##########
iocore/eventsystem/EventSystem.cc:
##########
@@ -45,24 +45,12 @@ ink_event_system_init(ts::ModuleVersion v)
int chunk_sizes[DEFAULT_BUFFER_SIZES] = {0};
char *chunk_sizes_string = REC_ConfigReadString("proxy.config.allocator.iobuf_chunk_sizes");
- if (chunk_sizes_string != nullptr) {
- ts::TextView src(chunk_sizes_string, ts::TextView::npos);
- int n = 0;
- while (n < DEFAULT_BUFFER_SIZES && !src.empty()) {
- ts::TextView token{src.take_prefix_at(' ')};
- auto x = ts::svto_radix<10>(token);
- if (token.empty() && x != std::numeric_limits<decltype(x)>::max()) {
- chunk_sizes[n++] = x;
- } else {
- break;
- }
- }
- ats_free(chunk_sizes_string);
+ if (chunk_sizes_string && !parse_buffer_chunk_sizes(chunk_sizes_string, chunk_sizes)) {
+ // If we can't parse the string then we can't be sure of the chunk sizes so just exit
+ ::exit(1);
Review Comment:
Why aren't you using `Fatal()` from `Diags.h`?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] cmcfarlen merged pull request #9479: Hugepage config cleanup
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen merged PR #9479:
URL: https://github.com/apache/trafficserver/pull/9479
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org