lkml.org 
[lkml]   [2021]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC 3/8] mm: Avoid using set_page_count() in set_page_recounted()
From
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

\
 
 \ /
  Last update: 2021-10-28 03:37    [W:0.127 / U:1.612 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site