linux-kernel - Re: [Jfs-discussion] [fs] 05c5a0273b: netperf.Throughput_total

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

Date:   Fri, 15 May 2020 15:34:39 +0800
From:   Jia-Ju Bai <baijiaju1990@...il.com>
To:     Hillf Danton <hdanton@...a.com>, Rong Chen <rong.a.chen@...el.com>
Cc:     Christian Kujau <lists@...dbynature.de>, shaggy@...nel.org,
        jfs-discussion@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
        Markus.Elfring@....de
Subject: Re: [Jfs-discussion] [fs] 05c5a0273b: netperf.Throughput_total_tps
 -71.8% regression



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