| Message ID | 6c64bb27ef5e0150db63c2415bb31f98331d2d4c.1571340084.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Extent buffer locking and documentation | expand |
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 >
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.
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
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(-)