linux-ext4 - Re: [PATCH v5 7/9] iomap/xfs: Eliminate the iomap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 10 Jan 2023 13:56:44 -0800
From:   "Darrick J. Wong" <djwong@...nel.org>
To:     Andreas Gruenbacher <agruenba@...hat.com>
Cc:     Christoph Hellwig <hch@...radead.org>,
        Matthew Wilcox <willy@...radead.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-ext4@...r.kernel.org, cluster-devel@...hat.com
Subject: Re: [PATCH v5 7/9] iomap/xfs: Eliminate the iomap_valid handler

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@...radead.org> 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
>