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

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

On Tue, Jan 28, 2025 at 04:07:00PM +0000, Lorenzo Stoakes wrote:
> 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 ;)
>

Actually, looking more closely, this is not a common pattern and the weirdness
you say is confusing.

The alternative of a bare flags field sucks badly, so while I dislike the
aesthetics of the bitfields, the fact you can't mask, the fact it's not a
clean 'state parameter' now, it's probably marginally better overall
vs. alternatives.

C doesn't help you here very much... some other languages have a concept of
a 'flags enum' or at least a specifically-typed value that can be used this
way.

We can add comments to make this less sucky like:

/* Flags which callers can use to modify merge behaviour */
bool just_expand :1;
/* Internal flags set during merge process */
bool __blahdy_blah :1;

TL;DR - will convert to bitfields, you're right I'm wrong, beer + pizzas in
Prague soon! ;)


> > ...
> >
> > >  /*
> > > 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;
> > >  }
> > >
> > >  /*
> >
>