* 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
> >>
>
