Received-SPF: pass (google.com: domain of linux-kernel+bounces-186424-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 22 May 2024 11:19:45 -0400
From: "Liam R. Howlett" <Liam.Howlett@oracle.com>
To: Mateusz Guzik <mjguzik@gmail.com>
Cc: akpm@linux-foundation.org, vbabka@suse.cz, lstoakes@gmail.com,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH v2] mm: batch unlink_file_vma calls in free_pgd_range
Message-ID: <v4k3u3h5b4xkss3qlltfqnlmobbihzoelqhnmjbhc57jup52wp@csaqg7h45co2>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@oracle.com>, 
	Mateusz Guzik <mjguzik@gmail.com>, akpm@linux-foundation.org, vbabka@suse.cz, lstoakes@gmail.com, 
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
References: <20240521234321.359501-1-mjguzik@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240521234321.359501-1-mjguzik@gmail.com>
User-Agent: NeoMutt/20231103
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?he1+YCmLTPPzP4jG30jgvfWmp77jilt+kxD8fBKV0Ttk4ZlFF4R8BsILEKk2?=
 =?us-ascii?Q?L17+woa+zY84juEdEQRBgz4dT/ThjS3qQD/FboIJklqu3YbCkW6RrpB/Rz2P?=
 =?us-ascii?Q?XaSFSGy0rburPPukX6uA8spW9uliHM3LNKHLXg9GvjK740rPVCrYq0lCaS0v?=
 =?us-ascii?Q?1eVBms1g7ozhu03PXqlS8nt+CQtFjW9sXtp5erLomP1CgjStt/jf7QVoWRGy?=
 =?us-ascii?Q?kQUtT8AHLKxn/nlc9/8FoRy2gTymAzVwj54rta8Zcser4alrbzK2hGT5TjRX?=
 =?us-ascii?Q?jp14ml2bFZRqhCMAzvrfPvrxeqOYdAbhzXSQRHER5tgj/Y2iZQ6g5pnhJyxi?=
 =?us-ascii?Q?zvBDPF24kipJ/uJUCKmFRxnopZL0A3MVv8/bsNZCOogjskyXP2fU0kUwsQgK?=
 =?us-ascii?Q?cONmWQh8txi8iSFVfI6+G2kCsP2gFKpsq0iQdZy2EUFAZHuZ473aqgw8jlFe?=
 =?us-ascii?Q?arSt8EeENTOELb/s5NaEsxA9+0XqMShlbwkWtP0VTJWr1j7+TjCDlEtL7yQs?=
 =?us-ascii?Q?VMKCZ9P11BjnWKtG4czxFqFVVh7l+7XNwDK4d3b3Z9txgfsWDaE4gvEO8U56?=
 =?us-ascii?Q?NdeWZLuK0ddrdtj2dWzvEP2fK1MBYTc8iiiiXOFU9oIKDK8zTuktljT8dftO?=
 =?us-ascii?Q?JWyshlgu9Ff0z7x0V/QUO381uxiynTP7j/tD4ntYR6EA3vUZWjl2wYGxbST/?=
 =?us-ascii?Q?QHkQ743CCB4VzCnmV5UQZh6nucU/A3E18EDAGiLmGTyfGe2umNSrMOBqwRzW?=
 =?us-ascii?Q?Azw1gGy0s0LADZc1RlqIyUrY/GAYdFdsTonG8gzbTKNywDeNRSrCKKccx6fi?=
 =?us-ascii?Q?amI0jtC5yL75GogcRu31omqVxkgSO114wTHnd2HJQqNO4ZUCSozcnT6vWnDk?=
 =?us-ascii?Q?16UfWGigoQP0+Q+7nx+8Z/OSttacUbPo71Kkx8dfppURdnzq777eQyuqJ7n3?=
 =?us-ascii?Q?aXubaV58DHaS3MGoO2w+wo4iUc0bpolJB7Nf4eEzwTLh7kwTt9jXqfbptoJZ?=
 =?us-ascii?Q?Zc7H7AJGIi29oOWQYZZHFspg3+RMPlWNAzSKNACkW6tklZkxXt1Fr6Oq8FnC?=
 =?us-ascii?Q?RkH+5/E7T4V+Ds05JewI51Wqp7LxvUzwHP59IxXMIlwHsNUOISNzY7vlNnMd?=
 =?us-ascii?Q?/66/R03u3tKqg3ThhCh/i2Vy0GRPgy0jZD193b5QN98nt2NYtVbbFb5nDUdx?=
 =?us-ascii?Q?ZExyuMxuoORE10RVFNXkvhC2IWJiN6ZW/Z0Pevr9bVUQtj59pPUvppmjpqWk?=
 =?us-ascii?Q?fyy4H5II3Z8hNES2njYEy7Ru2YOiCsqUALTUNS9E5hJ1qSg27Qt95gC4nhQ/?=
 =?us-ascii?Q?Cx0I2X+9Q+Y+yVAkaPzfBegde1LNZgt/qmogZUn19bTaEK4EceJVDah2J0mJ?=
 =?us-ascii?Q?q5h+7pNl+eSToKdi5u1I9t2sCOwoFrL15RF21Dvj9+UeB34XAmK+GO/P2JVU?=
 =?us-ascii?Q?Z9XrGKLmp4CNu5rAjSJRkx2hqYsV/gVhzwVxrSdCi+tz9/RmzzFJ9cPucFZM?=
 =?us-ascii?Q?VBQh0/dN/N/XJB40Uv5XrF7mpPnsWYLAXo3h00K0srrIg4tK943uSrCOw9Xd?=
 =?us-ascii?Q?4Z2sOnBZAVzg5/86Ma0mYaCQBcTqi+GwCcAE1gVy?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	beVjGp3bP8MFnXMYdL9OTJ9AvEW6rDlPYFhkEBmbmzRvV1A63e7eyLKls+TEuhwKt4XNjMCOnMwpqDlmhr08rnNE3O6YJzeDlxw++dRFjAXDoN0VeioGQIF6I/hSJ4zeBch0meKsgAM0A6iHXoh1+dUvtwWofFeOFlgYgcAdo9YF92qMNt4GXOz1Pb9rn7pMFBwe/AeJSGN99KzSt7IqkxvkB05wfAM90dXKON++xMHvPxSN1PrUYRiteQOqHdUZBnW0sY9YMhJT8INXya7+HnEHQ5zY5ch6W4mNGNhkmSm+nRmd0LdwzjHf9g2vy7tg5gJsDWR84LTFbQABiwZiUFhW2tpgn52/PNJEFbwZ6Z1/0bLLOq7YP5k1QBSYRgIPsd0FVnlQWsxd6AIpBo7CB6JalV3UKkWw3EsbriTfaf8nw2qCy+LMz0hWUKg/6LcBZCm2W6AGYkBoNohmR+fPOoHu65uMS/lhWWXYy3/6bAkCfP9kzKsi7TcZ3LYP/3r2RnL4njL0esVIB1FpcZK/j4d6cBEC/5XglkH54HiyYgUUJcIVzxz7IapPCL7CDJE0f+ONJhZk0dKYCH8xdxLWt5gz7WjuAc09aRMNobKne1Y=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b3f339fd-e337-4d3b-e271-08dc7a729d5d
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2024 15:19:46.6339
 (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: KSNmia4tVkCVeR/UpoDUlx+wRyZm32mZC1QH/CuZ6J7tdUMOVNQ8XStZsWqkpI16XAJflzv1GDQoyD7koPIzwg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB5081
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.12.28.16
 definitions=2024-05-22_08,2024-05-22_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0
 suspectscore=0 spamscore=0 mlxscore=0 adultscore=0 mlxlogscore=999
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2405220103
X-Proofpoint-ORIG-GUID: Vuw-uSO_USUys7Bs8VcfnibV377irLhe
X-Proofpoint-GUID: Vuw-uSO_USUys7Bs8VcfnibV377irLhe

* Mateusz Guzik <mjguzik@gmail.com> [240521 19:43]:
> Execs of dynamically linked binaries at 20-ish cores are bottlenecked on
> the i_mmap_rwsem semaphore, while the biggest singular contributor is
> free_pgd_range inducing the lock acquire back-to-back for all
> consecutive mappings of a given file.
> 
> Tracing the count of said acquires while building the kernel shows:
> [1, 2)     799579 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> [2, 3)          0 |                                                    |
> [3, 4)       3009 |                                                    |
> [4, 5)       3009 |                                                    |
> [5, 6)     326442 |@@@@@@@@@@@@@@@@@@@@@                               |
> 
> So in particular there were 326442 opportunities to coalesce 5 acquires
> into 1.
> 
> Doing so increases execs per second by 4% (~50k to ~52k) when running
> the benchmark linked below.
> 
> The lock remains the main bottleneck, I have not looked at other spots
> yet.

Thanks.  This change is compact and allows for a performance gain.  It
looks good to me.

I guess this would cause a regression on single mappings, probably
within the noise and probably not a real work load.  Just something to
keep in mind to check if the bots yell about some contrived benchmark.

> 
> Bench can be found here:
> http://apollo.backplane.com/DFlyMisc/doexec.c
> 
> $ cc -O2 -o shared-doexec doexec.c
> $ ./shared-doexec $(nproc)
> 
> Note this particular test makes sure binaries are separate, but the
> loader is shared.
> 
> Stats collected on the patched kernel (+ "noinline") with:
> bpftrace -e 'kprobe:unlink_file_vma_batch_process
> { @ = lhist(((struct unlink_vma_file_batch *)arg0)->count, 0, 8, 1); }'
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---
> 
> v2:
> - move new stuff to mm/internal.h
> 
>  mm/internal.h |  9 +++++++++
>  mm/memory.c   | 10 ++++++++--
>  mm/mmap.c     | 41 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 2adabe369403..2e7be1c773f2 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1484,4 +1484,13 @@ static inline void shrinker_debugfs_remove(struct dentry *debugfs_entry,
>  void workingset_update_node(struct xa_node *node);
>  extern struct list_lru shadow_nodes;
>  
> +struct unlink_vma_file_batch {
> +	int count;
> +	struct vm_area_struct *vmas[8];
> +};
> +
> +void unlink_file_vma_batch_init(struct unlink_vma_file_batch *);
> +void unlink_file_vma_batch_add(struct unlink_vma_file_batch *, struct vm_area_struct *);
> +void unlink_file_vma_batch_final(struct unlink_vma_file_batch *);
> +
>  #endif	/* __MM_INTERNAL_H */
> diff --git a/mm/memory.c b/mm/memory.c
> index b5453b86ec4b..1b96dce19796 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -365,6 +365,8 @@ void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
>  		   struct vm_area_struct *vma, unsigned long floor,
>  		   unsigned long ceiling, bool mm_wr_locked)
>  {
> +	struct unlink_vma_file_batch vb;
> +
>  	do {
>  		unsigned long addr = vma->vm_start;
>  		struct vm_area_struct *next;
> @@ -384,12 +386,15 @@ void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
>  		if (mm_wr_locked)
>  			vma_start_write(vma);
>  		unlink_anon_vmas(vma);
> -		unlink_file_vma(vma);
>  
>  		if (is_vm_hugetlb_page(vma)) {
> +			unlink_file_vma(vma);
>  			hugetlb_free_pgd_range(tlb, addr, vma->vm_end,
>  				floor, next ? next->vm_start : ceiling);
>  		} else {
> +			unlink_file_vma_batch_init(&vb);
> +			unlink_file_vma_batch_add(&vb, vma);
> +
>  			/*
>  			 * Optimization: gather nearby vmas into one call down
>  			 */
> @@ -402,8 +407,9 @@ void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas,
>  				if (mm_wr_locked)
>  					vma_start_write(vma);
>  				unlink_anon_vmas(vma);
> -				unlink_file_vma(vma);
> +				unlink_file_vma_batch_add(&vb, vma);
>  			}
> +			unlink_file_vma_batch_final(&vb);
>  			free_pgd_range(tlb, addr, vma->vm_end,
>  				floor, next ? next->vm_start : ceiling);
>  		}
> diff --git a/mm/mmap.c b/mm/mmap.c
> index d6d8ab119b72..1f9a43ecd053 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c

I see why you put them in mm/mmap.c and it's the best place right now,
for some definition of best.  The vma work is spread across several
files.

On that note, kernel/fork.c uses this lock for each cloned vma right
now.  If you saved the file pointer in your struct, it could be used
for bulk add as well.  The only complication I see is the insert order
being inserted "just after mpnt", maybe a bulk add version of the struct
would need two lists of vmas - if the size of the struct is of concern,
I don't think it would be.

> @@ -131,6 +131,47 @@ void unlink_file_vma(struct vm_area_struct *vma)
>  	}
>  }
>  
> +void unlink_file_vma_batch_init(struct unlink_vma_file_batch *vb)
> +{
> +	vb->count = 0;
> +}
> +
> +static void unlink_file_vma_batch_process(struct unlink_vma_file_batch *vb)
> +{
> +	struct address_space *mapping;
> +	int i;
> +
> +	mapping = vb->vmas[0]->vm_file->f_mapping;
> +	i_mmap_lock_write(mapping);
> +	for (i = 0; i < vb->count; i++) {
> +		VM_WARN_ON_ONCE(vb->vmas[i]->vm_file->f_mapping != mapping);
> +		__remove_shared_vm_struct(vb->vmas[i], mapping);
> +	}
> +	i_mmap_unlock_write(mapping);
> +
> +	unlink_file_vma_batch_init(vb);
> +}
> +
> +void unlink_file_vma_batch_add(struct unlink_vma_file_batch *vb,
> +			       struct vm_area_struct *vma)
> +{
> +	if (vma->vm_file == NULL)
> +		return;
> +

It might be worth a comment about count being always ahead of the last
vma in the array.  On first glance, I was concerned about an off-by-one
here (and in the process function).  But maybe it's just me, the
increment is pretty close to this statement - I had to think about
ARRAY_SIZE() here.

> +	if ((vb->count > 0 && vb->vmas[0]->vm_file != vma->vm_file) ||
> +	    vb->count == ARRAY_SIZE(vb->vmas))

Since you are checking vm_file and only support a single vm_file in this
version, it might be worth saving it in your unlink_vma_file_batch
struct.  It could also be used in the processing to reduce dereferencing
to f_mappings.

I'm not sure if this is worth it with modern cpus, though.  I'm just
thinking that this step is executed the most so any speedup here will
help you.

> +		unlink_file_vma_batch_process(vb);
> +
> +	vb->vmas[vb->count] = vma;
> +	vb->count++;
> +}
> +
> +void unlink_file_vma_batch_final(struct unlink_vma_file_batch *vb)
> +{
> +	if (vb->count > 0)
> +		unlink_file_vma_batch_process(vb);
> +}
> +
>  /*
>   * Close a vm structure and free it.
>   */
> -- 
> 2.39.2
> 

Feel free to add

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>