On Sun, Jan 08, 2023 at 07:50:01PM +0100, Andreas Gruenbacher wrote:
> On Sun, Jan 8, 2023 at 6:32 PM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> > On Wed, Jan 04, 2023 at 07:08:17PM +0000, Matthew Wilcox wrote:
> > > On Wed, Jan 04, 2023 at 09:53:17AM -0800, Darrick J. Wong wrote:
> > > > I wonder if this should be reworked a bit to reduce indenting:
> > > >
> > > > if (PTR_ERR(folio) == -ESTALE) {
> > >
> > > FYI this is a bad habit to be in. The compiler can optimise
> > >
> > > if (folio == ERR_PTR(-ESTALE))
> > >
> > > better than it can optimise the other way around.
> >
> > Yes. I think doing the recording that Darrick suggested combined
> > with this style would be best:
> >
> > if (folio == ERR_PTR(-ESTALE)) {
> > iter->iomap.flags |= IOMAP_F_STALE;
> > return 0;
> > }
> > if (IS_ERR(folio))
> > return PTR_ERR(folio);
>
> Again, I've implemented this as a nested if because the -ESTALE case
> should be pretty rare, and if we unnest, we end up with an additional
> check on the main code path. To be specific, the "before" code here on
> my current system is this:
>
> ------------------------------------
> if (IS_ERR(folio)) {
> 22ad: 48 81 fd 00 f0 ff ff cmp $0xfffffffffffff000,%rbp
> 22b4: 0f 87 bf 03 00 00 ja 2679 <iomap_write_begin+0x499>
> return 0;
> }
> return PTR_ERR(folio);
> }
> [...]
> 2679: 89 e8 mov %ebp,%eax
> if (folio == ERR_PTR(-ESTALE)) {
> 267b: 48 83 fd 8c cmp $0xffffffffffffff8c,%rbp
> 267f: 0f 85 b7 fc ff ff jne 233c <iomap_write_begin+0x15c>
> iter->iomap.flags |= IOMAP_F_STALE;
> 2685: 66 81 4b 42 00 02 orw $0x200,0x42(%rbx)
> return 0;
> 268b: e9 aa fc ff ff jmp 233a <iomap_write_begin+0x15a>
> ------------------------------------
>
> While the "after" code is this:
>
> ------------------------------------
> if (folio == ERR_PTR(-ESTALE)) {
> 22ad: 48 83 fd 8c cmp $0xffffffffffffff8c,%rbp
> 22b1: 0f 84 bc 00 00 00 je 2373 <iomap_write_begin+0x193>
> iter->iomap.flags |= IOMAP_F_STALE;
> return 0;
> }
> if (IS_ERR(folio))
> return PTR_ERR(folio);
> 22b7: 89 e8 mov %ebp,%eax
> if (IS_ERR(folio))
> 22b9: 48 81 fd 00 f0 ff ff cmp $0xfffffffffffff000,%rbp
> 22c0: 0f 87 82 00 00 00 ja 2348 <iomap_write_begin+0x168>
> ------------------------------------
>
> The compiler isn't smart enough to re-nest the ifs by recognizing that
> folio == ERR_PTR(-ESTALE) is a subset of IS_ERR(folio).
>
> So do you still insist on that un-nesting even though it produces worse code?
Me? Not anymore. :)
--D
> Thanks,
> Andreas
>
