linux-kernel - Re: [PATCH v2] vfs: remove redundant smp_mb for thp handling in do_dentry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240625085411.yqowxadalnha5t3f@quack3>
Date: Tue, 25 Jun 2024 10:54:11 +0200
From: Jan Kara <jack@...e.cz>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: akpm@...ux-foundation.org, brauner@...nel.org, viro@...iv.linux.org.uk,
	jack@...e.cz, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v2] vfs: remove redundant smp_mb for thp handling in
 do_dentry_open

On Mon 24-06-24 10:54:02, Mateusz Guzik wrote:
> opening for write performs:
> 
> if (f->f_mode & FMODE_WRITE) {
> [snip]
>         smp_mb();
>         if (filemap_nr_thps(inode->i_mapping)) {
> [snip]
>         }
> }
> 
> filemap_nr_thps on kernels built without CONFIG_READ_ONLY_THP_FOR
> expands to 0, allowing the compiler to eliminate the entire thing, with
> exception of the fence (and the branch leading there).
> 
> So happens required synchronisation between i_writecount and nr_thps
> changes is already provided by the full fence coming from
> get_write_access -> atomic_inc_unless_negative, thus the smp_mb instance
> above can be removed regardless of CONFIG_READ_ONLY_THP_FOR.
> 
> While I updated commentary in places claiming to match the now-removed
> fence, I did not try to patch them to act on the compile option.
> 
> I did not bother benchmarking it, not issuing a spurious full fence in
> the fast path does not warrant justification from perf standpoint.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@...il.com>

Yeah, I didn't like the ifdef but this version looks good to me. Feel free
to add:

Reviewed-by: Jan Kara <jack@...e.cz>

								Honza

> ---
> 
> v2:
> - just whack the fence instead of ifdefing
> - change To recipient, the person who committed the original change is
>   no longer active
> 
>  fs/open.c       |  9 ++++-----
>  mm/khugepaged.c | 10 +++++-----
>  2 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 28f2fcbebb1b..64976b6dc75f 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -986,12 +986,11 @@ static int do_dentry_open(struct file *f,
>  	 */
>  	if (f->f_mode & FMODE_WRITE) {
>  		/*
> -		 * Paired with smp_mb() in collapse_file() to ensure nr_thps
> -		 * is up to date and the update to i_writecount by
> -		 * get_write_access() is visible. Ensures subsequent insertion
> -		 * of THPs into the page cache will fail.
> +		 * Depends on full fence from get_write_access() to synchronize
> +		 * against collapse_file() regarding i_writecount and nr_thps
> +		 * updates. Ensures subsequent insertion of THPs into the page
> +		 * cache will fail.
>  		 */
> -		smp_mb();
>  		if (filemap_nr_thps(inode->i_mapping)) {
>  			struct address_space *mapping = inode->i_mapping;
>  
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 409f67a817f1..2e017585f813 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1997,9 +1997,9 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,
>  	if (!is_shmem) {
>  		filemap_nr_thps_inc(mapping);
>  		/*
> -		 * Paired with smp_mb() in do_dentry_open() to ensure
> -		 * i_writecount is up to date and the update to nr_thps is
> -		 * visible. Ensures the page cache will be truncated if the
> +		 * Paired with the fence in do_dentry_open() -> get_write_access()
> +		 * to ensure i_writecount is up to date and the update to nr_thps
> +		 * is visible. Ensures the page cache will be truncated if the
>  		 * file is opened writable.
>  		 */
>  		smp_mb();
> @@ -2187,8 +2187,8 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,
>  	if (!is_shmem && result == SCAN_COPY_MC) {
>  		filemap_nr_thps_dec(mapping);
>  		/*
> -		 * Paired with smp_mb() in do_dentry_open() to
> -		 * ensure the update to nr_thps is visible.
> +		 * Paired with the fence in do_dentry_open() -> get_write_access()
> +		 * to ensure the update to nr_thps is visible.
>  		 */
>  		smp_mb();
>  	}
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR