Re: [PATCH] mm: remove an avoidable load of page refcount in page_ref_add_unless

[Mateusz Guzik <mjguzik@gmail.com>]

On Mon, Dec 9, 2024 at 11:56 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 09.12.24 11:25, Mateusz Guzik wrote:
> > On Mon, Dec 9, 2024 at 10:28 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 07.12.24 09:29, Mateusz Guzik wrote:
> >>> Explicitly pre-checking the count adds nothing as atomic_add_unless
> >>> starts with doing the same thing. iow no functional changes.
> >>
> >> I recall that we added that check because with the hugetlb vmemmap
> >> optimization, some of the tail pages we don't ever expect to be modified
> >>    (because they are fake-duplicated) might be mapped R/O.
> >>
> >> If the arch implementation of atomic_add_unless() would trigger an
> >> unconditional write fault, we'd be in trouble. That would likely only be
> >> the case if the arch provides a dedicate instruction.
> >>
> >> atomic_add_unless()->raw_atomic_add_unless()
> >>
> >> Nobody currently defines arch_atomic_add_unless().
> >>
> >> raw_atomic_fetch_add_unless()->arch_atomic_fetch_add_unless() is defined
> >> on some architectures.
> >>
> >> I scanned some of the inline-asm, and I think most of them perform a
> >> check first.
> >>
> >
> > Huh.
> >
> > Some arch triggering a write fault despite not changing the value is
> > not something I thought about. Sounds pretty broken to me if any arch
> > was to do it, but then stranger things did happen.
>
> Yeah, it really depends on what the architecture defines. For example,
> on s390x for "COMPARE AND SWAP" the spec states something like
>
[snip]

Well in this context you need to do the initial load to even know what
to CAS with, unless you want to blindly do it hoping to get lucky,
which I'm assuming no arch is doing.

Granted, if there was an architecture which had an actual "cas unless
the value is x" then this would not hold, but I don't know of any.
[such an extension would be most welcome fwiw]

Assuming you indeed want the patch after all, can you sort out adding
a comment to atomic_add_unless yourself? ;) I presume you know the
right people and whatnot, so this would cut down on back and forth.

That is to say I think this thread just about exhausted the time
warranted by this patch. No hard feelz if it gets dropped, but then I
do strongly suggest adding a justification to the extra load.

Cheers,
-- 
Mateusz Guzik <mjguzik gmail.com>