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.
