Received-SPF: pass (google.com: domain of linux-kernel+bounces-42956-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 29 Jan 2024 10:04:17 -0500
From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To: Yajun Deng <yajun.deng@linux.dev>
Cc: akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm/mmap: remove the mm parameter in vma_complete()
Message-ID: <20240129150417.3m6jyj3ftdh6ka7x@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
	Yajun Deng <yajun.deng@linux.dev>, akpm@linux-foundation.org,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20240129075305.3512138-1-yajun.deng@linux.dev>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240129075305.3512138-1-yajun.deng@linux.dev>
User-Agent: NeoMutt/20220429
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?BUg4RfSl9PlgeHihlCE8pXedqBAXb5Ic6f7TuH18oNqlyrOagQcLxaPUSPxu?=
 =?us-ascii?Q?LrIrn82DUmlV9AVtKsMb86OzvD/UZFjTqTcJJB77ohkmCJkKYzup8nK0p9tc?=
 =?us-ascii?Q?RVpjjWZXQwkzxiJbhqWp0+U6t11Juvh+s1xhpj9aPmDaBwPLQ+fUcAq4TlN6?=
 =?us-ascii?Q?jgn5SJS2VrNpAYpZ3EK3t0F5vLcu075fTquiDVjVU412HMqp9nNIFu8LI3HO?=
 =?us-ascii?Q?h/M18flSbhlMMtdqr/4BuNQ+5doUzzqSrVoaYpUw1BaxrdHZUnoog01bN6f4?=
 =?us-ascii?Q?nXZFQYVpATOWUSLU++jU+OGEveIlkAKHS0jD63L/p+DTIzdEzkzn1AQeULnA?=
 =?us-ascii?Q?Mkq1/L/H7wYhFmb3uoO3qMblbTJFR0MuXYSVEEMwK5JDH1jteF0ebEvHTOXg?=
 =?us-ascii?Q?ngUgmwrc5up5hUbevY2b5KZzPmWfz0I/rh2H0MfvtCSMZeUnCGOA8FxKKG1k?=
 =?us-ascii?Q?6jl9ZwzioeM4PuG3TkaivAQyD9ShIGGLC/9Ez5BEBpXMann7uhTHxy3JjANF?=
 =?us-ascii?Q?FFbvpZEHXUkveTY+FqRZW4Tfvcz4sZV5kS/cCa8oWgQiFP9AZarmmGKizOF7?=
 =?us-ascii?Q?ChK8FWtBF2Il+JT4dqUZmvUzo6cX00F7IneP2r0irg8t+JvhHZ1reuI77YQ3?=
 =?us-ascii?Q?ZYrK6lWd2h0RqrkwP4ZxLA2UGMJUtgwJi+pTsSM8O9PaRrV27HGCcKAQVgjd?=
 =?us-ascii?Q?rUupNNC5qtUXHanGLey/475c/ePUyeHZfEEpNN1SlXt7ft6ccZtgFrT4Iqvu?=
 =?us-ascii?Q?oBT3umy3IqSJe9c9EFDBjqKubThFpwJ1EZ24BGnb+xGFThcQz4WD7bH5OCAL?=
 =?us-ascii?Q?4FzF2dGbRNEbUweaTzYCtygukV1SpEC+leCEugK/CQYlVKfJBMgvLfZAf6ua?=
 =?us-ascii?Q?zHtscfM+IWiTCAYShMO/D6xTTG2vzojDu0O+b3MsW4DNLEeS5lPH5cobwxSr?=
 =?us-ascii?Q?W6UN7g2z7Tcf90yNM904Piai15OgM7RoV6/k1fYNPmKidZuXdHh52EB4BJt/?=
 =?us-ascii?Q?hDWupez3LQHayLRX6qcGB3Dqyi8mZMfdlwZ8v2LqHECOf8pKr/DEVCMMLGQM?=
 =?us-ascii?Q?oHF0Mki+aIZQkCvAEao1ItRU4UHFTfG1gt0MsK7HhyaFfGEJsU9ZVznk0CFG?=
 =?us-ascii?Q?vjkzwQNUuNArDtFBvS96e7eCnJosF9vu1TAhbDEgyiOJ93/0XLK6Xe5EI65e?=
 =?us-ascii?Q?PfpsHmWybyTcc/Orp1YT7KFaLp8xiZxvLq4+C2ptQ64mv8KLLU9raIWIpcem?=
 =?us-ascii?Q?XPS9DNbbxkEirw+qbyl5ksMxO3I0zcghQoJYYromIw/OqfaD1yGryQW2STC/?=
 =?us-ascii?Q?JrvHbOCUkoOaS75LpmHCREXx3HlVqnvw9O9gyn++ec7sLktJyVVk3/Qdskb6?=
 =?us-ascii?Q?uiABbxk5PQqIiQeyBlb+o/mZC0+TAeKA8kG5OJge8RP5ryXV2+wDc5tFt3wm?=
 =?us-ascii?Q?sCIHrMeaeB1BeldQ153dBSSqwlcUQCPrfaSB+A06C9DuCvrqoUcfyfkuQySB?=
 =?us-ascii?Q?FtfbICo3L7XM3X6eKyepsAnuihQnmgDnJMw0jv8efArpa95YvYqqBkAWa1n/?=
 =?us-ascii?Q?qwK9YaEZy0lmP/8in/2dc+ZuoOXwG8gXlYz9pF/t?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	Hac1Z0TDOSBgqJ1onH54LCPVINT+B0BgGj9nqncG1/cW+x12H71Lk1hGb4xRcJ78OzlG01eOePqXF0zfIDoZLOR08PlCC3L6Uukd7zGB2mWy8WoLatSY66yUh1Nzelvf/fMKEFZD1Y/GnUximf69E0uRD1VP8wI7kMyC4QuP7xIjVUiu+QaeeNftl/xQhWzMtcCaeVH2585id15e/vtHe4vGfsMyu3GTG3Y5tmjdlf4hPhkboJjjNr5rLrHPshBociZT/E4YpRb3Sxcbchl+cm1rQq+4b3yzbmKNilDX1mjBJH4yH5C/9zv0vW6YFAzZkECXMUlW9+sMXnUM21qys2z9Jt0y3DVkHcetgdM0S2+/27fxs/c4mvkyp9XEp9S9BilMu08hmuRXpU4LGPCgG8Lba5LGMUoyVDb8/fXhRrxU6a9t8Fq9peUubStnbFlRThy1w2JE1W0f3vkPfAGlyMWZcid5/wdD1y2l7PULMLkTbcWpZe41+a/Z9cZ+8CT49FqiSvD9H/LsLHGa3yl99nl0z6T3aH7TICkQXAsMBsu/eJtS+8Gejfp4LUf+xj8AQmCHOjinxXRbvIGta3PkZ04zXZ1mo97suKWrYXLf0EM=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f0d77d96-b29f-4dfa-e056-08dc20db9176
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jan 2024 15:04:19.1409
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: nt3oetM/aMwiVqRL9U/w5XIz0W9I42JzAV/TFCpEwwRsuOLKbDTjfz6Wt7Es1teyy5esMcFZIwdHWxGyHSGfyA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB6984
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-29_09,2024-01-29_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 bulkscore=0 malwarescore=0
 suspectscore=0 phishscore=0 mlxscore=0 mlxlogscore=732 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2401290111
X-Proofpoint-GUID: kzWntR_a6owViJECwzDzJngZJp2RlIAJ
X-Proofpoint-ORIG-GUID: kzWntR_a6owViJECwzDzJngZJp2RlIAJ

* Yajun Deng <yajun.deng@linux.dev> [240129 02:53]:
> There are vma_merge() and do_brk_flags() pass mm to vma_complete(), others
> would pass the vma->vm_mm. The following explains that the mm is the
> vma->vm_mm in vma_merge() and do_brk_flags().
> 
> All vma will point to the same mm struct if the vma_merge() is successful.
> So the mm and the vma->mm are the same.

Absolutely, they must be the same.  I don't think vma_merge() checks
this, but it is true.

> 
> vm_brk_flags() and brk syscall will initialize vmi with current->mm,
> so the vma->vm_mm and the current->mm are the same if vma exists in
> do_brk_flags().
> 
> Remove the mm parameter in vma_complete() and get mm from the vma in vp.

You have added a dereference to the two paths that don't need it to
reduce the argument list from 3 to 2.  It's the same number of lines as
well.  vma_shrink() is only used on process creation, but brk is more
common.  Note that this function is marked as inline.

I'm not sure this change is worth making.

> 
> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> ---
>  mm/mmap.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index e97b9144c61a..9b968d1edf55 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -509,11 +509,11 @@ static inline void vma_prepare(struct vma_prepare *vp)
>   *
>   * @vp: The vma_prepare struct
>   * @vmi: The vma iterator
> - * @mm: The mm_struct
>   */
> -static inline void vma_complete(struct vma_prepare *vp,
> -				struct vma_iterator *vmi, struct mm_struct *mm)
> +static inline void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi)
>  {
> +	struct mm_struct *mm = vp->vma->vm_mm;
> +
>  	if (vp->file) {
>  		if (vp->adj_next)
>  			vma_interval_tree_insert(vp->adj_next,
> @@ -666,7 +666,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	vma_set_range(vma, start, end, pgoff);
>  	vma_iter_store(vmi, vma);
>  
> -	vma_complete(&vp, vmi, vma->vm_mm);
> +	vma_complete(&vp, vmi);
>  	return 0;
>  
>  nomem:
> @@ -707,7 +707,7 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  
>  	vma_iter_clear(vmi);
>  	vma_set_range(vma, start, end, pgoff);
> -	vma_complete(&vp, vmi, vma->vm_mm);
> +	vma_complete(&vp, vmi);
>  	return 0;
>  }
>  
> @@ -1030,7 +1030,7 @@ static struct vm_area_struct
>  		}
>  	}
>  
> -	vma_complete(&vp, vmi, mm);
> +	vma_complete(&vp, vmi);
>  	khugepaged_enter_vma(res, vm_flags);
>  	return res;
>  
> @@ -2377,7 +2377,7 @@ static int __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	}
>  
>  	/* vma_complete stores the new vma */
> -	vma_complete(&vp, vmi, vma->vm_mm);
> +	vma_complete(&vp, vmi);
>  
>  	/* Success. */
>  	if (new_below)
> @@ -3145,7 +3145,7 @@ static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  		vm_flags_set(vma, VM_SOFTDIRTY);
>  		vma_iter_store(vmi, vma);
>  
> -		vma_complete(&vp, vmi, mm);
> +		vma_complete(&vp, vmi);
>  		khugepaged_enter_vma(vma, flags);
>  		goto out;
>  	}
> -- 
> 2.25.1
> 
>