| Message ID | 20241113155103.4194099-1-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vfs: make evict() use smp_mb__after_spinlock instead of smp_mb | expand |
On Wed 13-11-24 16:51:03, Mateusz Guzik wrote: > It literally directly follows a spin_lock() call. > > This whacks an explicit barrier on x86-64. > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> > This plausibly can go away altogether, but I could not be arsed to > convince myself that's correct. Individuals willing to put in time are > welcome :) AFAICS there's nothing else really guaranteeing the last store to inode->i_state cannot be reordered up to after the wake up so I think the barrier should be there. Honza > > fs/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/inode.c b/fs/inode.c > index e5a60084a7a9..b3db1234737f 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -817,7 +817,7 @@ static void evict(struct inode *inode) > * ___wait_var_event() either sees the bit cleared or > * waitqueue_active() check in wake_up_var() sees the waiter. > */ > - smp_mb(); > + smp_mb__after_spinlock(); > inode_wake_up_bit(inode, __I_NEW); > BUG_ON(inode->i_state != (I_FREEING | I_CLEAR)); > spin_unlock(&inode->i_lock); > -- > 2.43.0 >
On Wed, Nov 13, 2024 at 5:17 PM Jan Kara <jack@suse.cz> wrote: > > On Wed 13-11-24 16:51:03, Mateusz Guzik wrote: > > It literally directly follows a spin_lock() call. > > > > This whacks an explicit barrier on x86-64. > > > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > > Looks good. Feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > thanks > > This plausibly can go away altogether, but I could not be arsed to > > convince myself that's correct. Individuals willing to put in time are > > welcome :) > > AFAICS there's nothing else really guaranteeing the last store to > inode->i_state cannot be reordered up to after the wake up so I think the > barrier should be there. > There is a bunch of lock round trips in this routine alone, including on i_lock itself, but that aside: I *suspect* something like spin_wait_unlocked(&inode->i_state) shipping with a full fence at the beginning of the routine would correctly allow to check all the possible waiter et al flags without acquiring the lock anymore, shaving off at least 2 lock trips in the common case. However, I don't see such a routine as is and I'm definitely not going to flame about adding it for the time being. > Honza > > > > fs/inode.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/inode.c b/fs/inode.c > > index e5a60084a7a9..b3db1234737f 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -817,7 +817,7 @@ static void evict(struct inode *inode) > > * ___wait_var_event() either sees the bit cleared or > > * waitqueue_active() check in wake_up_var() sees the waiter. > > */ > > - smp_mb(); > > + smp_mb__after_spinlock(); > > inode_wake_up_bit(inode, __I_NEW); > > BUG_ON(inode->i_state != (I_FREEING | I_CLEAR)); > > spin_unlock(&inode->i_lock); > > -- > > 2.43.0 > > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
On Wed, 13 Nov 2024 16:51:03 +0100, Mateusz Guzik wrote: > It literally directly follows a spin_lock() call. > > This whacks an explicit barrier on x86-64. > > 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: make evict() use smp_mb__after_spinlock instead of smp_mb https://git.kernel.org/vfs/vfs/c/45c9faf50665
diff --git a/fs/inode.c b/fs/inode.c index e5a60084a7a9..b3db1234737f 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -817,7 +817,7 @@ static void evict(struct inode *inode) * ___wait_var_event() either sees the bit cleared or * waitqueue_active() check in wake_up_var() sees the waiter. */ - smp_mb(); + smp_mb__after_spinlock(); inode_wake_up_bit(inode, __I_NEW); BUG_ON(inode->i_state != (I_FREEING | I_CLEAR)); spin_unlock(&inode->i_lock);
It literally directly follows a spin_lock() call. This whacks an explicit barrier on x86-64. Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> --- This plausibly can go away altogether, but I could not be arsed to convince myself that's correct. Individuals willing to put in time are welcome :) fs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)