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
