Re: [Jfs-discussion] [fs] 05c5a0273b: netperf.Throughput_total_tps -71.8% regression - Jia-Ju Bai

From: Jia-Ju Bai <baijiaju1990@gmail.com>
To: Hillf Danton <hdanton@sina.com>, Rong Chen <rong.a.chen@intel.com>
Cc: Christian Kujau <lists@nerdbynature.de>,
	shaggy@kernel.org, jfs-discussion@lists.sourceforge.net,
	linux-kernel@vger.kernel.org, Markus.Elfring@web.de
Subject: Re: [Jfs-discussion] [fs] 05c5a0273b: netperf.Throughput_total_tps -71.8% regression
Date: Fri, 15 May 2020 15:34:39 +0800	[thread overview]
Message-ID: <096463bb-cef1-495b-5ef9-460f8f41fffb@gmail.com> (raw)
In-Reply-To: <20200514154251.18184-1-hdanton@sina.com>

On 2020/5/14 23:42, Hillf Danton wrote:
> On Thu, 14 May 2020 13:12:18 +0800 Rong Chen wrote:
>> On 5/14/20 12:27 PM, Christian Kujau wrote:
>>> On Tue, 12 May 2020, kernel test robot wrote:
>>>> FYI, we noticed a -71.8% regression of netperf.Throughput_total_tps due to commit:
>>> As noted in this report, netperf is used to "measure various aspect of
>>> networking performance". Are we sure the bisect is correct? JFS is a
>>> filesystem and is not touching net/ in any way. So, having not attempted
>>> to reproduce this, maybe the JFS commit is a red herring?
>>>
>>> C.
>> Hi,
>>
>> The commit also causes -74.6% regression of will-it-scale.per_thread_ops:
>>
>> in testcase: will-it-scale
>> on test machine: 8 threads Intel(R) Core(TM) i7-3770K CPU @ 3.50GHz with 16G memory
>> with following parameters:
>>
>> 	nr_task: 100%
>> 	mode: thread
>> 	test: unlink2
>> 	cpufreq_governor: performance
>> 	ucode: 0x21
>>
>> I'll send another report for this regression.
>>
>> Best Regards,
>> Rong Chen
> Hi
>
> Would it make sense in terms of making robot and fuzzer happy to replace
> spin lock with memory barrier, say the changes below?
>
> Hillf
>
> --- a/fs/jfs/jfs_txnmgr.c
> +++ b/fs/jfs/jfs_txnmgr.c
> @@ -416,6 +416,7 @@ tid_t txBegin(struct super_block *sb, in
>   	 * memset(tblk, 0, sizeof(struct tblock));
>   	 */
>   	tblk->next = tblk->last = tblk->xflag = tblk->flag = tblk->lsn = 0;
> +	smp_wmb(); /* match mb in txLazyCommit() */
>   
>   	tblk->sb = sb;
>   	++log->logtid;
> @@ -2683,10 +2684,16 @@ static void txLazyCommit(struct tblock *
>   {
>   	struct jfs_log *log;
>   
> -	while (((tblk->flag & tblkGC_READY) == 0) &&
> -	       ((tblk->flag & tblkGC_UNLOCKED) == 0)) {
> -		/* We must have gotten ahead of the user thread
> -		 */
> +	for (;;) {
> +		u16 flag = tblk->flag;
> +
> +		smp_rmb(); /* match mb in txBegin() */
> +		if (flag & tblkGC_READY)
> +			break;
> +		if (flag & tblkGC_UNLOCKED)
> +			break;
> +
> +		/* We must have gotten ahead of the user thread */
>   		jfs_info("jfs_lazycommit: tblk 0x%p not unlocked", tblk);
>   		yield();
>   	}
> @@ -2698,9 +2705,9 @@ static void txLazyCommit(struct tblock *
>   	log = (struct jfs_log *) JFS_SBI(tblk->sb)->log;
>   
>   	spin_lock_irq(&log->gclock);	// LOGGC_LOCK
> -
> +	smp_mb__after_spinlock();
>   	tblk->flag |= tblkGC_COMMITTED;
> -
> +	smp_wmb();
>   	if (tblk->flag & tblkGC_READY)
>   		log->gcrtc--;
>   
>

I think this patch is okay.
Thanks a lot, Hillf :)

Best wishes,
Jia-Ju Bai