On Wed, Aug 05, 2026 at 11:19:20AM +0200, David Hildenbrand (Arm) wrote:
> On 8/5/26 10:59, Lorenzo Stoakes (ARM) wrote:
> > On Wed, Aug 05, 2026 at 09:35:56AM +0200, David Hildenbrand (Arm) wrote:
> >> On 8/3/26 15:46, Lorenzo Stoakes (ARM) wrote:
> >>>
> >>> No this would be incorrect.
> >>>
> >>> A read-only mapping would become unmergeable here. So this is something
> >>> apart
> >>> from the rmap aspect,
> >>
> >> I'd assume that we should never even consider anon_pgoff when merging
> >> !is_cow_mapping(), it doesn't make any sense.
> >>
> >> No anon folios -> no anon_vma -> no anon_pgoff
> >
> > You can merge unfaulted ranges is the thing here.
> >
> > But anyway I actually wonder whether this whole branch shouldn't be:
> >
> > if (!vma->anon_vma) {
> > ...
> > }
> >
> > Because that way we keep anon_pgoff updated even for MAP_SHARED mappings.
> > This
> > isn't necessary and doesn't impact anything _except_ print_bad_page_map
> > which
> > outputs both pgoffs.
>
> Agreed.
>
> >
> > But it'd be consistent, avoid any confusion about gating on VMA_SHARED, and
> > simplify the code :)
> >
> >>
> >> But I think I am missing one detail here:
> >>
> >>> and it is a contract that upon move of an unfaulted
> >>> mapping (which for read-only anon would always be unfaulted) that
> >>> vma->vm_pgoff
> >>> is updated.
> >>
> >> "read-only anon": I assume you mean an anon mapping that does not have
> >> VM_MAYWRITE set?
> >
> > A MAP_SHARED mapping of a read-only file becomes a MAP_PRIVATE
> > !VMA_MAYWRITE_BIT
> > mapping and must adhere to the same contract.
>
> Right, but that is not an anon mapping, it's a file mapping that similarly
> cannot have anon folios, ever.
>
> >
> > Also mmap hooks can clear the VMA_MAYWRITE_BIT.
>
> Right, but again, if we'd have that being done to anon mappings, other things
> in
> MM would already be broken.
>
> We assume that anon folios can only ever end up in cow mappings.
>
> >
> > However:
> >
> > - If you're a driver clearing VMA_MAYWRITE_BIT you should only be doing
> > this for
> > 'special' mappings anyway (I have a series I've not sent yet that
> > establishes
> > this as an invariant also) - and these are not mergeable anyway.
>
> Jup.
>
> >
> > - If you're a !VMA_MAYWRITE_BIT MAP_PRIVATE-file backed mappings you never
> > set
> > vma->anon_vma and always update anon pgoff so you always have alignment
> > for
> > purposes of merge.
>
> Jup.
>
> >
> > So I think also we can then change needs_adjacent_anon_pgoff() to:
> >
> > static bool needs_adjacent_anon_pgoff(const struct vma_merge_struct *vmg)
> > {
> > return vmg->file && is_cow_mapping(...);
> > }
>
> Agreed.
>
> >
> > [I have to create a vma_flags_t variant of is_cow_mapping()]
> >
> > With those two changes we gate on VMA_SHARED_BIT nowhere :)
>
> That's much clearer.
Yes agreed :)
>
> I was thinking for a second whether to have a more expressive
> "mapping_might_have_anon_folio" or sth like that. But it's a bit mouthful.
>
> Most instances of is_cow_mapping() in memory.c want to know exactly that.
Yeah and obviously once scalable CoW is in even that name will be wrong :)
I think the semantics are a non-obvious but I do like that we are explicit in
saying 'is a CoW mapping'. We just maybe need to be more consistent with that.
Maybe once scalable CoW is in can work to improve consistency of that...
>
> >
> >>
> >> I recall that that's a combination that cannot be created. While you can
> >> create
> >> something that does not have VM_WRITE set, IIRC VM_MAYWRITE is always set
> >> for
> >> anon vmas.
> >
> > For pure anon yeah, see above for the MAP_SHARED->MAP_PRIVATE-file backed
> > weird
> > case.
> >
>
> Agreed.
Cool will make the changes as above then! :)
>
> >>
> >> --
> >> Cheers,
> >>
> >> David
> >
> > (It's funny to me that if you want a truly read-only MAP_PRIVATE file-backed
> > mapping (no idea why you would but anyway) you have to MAP_SHARED, but an
> > actually MAP_PRIVATE file-backed mapping of a read-only file is writable
> > [which
> > makes sense obviously] :)
> I guess this boils down to
>
> MAP_PRIVATE of a read-only file allows you to COW. Which is usually what you
> want when placing breakpoints / letting the debugger go wild.
>
> MAP_SHARED of a read-only file doesn't allow you to COW, and can consequently
> never become writable.
>
> It's confusing, yes.
Yeah indeed, it's just amusing to think about, but in practice makes perfect
sense.
Though honestly I wish we didn't remove VMA_SHARED_BIT in this situation and
then rely on the hugely-confusingly-named VMA_MAY_SHARE_BIT for stuff where we
_really_ need to figure out whether it was mapped shared.
But that's another thing :)
>
> --
> Cheers,
>
> David
--
Cheers, Lorenzo
