Date:   Wed, 15 Mar 2023 19:04:13 +0000
From:   Lorenzo Stoakes <lstoakes@gmail.com>
To:     Vlastimil Babka <vbabka@suse.cz>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        "Liam R. Howlett" <Liam.Howlett@oracle.com>,
        Matthew Wilcox <willy@infradead.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, patches@lists.linux.dev,
        maple-tree@lists.infradead.org
Subject: Re: [PATCH 02/10] mm/mmap/vma_merge: use the proper vma pointer in
 case 3
Message-ID: <f60110e2-2b91-4233-8f00-65682fc7907b@lucifer.local>
References: <20230309111258.24079-1-vbabka@suse.cz>
 <20230309111258.24079-3-vbabka@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230309111258.24079-3-vbabka@suse.cz>
Precedence: bulk

On Thu, Mar 09, 2023 at 12:12:50PM +0100, Vlastimil Babka wrote:
> In case 3 we we use 'next' for everything but vma_pgoff. So use 'next'
> for that as well, instead of 'mid', for consistency. Then in case 8 we
> have to use 'mid' explicitly, which should also make the intent more
> obvious.
>
> Adjust the diagram for cases 1-3 in the comment to match the code - we
> are using 'next' for case 3 so mark the range with XXXX instead of NNNN.
> For case 2 that's a no-op as the code doesn't touch 'next' or 'mid'. For
> case 1 it's now wrong but that will be fixed next.
>
> No functional change.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/mmap.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 0a8b052e3022..1af4c9bc2c87 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -857,11 +857,11 @@ can_vma_merge_after(struct vm_area_struct *vma, unsigned long vm_flags,
>   *    mmap, brk or    case 4 below       case 5 below
>   *    mremap move:
>   *                        AAAA               AAAA
> - *                    PPPP    NNNN       PPPPNNNNXXXX
> + *                    PPPP    XXXX       PPPPNNNNXXXX
>   *                    might become       might become
>   *                    PPPPPPPPPPPP 1 or  PPPPPPPPPPPP 6 or
> - *                    PPPPPPPPNNNN 2 or  PPPPPPPPXXXX 7 or
> - *                    PPPPNNNNNNNN 3     PPPPXXXXXXXX 8
> + *                    PPPPPPPPXXXX 2 or  PPPPPPPPXXXX 7 or
> + *                    PPPPXXXXXXXX 3     PPPPXXXXXXXX 8
>   *

I'm glad you're making things more consistent and what you're addressing here is
a real clanger, but these diagrams while great to have do definitely feel
quite confusing even now.  But that's something for a future patch!

>   * It is important for case 8 that the vma NNNN overlapping the
>   * region AAAA is never going to extended over XXXX. Instead XXXX must
> @@ -978,9 +978,10 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  			vma = next;			/* case 3 */
>  			vma_start = addr;
>  			vma_end = next->vm_end;
> -			vma_pgoff = mid->vm_pgoff;
> +			vma_pgoff = next->vm_pgoff;
>  			err = 0;
>  			if (mid != next) {		/* case 8 */
> +				vma_pgoff = mid->vm_pgoff;
>  				remove = mid;
>  				err = dup_anon_vma(next, mid);
>  			}
> --
> 2.39.2
>

This does fix a big incongruity in that previously everything but vm_pgoff was
relative to next, while in the non-8 case mid is equal to next anyway.

Good, clarifying improvement!

Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com>