[PATCH v2] vfs: remove redundant smp_mb for thp handling in do_dentry_open

[PATCH v2] vfs: remove redundant smp_mb for thp handling in do_dentry_open

[Mateusz Guzik]

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@gmail.com>
---

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

Re: [PATCH v2] vfs: remove redundant smp_mb for thp handling in do_dentry_open

[Christian Brauner]

On Mon, 24 Jun 2024 10:54:02 +0200, Mateusz Guzik wrote:
> opening for write performs:
> 
> if (f->f_mode & FMODE_WRITE) {
> [snip]
>         smp_mb();
>         if (filemap_nr_thps(inode->i_mapping)) {
> [snip]
>         }
> }
> 
> [...]

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/1] vfs: remove redundant smp_mb for thp handling in do_dentry_open
      https://git.kernel.org/vfs/vfs/c/badb54d62002

Re: [PATCH v2] vfs: remove redundant smp_mb for thp handling in do_dentry_open

[Jan Kara]

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@gmail.com>

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

Reviewed-by: Jan Kara <jack@suse.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@suse.com>
SUSE Labs, CR