[2/5] btrfs: set blocking_writers directly, no increment or decrement
diff mbox series

Message ID 6c64bb27ef5e0150db63c2415bb31f98331d2d4c.1571340084.git.dsterba@suse.com
State New
Headers show
Series
  • Extent buffer locking and documentation
Related show

Commit Message

David Sterba Oct. 17, 2019, 7:38 p.m. UTC
The increment and decrement was inherited from previous version that
used atomics, switched in commit 06297d8cefca ("btrfs: switch
extent_buffer blocking_writers from atomic to int"). The only possible
values are 0 and 1 so we can set them directly.

The generated assembly (gcc 9.x) did the direct value assignment in
btrfs_set_lock_blocking_write:

     5d:   test   %eax,%eax
     5f:   je     62 <btrfs_set_lock_blocking_write+0x22>
     61:   retq

  -  62:   lock incl 0x44(%rdi)
  -  66:   add    $0x50,%rdi
  -  6a:   jmpq   6f <btrfs_set_lock_blocking_write+0x2f>

  +  62:   movl   $0x1,0x44(%rdi)
  +  69:   add    $0x50,%rdi
  +  6d:   jmpq   72 <btrfs_set_lock_blocking_write+0x32>

The part in btrfs_tree_unlock did a decrement because
BUG_ON(blockers > 1) is probably not a strong hint for the compiler, but
otherwise the output looks safe:

  - lock decl 0x44(%rdi)

  + sub    $0x1,%eax
  + mov    %eax,0x44(%rdi)

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/locking.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Nikolay Borisov Oct. 18, 2019, 12:08 p.m. UTC | #1
On 17.10.19 г. 22:38 ч., David Sterba wrote:
> The increment and decrement was inherited from previous version that
> used atomics, switched in commit 06297d8cefca ("btrfs: switch
> extent_buffer blocking_writers from atomic to int"). The only possible
> values are 0 and 1 so we can set them directly.
> 
> The generated assembly (gcc 9.x) did the direct value assignment in
> btrfs_set_lock_blocking_write:
> 
>      5d:   test   %eax,%eax
>      5f:   je     62 <btrfs_set_lock_blocking_write+0x22>
>      61:   retq
> 
>   -  62:   lock incl 0x44(%rdi)
>   -  66:   add    $0x50,%rdi
>   -  6a:   jmpq   6f <btrfs_set_lock_blocking_write+0x2f>
> 
>   +  62:   movl   $0x1,0x44(%rdi)
>   +  69:   add    $0x50,%rdi
>   +  6d:   jmpq   72 <btrfs_set_lock_blocking_write+0x32>
> 
> The part in btrfs_tree_unlock did a decrement because
> BUG_ON(blockers > 1) is probably not a strong hint for the compiler, but
> otherwise the output looks safe:
> 
>   - lock decl 0x44(%rdi)
> 
>   + sub    $0x1,%eax
>   + mov    %eax,0x44(%rdi)

I find it strange that plain inc/decs had the lock prefix in the
resulting assembly. The sub instruction can have the lock prefix but
here the compiler chooses not to. While this doesn't mean it's buggy I'm
just curious what's happening.

> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/locking.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> index c84c650e56c7..00edf91c3d1c 100644
> --- a/fs/btrfs/locking.c
> +++ b/fs/btrfs/locking.c
> @@ -109,7 +109,7 @@ void btrfs_set_lock_blocking_write(struct extent_buffer *eb)
>  	if (eb->blocking_writers == 0) {
>  		btrfs_assert_spinning_writers_put(eb);
>  		btrfs_assert_tree_locked(eb);
> -		eb->blocking_writers++;
> +		eb->blocking_writers = 1;
>  		write_unlock(&eb->lock);
>  	}
>  }
> @@ -305,7 +305,7 @@ void btrfs_tree_unlock(struct extent_buffer *eb)
>  
>  	if (blockers) {
>  		btrfs_assert_no_spinning_writers(eb);
> -		eb->blocking_writers--;
> +		eb->blocking_writers = 0;
>  		/*
>  		 * We need to order modifying blocking_writers above with
>  		 * actually waking up the sleepers to ensure they see the
>
David Sterba Oct. 18, 2019, 5:31 p.m. UTC | #2
On Fri, Oct 18, 2019 at 03:08:43PM +0300, Nikolay Borisov wrote:
> 
> 
> On 17.10.19 г. 22:38 ч., David Sterba wrote:
> > The increment and decrement was inherited from previous version that
> > used atomics, switched in commit 06297d8cefca ("btrfs: switch
> > extent_buffer blocking_writers from atomic to int"). The only possible
> > values are 0 and 1 so we can set them directly.
> > 
> > The generated assembly (gcc 9.x) did the direct value assignment in
> > btrfs_set_lock_blocking_write:
> > 
> >      5d:   test   %eax,%eax
> >      5f:   je     62 <btrfs_set_lock_blocking_write+0x22>
> >      61:   retq
> > 
> >   -  62:   lock incl 0x44(%rdi)
> >   -  66:   add    $0x50,%rdi
> >   -  6a:   jmpq   6f <btrfs_set_lock_blocking_write+0x2f>
> > 
> >   +  62:   movl   $0x1,0x44(%rdi)
> >   +  69:   add    $0x50,%rdi
> >   +  6d:   jmpq   72 <btrfs_set_lock_blocking_write+0x32>
> > 
> > The part in btrfs_tree_unlock did a decrement because
> > BUG_ON(blockers > 1) is probably not a strong hint for the compiler, but
> > otherwise the output looks safe:
> > 
> >   - lock decl 0x44(%rdi)
> > 
> >   + sub    $0x1,%eax
> >   + mov    %eax,0x44(%rdi)
> 
> I find it strange that plain inc/decs had the lock prefix in the
> resulting assembly. The sub instruction can have the lock prefix but
> here the compiler chooses not to. While this doesn't mean it's buggy I'm
> just curious what's happening.

The asm snippet is from the patch that did atomics -> int, which was
atomic_dec(blocking_writers) -> blocking_writers--, yet the result was
plain store.

It's probably not too clear from the context that it referes to the
menined commit 06297d8cefca.

Patch
diff mbox series

diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index c84c650e56c7..00edf91c3d1c 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -109,7 +109,7 @@  void btrfs_set_lock_blocking_write(struct extent_buffer *eb)
 	if (eb->blocking_writers == 0) {
 		btrfs_assert_spinning_writers_put(eb);
 		btrfs_assert_tree_locked(eb);
-		eb->blocking_writers++;
+		eb->blocking_writers = 1;
 		write_unlock(&eb->lock);
 	}
 }
@@ -305,7 +305,7 @@  void btrfs_tree_unlock(struct extent_buffer *eb)
 
 	if (blockers) {
 		btrfs_assert_no_spinning_writers(eb);
-		eb->blocking_writers--;
+		eb->blocking_writers = 0;
 		/*
 		 * We need to order modifying blocking_writers above with
 		 * actually waking up the sleepers to ensure they see the