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@xxxxxxxxx> Yeah, I didn't like the ifdef but this version looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@xxxxxxx> 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@xxxxxxxx> SUSE Labs, CR