On 6/13/21 9:47 PM, Jann Horn wrote:
...
The VM_WARN_ON_ONCE_PAGE() is not implemented exactly right
in the !CONFIG_DEBUG_VM case. IMHO it should follow the WARN*()
behavior, and return the original condition and keep going
in that case.
But the point of the existing definition is that the compiler can
avoid generating code for the condition in !DEBUG_VM builds, even if
it can't prove that the condition is free of side effects, right? If
VM_WARN_ON_ONCE_PAGE() was changed as you propose, then I think that
in mem_cgroup_page_lruvec(), the compiler would have to generate code
for mem_cgroup_disabled(), which calls static_branch_likely(), which
ends up in "asm volatile" statements; so the compiler probably won't
be able to eliminate the condition.
Then you could use it directly here.
Depending on whether the intended behavior here is to skip the check
in !DEBUG_VM builds (which was the case before) or also perform the
check in DEBUG_VM builds. And if DEBUG_VM is a config option because
it might have some performance impact, isn't the cost of the check
probably quite large compared to the cost of printing the warning on a
codpath that should never execute?
That's true for these VM_WARN*() macros, but not true for the more widely
used WARN*() macros. And I was hoping to bring VM macros closer to the
WARN macros. But as you point out, pre-existing callers expect to have
zero impact in !DEBUG_VM builds, and so some caution is required.
I feel like a separate set of macros would be reasonable. Something that
has WARN*() type of behavior, and accepts a struct page (which typically
means that WARN_ON_ONCE is required, because for pages you have to limit
it to that pretty much always).
thanks,
--
John Hubbard
NVIDIA
