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

[Vlastimil Babka]

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?

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;
...

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