Re: [PATCH] mm/page_isolation: don't putback unisolated page

[David Hildenbrand]

On 09.09.21 11:07, Vlastimil Babka wrote:
> On 9/9/21 10:56, David Hildenbrand wrote:
> > On 09.09.21 00:42, John Hubbard wrote:
> > > On 9/7/21 2:56 AM, David Hildenbrand wrote:
> > > ...
> > > > > If this can be handled gracefully, then I'd rather go with VM_WARN_ON.
> > > > > Maybe even WARN_ON_ONCE?
> > > >
> > > > I think either VM_BUG_ON() or VM_WARN_ON() -- compiling the runtime
> > > > checks out -- should be good
> > > > enough.
> > > >
> > > > I'd just go with VM_BUG_ON(), because anybody messing with
> > > > __isolate_free_page() should clearly spot
> > > > that we expect the current handling. But no strong opinion.
> > >
> > > If in doubt, WARN*() should be preferred over BUG*(). There's a pretty long
> > > history of "don't kill the machine unless you have to" emails about this, let
> > > me dig up one...OK, maybe not the best example, but the tip of the iceberg:
> >
> > Please note the subtle difference between BUG_ON and VM_BUG_ON. We expect
> > VM_BUG_ON to be compiled out on any production system. So it's really only a
> > mean to identify things that really shouldn't be like that during
> > debugging/testing.
>
> IIRC Fedora used to have CONFIG_DEBUG_VM enabled, did it change?

Excellent question. Apparently you are right. Fortunately it's not a 
distro to use in production ;)

In kernel-ark:

redhat/configs/fedora/generic/CONFIG_DEBUG_VM:CONFIG_DEBUG_VM=y

While for ARK (rhel-next so to say)

redhat/configs/ark/generic/CONFIG_DEBUG_VM:# CONFIG_DEBUG_VM is not set

So yes, the VM_WARN_ON would then be preferred in that case. But it's 
something that should never ever happen unless reviewers and developers 
really mess up, so I don't actually would sleep over that. We have other 
WARN... that can trigger more easily.

> > Using WARN... instead of VM_BUG_ON is even worse for production systems.
> > There are distros that set panic_on_warn, essentially converting WARN...
> > into BUG...
>
> Uh, does any distro really do that?

Apparently, so I was told by Greg a year ago or so when wanting to add 
WARN_ON(). The advisory is to us pr_warn_once() instead. I rememebr it 
was a debian based distro.



--
Thanks,

David / dhildenb