On 10/26/21 11:50 AM, Pasha Tatashin wrote: > On Tue, Oct 26, 2021 at 2:45 PM Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote: >> >> On 10/26/21 10:38 AM, Pasha Tatashin wrote: >>> prep_compound_gigantic_page() calls set_page_count(0, p), but it is not >>> needed because page_ref_freeze(p, 1) already sets refcount to 0. >>> >>> Using, set_page_count() is dangerous, because it unconditionally resets >>> refcount from the current value to unrestrained value, and therefore >>> should be minimized. >>> >>> Signed-off-by: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx> >> >> Thanks! >> >> My bad for not removing the set_page_count when adding the page_ref_freeze. >> >> FYI, there have been additional changes to this routine in Andrew's >> tree. Not really sure if we want/need the VM_BUG_ON_PAGE as that would >> only check if there was a 'bug' in page_ref_freeze. > > I would like to keep it. Part of the idea of this series is to reduce > reliance on comments such as: > > /* No worries, refcount is A therefore we can do B */ > > And instead enforce that via VM_BUG_ON(). It should be able to > prevent existing and future _refcount related bugs from manifesting as > memory corruptions. Ok, but that seems a bit redundant to me. There is actually a VM_BUG_ON_PAGE(page_count(p), p) in the 'non-demote' case in Andrew's tree. This is in the path where we do not call page_ref_freeze to zero page ref. That seems sufficient to me. Since you did point out the unnecessary set_page_count, I will submit a code cleanup patch to remove it. I think that is independent of your efforts here, and adding VM_BUG_ON can be discussed in the context of this series. -- Mike Kravetz
