diff mbox series

[1/3] xfs: set ip->i_diflags directly in xfs_inode_inherit_flags

Message ID 162250083819.490289.18171121927859927558.stgit@locust (mailing list archive)
State New
Headers show
Series xfs: various unit conversion | expand

Commit Message

Darrick J. Wong May 31, 2021, 10:40 p.m. UTC
From: Darrick J. Wong <djwong@kernel.org>

Remove the unnecessary convenience variable.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_inode.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Comments

Dave Chinner May 31, 2021, 11:33 p.m. UTC | #1
On Mon, May 31, 2021 at 03:40:38PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Remove the unnecessary convenience variable.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  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 think it's worth checking, because we have changed the structure
of the xfs_inode (removed the i_d structure) so maybe this isn't a
concern anymore?

Cheers,

Dave.
Carlos Maiolino June 1, 2021, 10:43 a.m. UTC | #2
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@kernel.org>
> > 
> > Remove the unnecessary convenience variable.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  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@redhat.com>

Cheers.
Darrick J. Wong June 1, 2021, 6:52 p.m. UTC | #3
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@kernel.org>
> > 
> > Remove the unnecessary convenience variable.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  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 think it's worth checking, because we have changed the structure
> of the xfs_inode (removed the i_d structure) so maybe this isn't a
> concern anymore?

Before the patch, the extszinherit code emitted on my system (gcc 10.2)
looked like this (0x15c is the offset of i_extsize and 0x166 is the
offset of i_diflags):

699                     if (pip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) {
   0xffffffffa0375715 <+1909>:  test   $0x10,%ah
   0xffffffffa0375718 <+1912>:  jne    0xffffffffa0375759 <xfs_dir_ialloc+1977>
   0xffffffffa037571a <+1914>:  mov    0x15c(%r13),%esi

700                             di_flags |= XFS_DIFLAG_EXTSZINHERIT;
   0xffffffffa0375759 <+1977>:  mov    0x15c(%r12),%esi
   0xffffffffa0375761 <+1985>:  or     $0x10,%ch

701                             ip->i_extsize = pip->i_extsize;
   0xffffffffa0375764 <+1988>:  mov    %esi,0x15c(%r13)
   0xffffffffa037576b <+1995>:  movzwl 0x166(%r12),%eax
   0xffffffffa0375774 <+2004>:  jmp    0xffffffffa0375721 <xfs_dir_ialloc+1921>

702                     }
703                     if (pip->i_diflags & XFS_DIFLAG_PROJINHERIT)
   0xffffffffa0375721 <+1921>:  mov    %ecx,%r8d
   0xffffffffa0375724 <+1924>:  or     $0x200,%r8d
   0xffffffffa037572b <+1931>:  test   $0x2,%ah
   0xffffffffa037572e <+1934>:  cmovne %r8d,%ecx
   0xffffffffa0375732 <+1938>:  jmpq   0xffffffffa03755e7 <xfs_dir_ialloc+1607>

