[0/4] further cleanup of vma_merge()

Message

Lorenzo Stoakes March 18, 2023, 11:13 a.m. UTC

Following on from Vlastimil Babka's patch series "cleanup vma_merge() and
improve mergeability tests" which was in turn based on Liam's prior
cleanups, this patch series introduces changes discussed in review of
Vlastimil's series and goes further in attempting to make the logic as
clear as possible.

Nearly all of this should have absolutely no functional impact, however it
does add a singular VM_WARN_ON() case.

Lorenzo Stoakes (4):
  mm/mmap/vma_merge: further improve prev/next VMA naming
  mm/mmap/vma_merge: set next to NULL if not applicable
  mm/mmap/vma_merge: extend invariants, avoid invalid res, vma
  mm/mmap/vma_merge: be explicit about the non-mergeable case

 mm/mmap.c | 165 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 107 insertions(+), 58 deletions(-)

--
2.39.2

Comments

Liam R. Howlett March 20, 2023, 4:47 p.m. UTC | #1

* Lorenzo Stoakes <[e-mail]> [230318 07:15]:
> Following on from Vlastimil Babka's patch series "cleanup vma_merge() and
> improve mergeability tests" which was in turn based on Liam's prior
> cleanups, this patch series introduces changes discussed in review of
> Vlastimil's series and goes further in attempting to make the logic as
> clear as possible.
> 
> Nearly all of this should have absolutely no functional impact, however it
> does add a singular VM_WARN_ON() case.

Thanks for looking at this function and adding more clarity.  I'm happy
to have comments within the code, especially tricky areas.  But I find
that adding almost 50 lines to this function makes it rather hard to
follow.

Can we remove the more obvious comments and possibly reduce the nesting
of others so there are less lines?

For example in patch 2:
        /*
         * If there is a previous VMA specified, find the next, otherwise find                                          
         * the first.                                                                                                   
         */ 
        vma = find_vma(mm, prev ? prev->vm_end : 0);

Is rather verbose for something that can be seen in the code itself.

I think we are risking over-documenting what is going on here which is
making the code harder to read; the function is pushing 200 lines now.

> 
> Lorenzo Stoakes (4):
>   mm/mmap/vma_merge: further improve prev/next VMA naming
>   mm/mmap/vma_merge: set next to NULL if not applicable
>   mm/mmap/vma_merge: extend invariants, avoid invalid res, vma
>   mm/mmap/vma_merge: be explicit about the non-mergeable case
> 
>  mm/mmap.c | 165 +++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 107 insertions(+), 58 deletions(-)
> 
> --
> 2.39.2

Lorenzo Stoakes March 20, 2023, 5:09 p.m. UTC | #2

On Mon, Mar 20, 2023 at 12:47:07PM -0400, Liam R. Howlett wrote:
> * Lorenzo Stoakes <[e-mail]> [230318 07:15]:
> > Following on from Vlastimil Babka's patch series "cleanup vma_merge() and
> > improve mergeability tests" which was in turn based on Liam's prior
> > cleanups, this patch series introduces changes discussed in review of
> > Vlastimil's series and goes further in attempting to make the logic as
> > clear as possible.
> >
> > Nearly all of this should have absolutely no functional impact, however it
> > does add a singular VM_WARN_ON() case.
>
> Thanks for looking at this function and adding more clarity.  I'm happy
> to have comments within the code, especially tricky areas.  But I find
> that adding almost 50 lines to this function makes it rather hard to
> follow.
>
> Can we remove the more obvious comments and possibly reduce the nesting
> of others so there are less lines?
>
> For example in patch 2:
>         /*
>          * If there is a previous VMA specified, find the next, otherwise find
>          * the first.
>          */
>         vma = find_vma(mm, prev ? prev->vm_end : 0);
>
> Is rather verbose for something that can be seen in the code itself.
>
> I think we are risking over-documenting what is going on here which is
> making the code harder to read; the function is pushing 200 lines now.
>
> >
> > Lorenzo Stoakes (4):
> >   mm/mmap/vma_merge: further improve prev/next VMA naming
> >   mm/mmap/vma_merge: set next to NULL if not applicable
> >   mm/mmap/vma_merge: extend invariants, avoid invalid res, vma
> >   mm/mmap/vma_merge: be explicit about the non-mergeable case
> >
> >  mm/mmap.c | 165 +++++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 107 insertions(+), 58 deletions(-)
> >
> > --
> > 2.39.2
>

Sure, I did try not to overdo things (once you start simplifying you can go
too far), but it seems like I _did_ go too far on the commenting (perhaps
pushing too far the other way).

I will simplify, remove things implied by the code and strip down + respin.