Re: [RFC 3/8] mm: Avoid using set_page_count() in set_page_recounted()

[John Hubbard]

On 11/1/21 07:22, Pasha Tatashin wrote:
> > > > Yes, you are just repeating what the diffs say.
> > > >
> > > > But it's still not good to have this function name doing something completely
> > > > different than its name indicates.
> > >
> > > I see, I can rename it to: 'set_page_recounted/get_page_recounted' ?
> >
> > What? No, that's not where I was going at all. The function is already
> > named set_page_refcounted(), and one of the problems I see is that your
> > changes turn it into something that most certainly does not
> > set_page_refounted(). Instead, this patch *increments* the refcount.
> > That is not the same thing.
> >
> > And then it uses a .config-sensitive assertion to "prevent" problems.
> > And by that I mean, the wording throughout this series seems to equate
> > VM_BUG_ON_PAGE() assertions with real assertions. They are only active,
> > however, in CONFIG_DEBUG_VM configurations, and provide no protection at
> > all for normal (most distros) users. That's something that the wording,
> > comments, and even design should be tweaked to account for.
>
> VM_BUG_ON and BUG_ON should be treated the same. Yes, they are config
> sensitive, but in both cases *BUG_ON() means that there is an
> unrecoverable problem that occured. The only difference between the
> two is that VM_BUG_ON() is not enabled when distros decide to reduce
> the size of their kernel and improve runtime performance by skipping
> some extra checking.
>
> There is no logical separation between VM_BUG_ON and BUG_ON, there is
> been a lengthy discussion about this:

Actually I do want to mention one more thing about this, before we move
on to the next version of the patchset. The above is inaccurate. The
intent of VM_BUG_ON() and BUG_ON() is similar, but there is *definitely*
a logical separation between them: they do not behave the same at runtime.

Just because some distros enable VM_BUG_ON(), does not mean that we can
treat it the same as BUG_ON() in "both directions". From a "don't BUG()
crash the kernel unless really warranted", they are about the same, as
Linus keeps repeating. From the other direction, though ("I need to BUG()-
crash the kernel"), they are NOT the same. BUG_ON() is more reliably
available.

And that's the essence of my object to treating this as if you have
reliably stopped the kernel with a VM_BUG_ON(). It's not really the same!


thanks,
--
John Hubbard
NVIDIA