704                             di_flags |= XFS_DIFLAG_PROJINHERIT;
705             } else if (S_ISREG(mode)) {

With a snippet at the bottom to copy %cx back to the inode:

730                     di_flags |= XFS_DIFLAG_FILESTREAM;
731
732             ip->i_diflags |= di_flags;
   0xffffffffa0375680 <+1760>:  or     0x166(%r13),%cx
   0xffffffffa0375688 <+1768>:  mov    %cx,0x166(%r13)

With the patch applied, that code becomes:

698                     if (pip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) {
   0xffffffffa0375751 <+1969>:  test   $0x10,%ah
   0xffffffffa0375754 <+1972>:  jne    0xffffffffa03757d0 <xfs_dir_ialloc+2096>
   0xffffffffa0375756 <+1974>:  mov    0x15c(%r13),%esi

699                             ip->i_diflags |= XFS_DIFLAG_EXTSZINHERIT;
   0xffffffffa03757d0 <+2096>:  or     $0x10,%ch
   0xffffffffa03757d3 <+2099>:  mov    %cx,0x166(%r13)

700                             ip->i_extsize = pip->i_extsize;
   0xffffffffa03757db <+2107>:  mov    0x15c(%r12),%esi
   0xffffffffa03757e3 <+2115>:  mov    %esi,0x15c(%r13)
   0xffffffffa03757ea <+2122>:  movzwl 0x166(%r12),%eax
   0xffffffffa03757f3 <+2131>:  jmpq   0xffffffffa037575d <xfs_dir_ialloc+1981>
   0xffffffffa03757f8 <+2136>:  callq  0xffffffff81713cb0 <__stack_chk_fail>
   0xffffffffa03757fd:  nopl   (%rax)

701                     }
702                     if (pip->i_diflags & XFS_DIFLAG_PROJINHERIT)
   0xffffffffa037575d <+1981>:  test   $0x2,%ah
   0xffffffffa0375760 <+1984>:  je     0xffffffffa03755f2 <xfs_dir_ialloc+1618>

703                             ip->i_diflags |= XFS_DIFLAG_PROJINHERIT;
   0xffffffffa0375766 <+1990>:  or     $0x2,%ch
   0xffffffffa0375769 <+1993>:  mov    %cx,0x166(%r13)
   0xffffffffa0375771 <+2001>:  movzwl 0x166(%r12),%eax
   0xffffffffa037577a <+2010>:  jmpq   0xffffffffa03755f2 <xfs_dir_ialloc+1618>

704             } else if (S_ISREG(mode)) {

AFAICT the main difference between the two versions now is that the new
version will copy %cx back to ip->i_diflags after every set operation,
and the old version would sometimes use a conditional move to update
%cx instead of a conditional branch.

I suspect the old version /is/ more efficient on x86, but I didn't see
any measurable difference between the two versions.  Hm.  The old code
was 197 bytes vs. 243 for the new one, so fmeh, I'll drop this because
I'd rather move on to the iwalk pre-cleanup cleanup series.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
diff mbox series

Patch

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;
 		}
 		if (pip->i_diflags & XFS_DIFLAG_PROJINHERIT)
-			di_flags |= XFS_DIFLAG_PROJINHERIT;
+			ip->i_diflags |= XFS_DIFLAG_PROJINHERIT;
 	} else if (S_ISREG(mode)) {
 		if ((pip->i_diflags & XFS_DIFLAG_RTINHERIT) &&
 		    xfs_sb_version_hasrealtime(&ip->i_mount->m_sb))
-			di_flags |= XFS_DIFLAG_REALTIME;
+			ip->i_diflags |= XFS_DIFLAG_REALTIME;
 		if (pip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) {
-			di_flags |= XFS_DIFLAG_EXTSIZE;
+			ip->i_diflags |= XFS_DIFLAG_EXTSIZE;
 			ip->i_extsize = pip->i_extsize;
 		}
 	}
 	if ((pip->i_diflags & XFS_DIFLAG_NOATIME) &&
 	    xfs_inherit_noatime)
-		di_flags |= XFS_DIFLAG_NOATIME;
+		ip->i_diflags |= XFS_DIFLAG_NOATIME;
 	if ((pip->i_diflags & XFS_DIFLAG_NODUMP) &&
 	    xfs_inherit_nodump)
-		di_flags |= XFS_DIFLAG_NODUMP;
+		ip->i_diflags |= XFS_DIFLAG_NODUMP;
 	if ((pip->i_diflags & XFS_DIFLAG_SYNC) &&
 	    xfs_inherit_sync)
-		di_flags |= XFS_DIFLAG_SYNC;
+		ip->i_diflags |= XFS_DIFLAG_SYNC;
 	if ((pip->i_diflags & XFS_DIFLAG_NOSYMLINKS) &&
 	    xfs_inherit_nosymlinks)
-		di_flags |= XFS_DIFLAG_NOSYMLINKS;
+		ip->i_diflags |= XFS_DIFLAG_NOSYMLINKS;
 	if ((pip->i_diflags & XFS_DIFLAG_NODEFRAG) &&
 	    xfs_inherit_nodefrag)
-		di_flags |= XFS_DIFLAG_NODEFRAG;
+		ip->i_diflags |= XFS_DIFLAG_NODEFRAG;
 	if (pip->i_diflags & XFS_DIFLAG_FILESTREAM)
-		di_flags |= XFS_DIFLAG_FILESTREAM;
-
-	ip->i_diflags |= di_flags;
+		ip->i_diflags |= XFS_DIFLAG_FILESTREAM;
 
 	/*
 	 * Inode verifiers on older kernels only check that the extent size