Re: [PATCH 2/5] mm: further refactor commit_merge()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, Jan 28, 2025 at 04:45:01PM +0100, Vlastimil Babka wrote:
> On 1/27/25 16:50, Lorenzo Stoakes wrote:
> > --- a/mm/vma.h
> > +++ b/mm/vma.h
> > @@ -67,6 +67,16 @@ enum vma_merge_flags {
> >  	 * at the gap.
> >  	 */
> >  	VMG_FLAG_JUST_EXPAND = 1 << 0,
> > +	/*
> > +	 * Internal flag used during the merge operation to indicate we will
> > +	 * remove vmg->middle.
> > +	 */
> > +	__VMG_FLAG_REMOVE_MIDDLE = 1 << 1,
> > +	/*
> > +	 * Internal flag used during the merge operationr to indicate we will
> > +	 * remove vmg->next.
> > +	 */
> > +	__VMG_FLAG_REMOVE_NEXT = 1 << 2,
> >  };
>
> Hm this is actually kinda weird? It's an enum, but the values of it are
> defined as different bits. And then struct vma_merge_struct has a "enum
> vma_merge_flags merge_flags;" but we don't store to it a single "enum
> vma_merge_flags" value defined above, but a combination of those. Is that
> even legal to do in C?

Yes it's legal to do. And we already did it. And other parts of the kernel do
it.

I get that it breaks a switch (enum val) { case ... } statement but we don't do
that.

>
> AFAIK the more common pattern is enum that has normal incremental values
> that are used for the shifts.
>
> But I don't think we need all of this at all here? Just have bitfields in
> struct vma_merge_struct?
>
> bool just_expand : 1;
> bool remove_middle : 1;

I find that ugly, and it necessitates the addition of a new field for every new
flag.

It also prevents any masking stuff going forward and clutters everything.

It also makes the interface confusiing, because now you have users having to
know there's a field that lets you do X rather than just a simple flags field
that can encapsulate all state.

And some of those fields are now internal...

If you were to insist we have to change this, then I'd pefer a set of defines
and the but then it'd be a question of whether we typedef something for that or
just pass an unsigned long.

I prefer having the type safety of the enum even if it pedantically 'not
correct'.

C doesn't give you many sane choices for this. I am doing my part to make rust
more of a thing in mm which will help on this front ;)

> ...
>
> >  /*
> > diff --git a/tools/testing/vma/vma.c b/tools/testing/vma/vma.c
> > index 3c0572120e94..8cce67237d86 100644
> > --- a/tools/testing/vma/vma.c
> > +++ b/tools/testing/vma/vma.c
> > @@ -154,6 +154,9 @@ static void vmg_set_range(struct vma_merge_struct *vmg, unsigned long start,
> >  	vmg->end = end;
> >  	vmg->pgoff = pgoff;
> >  	vmg->flags = flags;
> > +
> > +	vmg->merge_flags = VMG_FLAG_DEFAULT;
> > +	vmg->target = NULL;
> >  }
> >
> >  /*
>