Re: [PATCH 2/5] btrfs: set blocking_writers directly, no increment or decrement

[David Sterba <dsterba@suse.cz>]

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.