linux-kernel - Re: [linus:master] [xfs] 2edf06a50f: fsmark.files_per

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZGDyAOewWqjY5xvJ@feng-clx>
Date:   Sun, 14 May 2023 22:36:48 +0800
From:   Feng Tang <feng.tang@...el.com>
To:     Dave Chinner <david@...morbit.com>
CC:     Oliver Sang <oliver.sang@...el.com>,
        Dave Chinner <dchinner@...hat.com>, <oe-lkp@...ts.linux.dev>,
        <lkp@...el.com>, <linux-kernel@...r.kernel.org>,
        "Darrick J. Wong" <djwong@...nel.org>, <linux-xfs@...r.kernel.org>,
        <ying.huang@...el.com>, <fengwei.yin@...el.com>
Subject: Re: [linus:master] [xfs]  2edf06a50f:  fsmark.files_per_sec -5.7%
 regression

Hi Dave,

On Sat, May 13, 2023 at 09:05:04AM +1000, Dave Chinner wrote:
> On Fri, May 12, 2023 at 03:44:29PM +0800, Oliver Sang wrote:
[...]
> > Thanks a lot for guidance!
> > 
> > we plan to disable XFS_DEBUG (as well as XFS_WARN) in our performance tests.
> > want to consult with you if this is the correct thing to do?
> 
> You can use XFS_WARN=y with performance tests - that elides all the
> debug specific code that changes behaviour but leaves all the
> ASSERT-based correctness checks in the code.
> 
> > and I guess we should still keep them in functional tests, am I right?
> 
> Yes.
> 
> > BTW, regarding this case, we tested again with disabling XFS_DEBUG (as well as
> > XFS_WARN), kconfig is attached, only diff with last time is:
> > -CONFIG_XFS_DEBUG=y
> > -CONFIG_XFS_ASSERT_FATAL=y
> > +# CONFIG_XFS_WARN is not set
> > +# CONFIG_XFS_DEBUG is not set
> > 
> > but we still observed similar regression:
> > 
> > ecd788a92460eef4 2edf06a50f5bbe664283f3c55c4
> > ---------------- ---------------------------
> >          %stddev     %change         %stddev
> >              \          |                \
> >    8176057 ± 15%      +4.7%    8558110        fsmark.app_overhead
> >      14484            -6.3%      13568        fsmark.files_per_sec
> 
> So the application spent 5% more CPU time in userspace, and the rate
> the kernel processed IO went down by 6%. Seems to me like
> everything is running slower, not just the kernel code....
> 
> >     100.50 ±  5%      +0.3%     100.83        turbostat.Avg_MHz
> >       5.54 ± 11%      +0.3        5.82        turbostat.Busy%
> >       1863 ± 19%      -6.9%       1733        turbostat.Bzy_MHz
> 
> Evidence that the CPU is running at a 7% lower clock rate when the
> results are 6% slower is a bit suspicious to me. Shouldn't the CPU
> clock rate be fixed to the same value for A-B performance regression
> testing?

For commit 2edf06a50f5, it seems to change the semantics a little
about handling of 'flags' for xfs_alloc_fix_freelist(). With the debug
below, the performance is restored.


ecd788a92460eef4 2edf06a50f5bbe664283f3c55c4 68721405630744da1c07c9c1c3c 
---------------- --------------------------- --------------------------- 

     14349            -5.7%      13527            +0.6%      14437        fsmark.files_per_sec
    486.29            +5.8%     514.28            -0.5%     483.70        fsmark.time.elapsed_time

Please help to review if the debug patch miss anything as I don't
know the internals of xfs, thanks.

---
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 98defd19e09e..8c85cc68c5f4 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -3246,12 +3246,12 @@ xfs_alloc_vextent_set_fsbno(
  */
 static int
 __xfs_alloc_vextent_this_ag(
-	struct xfs_alloc_arg	*args)
+	struct xfs_alloc_arg	*args, int flag)
 {
 	struct xfs_mount	*mp = args->mp;
 	int			error;
 
-	error = xfs_alloc_fix_freelist(args, 0);
+	error = xfs_alloc_fix_freelist(args, flag);
 	if (error) {
 		trace_xfs_alloc_vextent_nofix(args);
 		return error;
@@ -3289,7 +3289,7 @@ xfs_alloc_vextent_this_ag(
 	}
 
 	args->pag = xfs_perag_get(mp, args->agno);
-	error = __xfs_alloc_vextent_this_ag(args);
+	error = __xfs_alloc_vextent_this_ag(args, 0);
 
 	xfs_alloc_vextent_set_fsbno(args, minimum_agno);
 	xfs_perag_put(args->pag);
@@ -3329,7 +3329,7 @@ xfs_alloc_vextent_iterate_ags(
 	args->agno = start_agno;
 	for (;;) {
 		args->pag = xfs_perag_get(mp, args->agno);
-		error = __xfs_alloc_vextent_this_ag(args);
+		error = __xfs_alloc_vextent_this_ag(args, flags);
 		if (error) {
 			args->agbno = NULLAGBLOCK;
 			break;


Also for the turbostat.Bzy_MHz diff, IIUC, 0Day always uses
'performance' cpufreq governor. And as the test case is running
32 thread in a platform with 96 CPUs, there are many CPUs in idle
state in average, and I suspect the Bzy_MHz may be calculated 
considering those cpufreq and cpuidle factors.

Thanks,
Feng

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@...morbit.com