Re: [RFC PATCH 4/4] [PATCH 4/4] mm: add tracing for VMA merges

[Liam Howlett]

* Vlastimil Babka <vbabka@xxxxxxx> [220225 05:39]:
> On 2/18/22 20:57, Liam Howlett wrote:
> >>  	/*
> >>  	 * Can we merge both predecessor and successor?
> >>  	 */
> >> -	if (merge_prev && merge_next)
> >> +	if (merge_prev >= MERGE_OK && merge_next >= MERGE_OK) {
> > 
> > What happened to making vma_merge easier to read?  What does > MERGE_OK
> > mean?  I guess this answers why booleans were not used, but I don't like
> 
> It's similar to e.g. enum compact_priority where specific values are defined
> as well as more abstract aliases.
> 
> > it.   Couldn't you just log the err/success and the value of
> > merge_prev/merge_next?  It's not like the code tries more than one way
> > of merging on failure..
> 
> An initial version had the "log" (trace point really) at multiple places and
> it was uglier than collecting details in the variables and having a single
> tracepoint call site.

Wouldn't the success/failure, merge_prev, and merge_next be all we need
to know what happened?  This could be places near the end of the
function.  It doesn't say why merge_prev or merge_next was set to false
but I think that's enough for most uses?

> 
> Note that the tracepoint is being provided as part of the series mainly to
> allow evaluation of the series. If it's deemed too specific to be included
> in mainline in the end, so be it.

Ah, in that case use bool until we arrive at this patch, then change
merge_* to int.

> 
> >>  		merge_both = is_mergeable_anon_vma(prev->anon_vma, next->anon_vma, NULL);
> >> +	}
> >>  
> >> -	if (merge_both) {	 /* cases 1, 6 */
> >> +	if (merge_both >= MERGE_OK) {	 /* cases 1, 6 */
> >>  		err = __vma_adjust(prev, prev->vm_start,
> >>  					next->vm_end, prev->vm_pgoff, NULL,
> >>  					prev);
> >>  		area = prev;
> >> -	} else if (merge_prev) {			/* cases 2, 5, 7 */
> >> +	} else if (merge_prev >= MERGE_OK) {			/* cases 2, 5, 7 */
> >>  		err = __vma_adjust(prev, prev->vm_start,
> >>  					end, prev->vm_pgoff, NULL, prev);
> >>  		area = prev;
> >> -	} else if (merge_next) {
> >> +	} else if (merge_next >= MERGE_OK) {
> >>  		if (prev && addr < prev->vm_end)	/* case 4 */
> >>  			err = __vma_adjust(prev, prev->vm_start,
> >>  					addr, prev->vm_pgoff, NULL, next);
> >> @@ -1252,7 +1255,7 @@ struct vm_area_struct *vma_merge(struct mm_struct *mm,
> >>  	} else {
> >>  		err = -1;
> >>  	}
> >> -
> >> +	trace_vm_av_merge(err, merge_prev, merge_next, merge_both);
> >>  	/*
> >>  	 * Cannot merge with predecessor or successor or error in __vma_adjust?
> >>  	 */
> >> @@ -3359,6 +3362,8 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
> >>  		/*
> >>  		 * Source vma may have been merged into new_vma
> >>  		 */
> >> +		trace_vm_pgoff_merge(vma, anon_pgoff_updated);
> >> +
> >>  		if (unlikely(vma_start >= new_vma->vm_start &&
> >>  			     vma_start < new_vma->vm_end)) {
> >>  			/*
> >> -- 
> >> 2.34.1
> >> 
>