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

[Jia-Ju Bai]

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