kevin/linux
1
Fork 0
Code

vfs: remove redundant smp_mb for thp handling in do_dentry_open

Browse Source 
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>
Link: https://lore.kernel.org/r/20240624085402.493630-1-mjguzik@gmail.com
Signed-off-by: Christian Brauner <brauner@kernel.org>
This commit is contained in:
Mateusz Guzik 2024-06-24 10:54:02 +02:00 committed by Christian Brauner
parent 153216cf7b
commit 8e3447822d
No known key found for this signature in database
GPG Key ID: 91C61BC06578DCA2
2 changed files with 9 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
fs/open.c
View File

@ -986,12 +986,11 @@ static int do_dentry_open(struct file *f,
*/ */
if (f->f_mode & FMODE_WRITE) { if (f->f_mode & FMODE_WRITE) {
/* /*
* Paired with smp_mb() in collapse_file() to ensure nr_thps * Depends on full fence from get_write_access() to synchronize
* is up to date and the update to i_writecount by * against collapse_file() regarding i_writecount and nr_thps
* get_write_access() is visible. Ensures subsequent insertion * updates. Ensures subsequent insertion of THPs into the page
* of THPs into the page cache will fail. * cache will fail.
*/ */
smp_mb();
if (filemap_nr_thps(inode->i_mapping)) { if (filemap_nr_thps(inode->i_mapping)) {
struct address_space *mapping = inode->i_mapping; struct address_space *mapping = inode->i_mapping;

10
mm/khugepaged.c
View File

@ -2000,9 +2000,9 @@ out_unlock:
if (!is_shmem) { if (!is_shmem) {
filemap_nr_thps_inc(mapping); filemap_nr_thps_inc(mapping);
/* /*
* Paired with smp_mb() in do_dentry_open() to ensure * Paired with the fence in do_dentry_open() -> get_write_access()
* i_writecount is up to date and the update to nr_thps is * to ensure i_writecount is up to date and the update to nr_thps
* visible. Ensures the page cache will be truncated if the * is visible. Ensures the page cache will be truncated if the
* file is opened writable. * file is opened writable.
*/ */
smp_mb(); smp_mb();
@ -2190,8 +2190,8 @@ rollback:
if (!is_shmem && result == SCAN_COPY_MC) { if (!is_shmem && result == SCAN_COPY_MC) {
filemap_nr_thps_dec(mapping); filemap_nr_thps_dec(mapping);
/* /*
* Paired with smp_mb() in do_dentry_open() to * Paired with the fence in do_dentry_open() -> get_write_access()
* ensure the update to nr_thps is visible. * to ensure the update to nr_thps is visible.
*/ */
smp_mb(); smp_mb();
} }