All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: linux-fsdevel@vger.kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Mateusz Guzik <mjguzik@gmail.com>,
	Eric Dumazet <edumazet@google.com>,
	"Paul E. McKenney" <paulmck@kernel.org>
Subject: [RFC] why do we need smp_rmb/smp_wmb pair in fd_install()/expand_fdtable()?
Date: Thu, 8 Aug 2024 03:50:29 +0100	[thread overview]
Message-ID: <20240808025029.GB5334@ZenIV> (raw)

	Take a look at fs/file.c:expand_fdtable() and fs/file.c:fd_install()

In the end of the former:
        copy_fdtable(new_fdt, cur_fdt);
        rcu_assign_pointer(files->fdt, new_fdt);
        if (cur_fdt != &files->fdtab)
                call_rcu(&cur_fdt->rcu, free_fdtable_rcu);
        /* coupled with smp_rmb() in fd_install() */
        smp_wmb();
        return 1;
the only caller (expand_files()) has
        expanded = expand_fdtable(files, nr);
        files->resize_in_progress = false;

OTOH, in fd_install() there's this:
        if (unlikely(files->resize_in_progress)) {
                rcu_read_unlock_sched();
                spin_lock(&files->file_lock);
                fdt = files_fdtable(files);
                WARN_ON(fdt->fd[fd] != NULL);
                rcu_assign_pointer(fdt->fd[fd], file);
                spin_unlock(&files->file_lock);
                return;
        }
        /* coupled with smp_wmb() in expand_fdtable() */
        smp_rmb();
        fdt = rcu_dereference_sched(files->fdt);

What's the problem with droping both barriers and turning that
into
        expanded = expand_fdtable(files, nr);
        smp_store_release(&files->resize_in_progress, false);
and
        if (unlikely(smp_load_acquire(&files->resize_in_progress))) {
		....
                return;
        }
        fdt = rcu_dereference_sched(files->fdt);
resp.?  Anyone who sees ->resize_in_progress being false will
see all stores prior to setting ->fdt on the expand size...

That went into the tree in 8a81252b774b "fs/file.c: don't acquire
files->file_lock in fd_install()".  I don't remember that having been
discussed back then, but it had been 7 years ago...
<check the thread on lore>
Nobody asked, AFAICS.

Is there any problem with going that way?  Because I'm pretty sure
that it's would be cheaper to use store_release/load_acquire instead
of paying for smp_rmb() on the fd_install() side...

Am I missing something subtle here?

             reply	other threads:[~2024-08-08  2:50 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-08-08  2:50 Al Viro [this message]
2024-08-08  3:06 ` [RFC] why do we need smp_rmb/smp_wmb pair in fd_install()/expand_fdtable()? Linus Torvalds
2024-08-08  3:35   ` Al Viro
2024-08-08  3:46     ` Al Viro
2024-08-08  6:08       ` Mateusz Guzik
2024-12-05 12:03         ` [RFC PATCH] fs: elide the smp_rmb fence in fd_install() Mateusz Guzik
2024-12-05 14:18           ` Al Viro
2024-12-05 14:43             ` Mateusz Guzik
2024-12-05 18:41               ` Paul E. McKenney
2024-12-05 19:03                 ` Mateusz Guzik
2024-12-05 20:01                   ` Paul E. McKenney
2024-12-05 20:15                     ` Mateusz Guzik
2024-12-05 21:17                       ` Paul E. McKenney
2024-12-05 19:26                 ` Linus Torvalds
2024-12-05 19:47                   ` Mateusz Guzik
2024-12-05 20:11                     ` Paul E. McKenney
2024-12-06 12:11                       ` Christian Brauner
2024-12-05 20:06                   ` Paul E. McKenney
2024-12-05 14:46           ` Jan Kara
2024-12-05 15:01             ` Mateusz Guzik
2024-12-05 15:29               ` Jan Kara
2024-12-05 15:36                 ` Mateusz Guzik
2024-12-06 15:32                   ` Jan Kara
2024-12-05 14:58           ` Christian Brauner
2024-12-05 15:06             ` Mateusz Guzik
2024-08-08 13:20   ` [RFC] why do we need smp_rmb/smp_wmb pair in fd_install()/expand_fdtable()? Christian Brauner
2024-08-08 16:11     ` Linus Torvalds

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240808025029.GB5334@ZenIV \
    --to=viro@zeniv.linux.org.uk \
    --cc=edumazet@google.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=mjguzik@gmail.com \
    --cc=paulmck@kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.