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
