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.
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.
>
>>
>> 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.
>>
>> --
>> 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.
--
Cheers,
David
