On 1/27/25 16:50, Lorenzo Stoakes wrote: > The adj_start calculation has been a constant source of confusion in the > VMA merge code. > > There are two cases to consider, one where we adjust the start of the > vmg->middle VMA (i.e. the __VMG_FLAG_ADJUST_MIDDLE_START merge flag is > set), in which case adj_start is calculated as: > > (1) adj_start = vmg->end - vmg->middle->vm_start > > And the case where we adjust the start of the vmg->next VMA (i.e.t he > __VMG_FLAG_ADJUST_NEXT_START merge flag is set), in which case adj_start is > calculated as: > > (2) adj_start = -(vmg->middle->vm_end - vmg->end) > > We apply (1) thusly: > > vmg->middle->vm_start = > vmg->middle->vm_start + vmg->end - vmg->middle->vm_start > > Which simplifies to: > > vmg->middle->vm_start = vmg->end > > Similarly, we apply (2) as: > > vmg->next->vm_start = > vmg->next->vm_start + -(vmg->middle->vm_end - vmg->end) > > Noting that for these VMAs to be mergeable vmg->middle->vm_end == > vmg->next->vm_start and so this simplifies to: > > vmg->next->vm_start = > vmg->next->vm_start + -(vmg->next->vm_start - vmg->end) > > Which simplifies to: > > vmg->next->vm_start = vmg->end > > Therefore in each case, we simply need to adjust the start of the VMA to > vmg->end (!) and can do away with this adj_start calculation. The only > caveat is that we must ensure we update the vm_pgoff field correctly. > > We therefore abstract this entire calculation to a new function > vmg_adjust_set_range() which performs this calculation and sets the > adjusted VMA's new range using the general vma_set_range() function. > > We also must update vma_adjust_trans_huge() which expects the > now-abstracted adj_start parameter. It turns out this is wholly > unnecessary. > > In vma_adjust_trans_huge() the relevant code is: > > if (adjust_next > 0) { > struct vm_area_struct *next = find_vma(vma->vm_mm, vma->vm_end); > unsigned long nstart = next->vm_start; > nstart += adjust_next; > split_huge_pmd_if_needed(next, nstart); > } > > The only case where this is relevant is when __VMG_FLAG_ADJUST_MIDDLE_START > is specified (in which case adj_next would have been positive), i.e. the > one in which the vma specified is vmg->prev and this the sought 'next' VMA > would be vmg->middle. > > We can therefore eliminate the find_vma() invocation altogether and simply > provide the vmg->middle VMA in this instance, or NULL otherwise. > > Again we have an adj_next offset calculation: > > next->vm_start + vmg->end - vmg->middle->vm_start > > Where next == vmg->middle this simplifies to vmg->end as previously > demonstrated. > > Therefore nstart is equal to vmg->end, which is already passed to > vma_adjust_trans_huge() via the 'end' parameter and so this code (rather > delightfully) simplifies to: > > if (next) > split_huge_pmd_if_needed(next, end); > > With these changes in place, it becomes silly for commit_merge() to return > vmg->target, as it is always the same and threaded through vmg, so we > finally change commit_merge() to return an error value once again. > > This patch has no change in functional behaviour. > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> Yeah this makes the preparations worth it. Nice! Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx> > +/* > + * Actually perform the VMA merge operation. > + * > + * On success, returns the merged VMA. Otherwise returns NULL. Needs updating? > + */ > +static int commit_merge(struct vma_merge_struct *vmg) > +{ > + struct vm_area_struct *vma; > + struct vma_prepare vp; > + bool adj_middle = vmg->merge_flags & __VMG_FLAG_ADJUST_MIDDLE_START; > > - vma_iter_config(vmg->vmi, vmg->next->vm_start + adj_start, > - vmg->next->vm_end); > + if (vmg->merge_flags & __VMG_FLAG_ADJUST_NEXT_START) { > + /* In this case we manipulate middle and return next. */ Also we don't return next anymore? At least not here. vma_merge_existing_range() does, but here it's rather "the target is next"? > + vma = vmg->middle; > + vma_iter_config(vmg->vmi, vmg->end, vmg->next->vm_end); > } else {