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,