Messages in this thread | | | Date | Wed, 27 Oct 2021 18:35:44 -0700 | Subject | Re: [RFC 3/8] mm: Avoid using set_page_count() in set_page_recounted() | From | 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
| |