Disable Transparent Hugepage #4174
Conversation
|
🤔 (oh, and also, are these really "stability" issues? All I am aware of is memory usage going overboard) |
|
I cherry-picked the first commit to master because it improves documentation for the release. |
cartoush
force-pushed
the
disable_thp
branch
2 times, most recently
from
22a3a4f
to
d4e100b
Compare
|
Varnish used to crash left and right when Transparent Hugepage first landed, and we sometime still see inexplicable problems go away once we disable the feature. I'm fine with the addition of a jail option to control this, would you agree to disabled by default? |
|
As a matter of principle, I'll be +0 on how linux_jail works and what it does. |
|
bugwash: @cartoush please make this a jail option. disabled by default is preferred, enabled by default would also work for me, as long as there is admin control at all |
cartoush
force-pushed
the
disable_thp
branch
from
d4e100b
to
60fa9b3
Compare
cartoush
force-pushed
the
disable_thp
branch
from
60fa9b3
to
417e24e
Compare
cartoush
force-pushed
the
disable_thp
branch
3 times, most recently
from
0677fd9
to
c6d0f08
Compare
cartoush
force-pushed
the
disable_thp
branch
from
c6d0f08
to
15f10ad
Compare
bin/varnishd/mgt/mgt_jail_linux.c
Outdated
| AN(unix_args); | ||
|
|
||
| for (i = 0; *args != NULL && ret == 0; args++) { | ||
| if (!strncmp(*args, "transparent_hugepage=", 21)) { |
There was a problem hiding this comment.
nit: can we please at least replace the magic 21 with something like
const char const *thpa = "transparent_hugepage=";
const size_t l = strlen(thpa);There was a problem hiding this comment.
FYI this was drawing inspiration from the UNIX jail.
There was a problem hiding this comment.
4001ea2
FTR, in the example above I got one const wrong.
There was a problem hiding this comment.
maybe sizeof("transparent_hugepage=") - 1 would be appropriate and avoid calling strlen just for that ?
There was a problem hiding this comment.
Or sizeof("transparent_hugepage") to swallow the equal sign :p
A call to strlen() for a static string will likely be optimized out at compile time, but if anything I'd rather work with a macro here and in the UNIX jail:
if ((val = ARG_MATCH(*args, transparent_hugepage)) != NULL)
ret = vjl_set_thp(val);Less ceremony, and the macro is in charge of dealing with the equal sign and value offset.
There was a problem hiding this comment.
That was fast, but I only meant to make a suggestion here. I'm interested in @nigoroll's opinion on my suggestion, which would do more than the macro you pushed.
There was a problem hiding this comment.
maybe
sizeof("transparent_hugepage=") - 1would be appropriate and avoid callingstrlenjust for that ?
see 4001ea2: This is constant, so strlen is no more expensive than sizeof and happens at compile or init time. Plus it's irrelevant, because the code runs exactly once.
There was a problem hiding this comment.
I'd rather work with a macro here and in the UNIX jail:
can you show how the macro would look like in vju_init? I thought about it but could not see a nice way cause of the "error if vju_get* fails, else continue"
There was a problem hiding this comment.
Shall I remove my 2nd commit and let this be decided in #4202 ?
Motivated by Dridi in #4174 (comment)
This commit provides a more elegant way to check jail arguments. It was motivated by this comment varnishcache#4174 (comment)
Disabling Transparent Hugepage has often been the solution to solve hard-to-diagnose instability issues and despite improvements in this area compared to the RHEL6 era, our recommandation is still to avoid THP to this day. In addition to refreshing the documentation on this topic, the Linux jail will tentatively disable THP or warn when it cannot.
This commit provides a more elegant way to check jail arguments. It was motivated by this comment varnishcache#4174 (comment)
cartoush
force-pushed
the
disable_thp
branch
from
08c71c7
to
f69c16b
Compare
TODO: find a name and a home for the ARG_MATCH() macro. Refs 4001ea2 Refs varnishcache#4174
No description provided.