On Sat, Jun 12, 2021 at 12:17 PM John Hubbard jhubbard@nvidia.com wrote:
On 6/11/21 3:49 PM, Jann Horn wrote:
On Sat, Jun 12, 2021 at 12:36 AM Andrew Morton akpm@linux-foundation.org wrote:
On Fri, 11 Jun 2021 18:15:45 +0200 Jann Horn jannh@google.com wrote:
+/* Equivalent to calling put_page() @refs times. */ +static void put_page_refs(struct page *page, int refs) +{
VM_BUG_ON_PAGE(page_ref_count(page) < refs, page);I don't think there's a need to nuke the whole kernel in this case. Can we warn then simply leak the page? That way we have a much better chance of getting a good bug report.
Ah, yeah, I guess that makes sense. I had just copied this over from put_compound_head(), and figured it was fine to keep it as-is, but I guess changing it would be reasonable. I'm not quite sure what the best way to do that would be though.
I guess the check should go away in !DEBUG_VM builds?
Should I just explicitly put the check in an ifdef block? Like so:
#ifdef CONFIG_DEBUG_VM if (VM_WARN_ON_ONCE_PAGE(...)) return; #endif
Or, since inline ifdeffery looks ugly, get rid of the explicit ifdef,
Agreed: VM_WARN_ON_ONCE_PAGE(), at least at the API level, seems like the best thing to use here. However, as you point out below, it needs a little something first.
and change the !DEBUG_VM definition of VM_WARN_ON_ONCE_PAGE() as follows so that the branch is compiled away?
#define VM_WARN_ON_ONCE_PAGE(cond, page) (BUILD_BUG_ON_INVALID(cond), false)
That would look kinda neat, but it would be different from the behavior of WARN_ON(), which still returns the original condition even in !BUG builds, so that could be confusing...
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?