Re: [PATCH v2 3/5] mm: implement sticky, copy on fork VMA flags

[Vlastimil Babka <vbabka@suse.cz>]

On 11/6/25 15:18, Lorenzo Stoakes wrote:
> On Thu, Nov 06, 2025 at 02:46:38PM +0100, Vlastimil Babka wrote:
>> On 11/6/25 11:46, Lorenzo Stoakes wrote:
>> > diff --git a/include/linux/mm.h b/include/linux/mm.h
>> > index 2ea65c646212..4d80eaf4ef3b 100644
>> > --- a/include/linux/mm.h
>> > +++ b/include/linux/mm.h
>> > @@ -527,6 +527,38 @@ extern unsigned int kobjsize(const void *objp);
>> >  #endif
>> >  #define VM_FLAGS_CLEAR	(ARCH_VM_PKEY_FLAGS | VM_ARCH_CLEAR)
>> >
>> > +/* Flags which should result in page tables being copied on fork. */
>> > +#define VM_COPY_ON_FORK VM_MAYBE_GUARD
>> > +
>> > +/*
>> > + * Flags which should be 'sticky' on merge - that is, flags which, when one VMA
>> > + * possesses it but the other does not, the merged VMA should nonetheless have
>> > + * applied to it:
>> > + *
>> > + * VM_COPY_ON_FORK - These flags indicates that a VMA maps a range that contains
>> > + *                   metadata which should be unconditionally propagated upon
>> > + *                   fork. When merging two VMAs, we encapsulate this range in
>> > + *                   the merged VMA, so the flag should be 'sticky' as a result.
>> > + */
>> > +#define VM_STICKY VM_COPY_ON_FORK
>>
>> TBH I don't see why there should be always an implication that copying on
>> fork implies stickiness in merging. Yeah, VM_MAYBE_GUARD is both, but in
>> general, is there any underlying property that makes this a rule?
> 
> Why do you copy on fork? It's because the page tables contain data that won't be
> reconstructed on fault.
> 
> If that is the case, that applies to any VMA which is merged, and also - since
> you can't be sure precisely which page tables contain the data we need to
> propagate - on split too.
> 
> This is why copy on fork implies sticky IMO.

Hmm I guess that makes some sense.
> I can update the commit message to make this clear if this makes sense?

It would help, thanks. Let's see if future will surprise us with some flag
where this won't be true :)

>>
>> > +/*
>> > + * VMA flags we ignore for the purposes of merge, i.e. one VMA possessing one
>> > + * of these flags and the other not does not preclude a merge.
>> > + *
>> > + * VM_SOFTDIRTY - Should not prevent from VMA merging, if we match the flags but
>> > + *                dirty bit -- the caller should mark merged VMA as dirty. If
>> > + *                dirty bit won't be excluded from comparison, we increase
>> > + *                pressure on the memory system forcing the kernel to generate
>> > + *                new VMAs when old one could be extended instead.
> 
> Note that I'm literally just moving the comment from is_mergeable_vma():
> 
> -	 * VM_SOFTDIRTY should not prevent from VMA merging, if we
> -	 * match the flags but dirty bit -- the caller should mark
> -	 * merged VMA as dirty. If dirty bit won't be excluded from
> -	 * comparison, we increase pressure on the memory system forcing
> -	 * the kernel to generate new VMAs when old one could be
> -	 * extended instead.
> 
> (OK I see you realised that below :P)
> 
>>
>> So I wonder if VM_SOFTDIRTY should be actually also sticky and not just
>> VM_IGNORE_MERGE. The way I understand the flag suggests it should.
>> Right now AFAICS its rather undefined if the result of vma merge has the
>> flag - depending on which of the two VMA's stays and which is removed by the
>> merge. "the caller should mark merged VMA as dirty" in the comment you're
>> moving here seems not really happening or I'm missing it. __mmap_complete()
> 
> No it's not happening, but I can't be blamed for existing incorrect comments :)
> 
>> and do_brk_flags() do it, so any new areas are marked, but on pure merge of
>> two vma's due to e.g. mprotect() this is really nondetermintic? AFAICT the
>> sticky flag behavior would work perfectly for VM_SOFTDIRTY.
> 
> Maybe we inavertantly changed this somehow or maybe it was just wrong, but we're
> not doing this on merge in general afaict.

Yeah wouldn't surprised me if we subtly changed it during some refactoring
and it's not causing such obvious issues to be noticed easily.

> I think you're right that we should make this sticky, but I'd rather deal with
> that in a follow-up series/patch as this is out of scope here.
> 
> Equally so I'd rather fix the comment in a follow up too for the same reason.

Sure it's just something I noticed and seems like a good fit for the new
concept.