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

[John Hubbard]

On 10/27/21 18:20, John Hubbard wrote:
> > > 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.

...and to clarify a bit more, maybe this also helps:

These patches are attempting to improve debugging, and that is fine, as
far as debugging goes. However, a point that seems to be slightly
misunderstood is: incrementing a bad refcount value is not actually any
better than overwriting it, from a recovery point of view. Maybe (?)
it's better from a debugging point of view.

That's because the problem occurred before this code, and its debug-only
assertions, ran. Once here, the code cannot actually recover: there is
no automatic way to recover from a refcount that it 1, -1, 2, or 706,
when it was supposed to be zero. Incrementing it is, again, not really
necessarily better than setting: setting it might actually make the
broken system appear to run--and in some cases, even avoid symptoms.
Whereas incrementing doesn't cover anything up. The only thing you can
really does is just panic() or BUG(), really.

Don't get me wrong, I don't want bugs covered up. But the claim that
incrementing is somehow better deserves some actual thinking about it.

Overall, I'm inclined to *not* switch anything over to incrementing the
refcounts. Instead, go ahead and:

a) Add assertions up to a "reasonable" point (some others have pointed
out that you don't need quite all of the assertions you've added).

b) Remove set_page_count() calls where possible--maybe everywhere.

c) Fix any bugs found along the way.

d) ...but, leave the basic logic as-is: no changing over to
page_ref_inc_return().

Anyway, that's my take on it.

 thanks,
--
John Hubbard
NVIDIA