Date:   Wed, 15 Mar 2023 21:43:50 +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 08/10] mm/mmap/vma_merge: convert mergeability checks to
 return bool
Message-ID: <7c7f9045-feda-446c-826c-5061534afcc0@lucifer.local>
References: <20230309111258.24079-1-vbabka@suse.cz>
 <20230309111258.24079-9-vbabka@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230309111258.24079-9-vbabka@suse.cz>
Precedence: bulk

On Thu, Mar 09, 2023 at 12:12:56PM +0100, Vlastimil Babka wrote:
> The comments already mention returning 'true' so make the code match
> them.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/mmap.c | 53 +++++++++++++++++++++++++----------------------------
>  1 file changed, 25 insertions(+), 28 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index c51d69592e4e..d20bbe9ec613 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -744,10 +744,10 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma,
>   * If the vma has a ->close operation then the driver probably needs to release
>   * per-vma resources, so we don't attempt to merge those.
>   */
> -static inline int is_mergeable_vma(struct vm_area_struct *vma,
> -				   struct file *file, unsigned long vm_flags,
> -				   struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
> -				   struct anon_vma_name *anon_name)
> +static inline bool is_mergeable_vma(struct vm_area_struct *vma,
> +		struct file *file, unsigned long vm_flags,
> +		struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
> +		struct anon_vma_name *anon_name)
>  {
>  	/*
>  	 * VM_SOFTDIRTY should not prevent from VMA merging, if we
> @@ -758,21 +758,20 @@ static inline int is_mergeable_vma(struct vm_area_struct *vma,
>  	 * extended instead.
>  	 */
>  	if ((vma->vm_flags ^ vm_flags) & ~VM_SOFTDIRTY)
> -		return 0;
> +		return false;
>  	if (vma->vm_file != file)
> -		return 0;
> +		return false;
>  	if (vma->vm_ops && vma->vm_ops->close)
> -		return 0;
> +		return false;
>  	if (!is_mergeable_vm_userfaultfd_ctx(vma, vm_userfaultfd_ctx))
> -		return 0;
> +		return false;
>  	if (!anon_vma_name_eq(anon_vma_name(vma), anon_name))
> -		return 0;
> -	return 1;
> +		return false;
> +	return true;
>  }
>
> -static inline int is_mergeable_anon_vma(struct anon_vma *anon_vma1,
> -					struct anon_vma *anon_vma2,
> -					struct vm_area_struct *vma)
> +static inline bool is_mergeable_anon_vma(struct anon_vma *anon_vma1,
> +		 struct anon_vma *anon_vma2, struct vm_area_struct *vma)
>  {
>  	/*
>  	 * The list_is_singular() test is to avoid merging VMA cloned from
> @@ -780,7 +779,7 @@ static inline int is_mergeable_anon_vma(struct anon_vma *anon_vma1,
>  	 */
>  	if ((!anon_vma1 || !anon_vma2) && (!vma ||
>  		list_is_singular(&vma->anon_vma_chain)))
> -		return 1;
> +		return true;
>  	return anon_vma1 == anon_vma2;
>  }
>
> @@ -795,19 +794,18 @@ static inline int is_mergeable_anon_vma(struct anon_vma *anon_vma1,
>   * indices (16TB on ia32) because do_mmap() does not permit mmap's which
>   * wrap, nor mmaps which cover the final page at index -1UL.
>   */
> -static int
> +static bool
>  can_vma_merge_before(struct vm_area_struct *vma, unsigned long vm_flags,
> -		     struct anon_vma *anon_vma, struct file *file,
> -		     pgoff_t vm_pgoff,
> -		     struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
> -		     struct anon_vma_name *anon_name)
> +		struct anon_vma *anon_vma, struct file *file,
> +		pgoff_t vm_pgoff, struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
> +		struct anon_vma_name *anon_name)
>  {
>  	if (is_mergeable_vma(vma, file, vm_flags, vm_userfaultfd_ctx, anon_name) &&
>  	    is_mergeable_anon_vma(anon_vma, vma->anon_vma, vma)) {
>  		if (vma->vm_pgoff == vm_pgoff)
> -			return 1;
> +			return true;
>  	}
> -	return 0;
> +	return false;
>  }
>
>  /*
> @@ -817,21 +815,20 @@ can_vma_merge_before(struct vm_area_struct *vma, unsigned long vm_flags,
>   * We cannot merge two vmas if they have differently assigned (non-NULL)
>   * anon_vmas, nor if same anon_vma is assigned but offsets incompatible.
>   */
> -static int
> +static bool
>  can_vma_merge_after(struct vm_area_struct *vma, unsigned long vm_flags,
> -		    struct anon_vma *anon_vma, struct file *file,
> -		    pgoff_t vm_pgoff,
> -		    struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
> -		    struct anon_vma_name *anon_name)
> +		struct anon_vma *anon_vma, struct file *file,
> +		pgoff_t vm_pgoff, struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
> +		struct anon_vma_name *anon_name)
>  {
>  	if (is_mergeable_vma(vma, file, vm_flags, vm_userfaultfd_ctx, anon_name) &&
>  	    is_mergeable_anon_vma(anon_vma, vma->anon_vma, vma)) {
>  		pgoff_t vm_pglen;
>  		vm_pglen = vma_pages(vma);
>  		if (vma->vm_pgoff + vm_pglen == vm_pgoff)
> -			return 1;
> +			return true;
>  	}
> -	return 0;
> +	return false;
>  }
>
>  /*
> --
> 2.39.2
>

Partying like it's C99 :) Good improvement, feels sensible to move on from using
return types as if the 1st edition of The C Programming Language were still
current! Therefore,

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