On Tue, Jun 01, 2021 at 09:33:15AM +1000, Dave Chinner wrote:
> On Mon, May 31, 2021 at 03:40:38PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> >
> > Remove the unnecessary convenience variable.
> >
> > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_inode.c | 25 +++++++++++--------------
> > 1 file changed, 11 insertions(+), 14 deletions(-)
> >
> >
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index e4c2da4566f1..1e28997c6f78 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -689,47 +689,44 @@ xfs_inode_inherit_flags(
> > struct xfs_inode *ip,
> > const struct xfs_inode *pip)
> > {
> > - unsigned int di_flags = 0;
> > xfs_failaddr_t failaddr;
> > umode_t mode = VFS_I(ip)->i_mode;
> >
> > if (S_ISDIR(mode)) {
> > if (pip->i_diflags & XFS_DIFLAG_RTINHERIT)
> > - di_flags |= XFS_DIFLAG_RTINHERIT;
> > + ip->i_diflags |= XFS_DIFLAG_RTINHERIT;
> > if (pip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) {
> > - di_flags |= XFS_DIFLAG_EXTSZINHERIT;
> > + ip->i_diflags |= XFS_DIFLAG_EXTSZINHERIT;
> > ip->i_extsize = pip->i_extsize;
> > }
>
> Hmmmm.
>
> IIRC, these functions were originally written this way because the
> compiler generated much better code using a local variable than when
> writing directly to the ip->i_d.di_flags. Is this still true now?
I did a quick look into the generated asm code, and although my asm is a bit
rusty, I don't see that much difference between the two versions, at least not
to describe it as much better code. The following is a snippet of the code with
and without the patch, first line is just as a reference guide from where the
ASM starts. Most of the other occurrences using di_flags variable are similar
if not the same:
Without patch applied:
} else if (S_ISREG(mode)) {
92473: 66 81 f9 00 80 cmp $0x8000,%cx
92478: 0f 84 59 01 00 00 je 925d7 <xfs_dir_ialloc+0x797>
9248e: 41 8b b5 0c 01 00 00 mov 0x10c(%r13),%esi
92485: 31 c9 xor %ecx,%ecx
92487: a8 40 test $0x40,%al
92489: 74 15 je 924a0 <xfs_dir_ialloc+0x660>
9248b: 44 8b 1d 00 00 00 00 mov 0x0(%rip),%r11d # 92492 <xfs_dir_ialloc+0x652>
92492: 41 89 c8 mov %ecx,%r8d
92495: 41 83 c8 40 or $0x40,%r8d
92499: 45 85 db test %r11d,%r11d
9249c: 41 0f 45 c8 cmovne %r8d,%ecx
924a0: a8 80 test $0x80,%al
With patch applied:
} else if (S_ISREG(mode)) {
92483: 66 81 fe 00 80 cmp $0x8000,%si
92488: 0f 84 94 01 00 00 je 92622 <xfs_dir_ialloc+0x7e2>
9248e: 41 8b b5 0c 01 00 00 mov 0x10c(%r13),%esi
92495: a8 40 test $0x40,%al
92497: 74 20 je 924b9 <xfs_dir_ialloc+0x679>
92499: 44 8b 05 00 00 00 00 mov 0x0(%rip),%r8d # 924a0 <xfs_dir_ialloc+0x660>
924a0: 45 85 c0 test %r8d,%r8d
924a3: 74 14 je 924b9 <xfs_dir_ialloc+0x679>
924a5: 83 c9 40 or $0x40,%ecx
924a8: 66 41 89 8d 16 01 00 mov %cx,0x116(%r13)
924af: 00
924b0: 41 0f b7 84 24 16 01 movzwl 0x116(%r12),%eax
924b7: 00 00
924b9: a8 80 test $0x80,%al
Roughly, the main difference here IMHO, is the fact the current code uses xor to
zero out the registers prior test/copying the flags into it, while using the
patch applied, this is replaced by movz instructions since it copies the value's
flag directly from the xfs_inode struct. So, IMHO I don't really see enough
difference to justify dropping this patch.
Anyway, as I mentioned, my ASM is a bit rusty, so take it with a pinch of salt
:), but from my side, you can add:
Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
Cheers.
--
Carlos
