Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 14 May 2023 22:36:48 +0800
From:   Feng Tang <feng.tang@intel.com>
To:     Dave Chinner <david@fromorbit.com>
CC:     Oliver Sang <oliver.sang@intel.com>,
        Dave Chinner <dchinner@redhat.com>, <oe-lkp@lists.linux.dev>,
        <lkp@intel.com>, <linux-kernel@vger.kernel.org>,
        "Darrick J. Wong" <djwong@kernel.org>, <linux-xfs@vger.kernel.org>,
        <ying.huang@intel.com>, <fengwei.yin@intel.com>
Subject: Re: [linus:master] [xfs]  2edf06a50f:  fsmark.files_per_sec -5.7%
 regression
Message-ID: <ZGDyAOewWqjY5xvJ@feng-clx>
References: <202305090905.aff4e0e6-oliver.sang@intel.com>
 <20230509065433.GT3223426@dread.disaster.area>
 <20230509071053.GE2651828@dread.disaster.area>
 <ZF3uXe+cjAsfCLic@xsang-OptiPlex-9020>
 <20230512230504.GF3223426@dread.disaster.area>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230512230504.GF3223426@dread.disaster.area>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RXZnaDM2Zzd2d3UxaUVHWit1eFBKcGVyTTdBTEJ3T3BFTVkzZnRDQnlxSW5a?=
 =?utf-8?B?a3NlYW9GV0lHd1B0RytVK0taZFBZRU15UjRCNVNVemY4QitmcGVESS96Q2Jx?=
 =?utf-8?B?VC9sN3dZSmtuYWU4YytkUkxMNTZCUEphZ3BadnhBMk9WMlpVa2NON2UydkJL?=
 =?utf-8?B?bFNEZFozUUFyV2ErRlZRNUxoNkR1WE9wQ1lzYi95cksyeDl0S05CYmVXTDdo?=
 =?utf-8?B?Qm5FU253RHovaUJrZndlQk94TjgyM05HVllBR3Y4QVh6S0Z1NWlIdUY5cm9p?=
 =?utf-8?B?NTZ6NldKM0xJUUlGUWZETmY1NjZvMUVZQ2V2SFpBY0JuOE1xclVoaEcvVldC?=
 =?utf-8?B?SXBBRk5DMit5ayt2UncwQVQwVU1ibm9DV0I3ZjNSMmRBQUN2NzBqOTY3RVRy?=
 =?utf-8?B?UHZBZGhZaVN4QlVDSnhucFF1VHBkQjg5WDRZZzgzaXQ1WlNkczNRMkxOclh2?=
 =?utf-8?B?ZW5Uc2M0cEQrQ1M2Umx5bWpVRzlYa0JNUnhqYnhnWkV6OUxVbEpYalVEOEZH?=
 =?utf-8?B?ZU1RbXppbG10S0tjNVo0aDlrWHJOWVpiNWFnN01mTUtWZzd5aEh4ODhFQllZ?=
 =?utf-8?B?djc1SUJpcFpvbklBaG1sVTJ3SU5vQkkrTUVYSTRIMWRNR1pjb0w3b0ZsKzBO?=
 =?utf-8?B?eUY4V0ZqMjVYbENCcTFEVG92SHlNTmVuVFJWUThOTHJKaEhOdTI1WTNqKy9O?=
 =?utf-8?B?VDhmWkszTm1zQzFSVkJDZzRyUHZMcHpJSUducUhJa2dlWkhkQnJiL3B3eERO?=
 =?utf-8?B?M1lOVDJuRFNiZDFlRGFJaFlGQWgrY3JvSksrTHVjSjRmbm1iTkNwWi9ab1RI?=
 =?utf-8?B?NlI3eXVIb290VlBWNVhieXp5ZGFzK2R0M2JoVythb244cUtmcmwvMTR6aXdB?=
 =?utf-8?B?VWhTaXBGbDRtaTQrTThHYThVTWNmcWhaQnkwaTdKSGI2SE1zdFZTYjMvUlBZ?=
 =?utf-8?B?L0R5T2VrdzB3ejhCMEFLTmduSCtlMmtaR3piT01aOW0yTlphRkJXVnZjaFM5?=
 =?utf-8?B?WlkxRnBTR0cvMU5qTkhhMlEwL2ZNUkN6UjlBaXl5dXpBMmw3S2ZiYUh1R0lC?=
 =?utf-8?B?TXBldGRWRWgrTlZJYkZ1RWN6SzRkRkliaGlEczNHTHpyYmRRb2lTUkwrT2la?=
 =?utf-8?B?Q1h6NGdWK2N6T0FXS2taZWQrclVtdE13bUNKeXZtVDNPMDZwNVRuVlhDY1NZ?=
 =?utf-8?B?VVQxR1dwRGZ6Y2c1Yk1wS214RVdXOTlaTytHWFdVdi9LVDNOYVJjNVp4WlRp?=
 =?utf-8?B?U1RPNlY5V1RjQndrUFo0N0ZKS3lKR2pBbERGYi9Qb3JhNHF3dzZKT25BSzI5?=
 =?utf-8?B?SXFsSE1sendSeVRMMVg4aHlEUG8vMWZVVXNRTjVPSXpiV3A1R2dOZGR2SjN4?=
 =?utf-8?B?UTJjREQycHJ1b0NDMUttcWhsRkc2R005Tks0Q3NUMGh1R0hvVEZFUUYrZXBN?=
 =?utf-8?B?NUFDeFJVOS80elptbEV1NDN5ZUkxRWJlSVgyc0RqUW5vSkpSeWFZNkdrZmMx?=
 =?utf-8?B?Z1hLQ3dEQ3VZRHRMaGtMRzBKRDNsR2c5Q1ZvVWpJNWsyZEFFUDRyRXdlRzVR?=
 =?utf-8?B?cWlPRGsxaGpWOG9GU1lacTlsYnJrWDR4NzJMcW0vZDRheCtBVm9SRzRGYVlM?=
 =?utf-8?B?MW5RUHRhTUpNb3BDRUVzSUQzNndKUkZLNDRHTTJjZ29mcElMZmVnSW5uY3V3?=
 =?utf-8?B?UTJSa3NpUXFZQlVZczJXeWZ4RW9GcDAxdEFwSHZBU2JUQ1RZdEFsWkxWalBG?=
 =?utf-8?B?LzRCekJIL2lXdHlDYnpKWlpNeVRTQTJzeUExWTB5ZWpTN1lzQmR1R1M3WWZ3?=
 =?utf-8?B?TFpJSThCSlJzUUhETVFvREN3Q2UzcHdUOEpYTm55QlRMc25uUWMwMGx4QVlv?=
 =?utf-8?B?aE1XbkR3bzZmTDdIalZzazZtOERSUTlrR0JZbmdjQm54V0E3VTVBbndtbHBQ?=
 =?utf-8?B?dHRwZFEvYTFJOEU4UWlDR3haUUZuVEZtdU9MUVhsdDBOeXh0NUlPVHpUTmN5?=
 =?utf-8?B?ekFKM2hidDZZVnBZSkdkM0JEUjJOUGVZYllpb00yb1ltOFp5NFc5bm5MR0Ix?=
 =?utf-8?B?WjdsaktOSStUbFRUWGJvemc3K2FadkxnRDc3QnlVVDUwd3gyTjdPNDNBZDlU?=
 =?utf-8?Q?Ez4LT+NU6y+lEFpUPNSL+S/Zb?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c10ac80d-e7f7-4d57-1a24-08db54897581
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 May 2023 14:42:36.4222
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: S22MbBJv1DoMcczk7oPB/EHCXXyYjToHMys0j+IuBZequuPvhLDXg3cIsHLiFYBF7HiCJ0wihFAa4jDtbO5BtA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB8328
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@fromorbit.com