Re: [mm] 8cc621d2f4: fio.write_iops -21.8% regression

[Xing, Zhengjun <zhengjun.xing@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7616 bytes --]

Hi Minchan,

On 5/21/2021 2:36 AM, Minchan Kim wrote:
> On Thu, May 20, 2021 at 04:31:44PM +0800, kernel test robot wrote:
>>
>> Greeting,
>>
>> FYI, we noticed a -21.8% regression of fio.write_iops due to commit:
>>
>>
>> commit: 8cc621d2f45ddd3dc664024a647ee7adf48d79a5 ("mm: fs: invalidate BH LRU during page migration")
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>>
>>
>> in testcase: fio-basic
>> on test machine: 96 threads 2 sockets Intel(R) Xeon(R) Gold 6252 CPU @ 2.10GHz with 256G memory
>> with following parameters:
>>
>> 	disk: 2pmem
>> 	fs: ext4
>> 	runtime: 200s
>> 	nr_task: 50%
>> 	time_based: tb
>> 	rw: randwrite
>> 	bs: 4k
>> 	ioengine: libaio
>> 	test_size: 200G
>> 	cpufreq_governor: performance
>> 	ucode: 0x5003006
>>
>> test-description: Fio is a tool that will spawn a number of threads or processes doing a particular type of I/O action as specified by the user.
>> test-url: https://github.com/axboe/fio
>>
>>
>>
>> If you fix the issue, kindly add following tag
>> Reported-by: kernel test robot <oliver.sang@intel.com>
>>
>>
>> Details are as below:
>> -------------------------------------------------------------------------------------------------->
>>
>>
>> To reproduce:
>>
>>          git clone https://github.com/intel/lkp-tests.git
>>          cd lkp-tests
>>          bin/lkp install                job.yaml  # job file is attached in this email
>>          bin/lkp split-job --compatible job.yaml  # generate the yaml file for lkp run
>>          bin/lkp run                    generated-yaml-file
> Hi,
>
> I tried to insall the lkp-test in my machine by following above guide but failed
> due to package problems(I guess it's my problem since I use something particular
> environement). However, I guess it comes from increased miss ratio of bh_lrus
> since the patch caused more frequent invalidation of the bh_lrus calls compared
> to old. For example, lru_add_drain could be called from several hot places(e.g.,
> unmap and pagevec_release from several path) and it could keeps invalidating
> bh_lrus.
>
> IMO, we should move the overhead from such hot path to cold one. How about this?
>
> >From ebf4ede1cf32fb14d85f0015a3693cb8e1b8dbfe Mon Sep 17 00:00:00 2001
> From: Minchan Kim <minchan@kernel.org>
> Date: Thu, 20 May 2021 11:17:56 -0700
> Subject: [PATCH] invalidate bh_lrus only at lru_add_drain_all
>
> Not-Yet-Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>   mm/swap.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/mm/swap.c b/mm/swap.c
> index dfb48cf9c2c9..d6168449e28c 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -642,7 +642,6 @@ void lru_add_drain_cpu(int cpu)
>   		pagevec_lru_move_fn(pvec, lru_lazyfree_fn);
>   
>   	activate_page_drain(cpu);
> -	invalidate_bh_lrus_cpu(cpu);
>   }
>   
>   /**
> @@ -725,6 +724,17 @@ void lru_add_drain(void)
>   	local_unlock(&lru_pvecs.lock);
>   }
>   
> +void lru_and_bh_lrus_drain(void)
> +{
> +	int cpu;
> +
> +	local_lock(&lru_pvecs.lock);
> +	cpu = smp_processor_id();
> +	lru_add_drain_cpu(cpu);
> +	local_unlock(&lru_pvecs.lock);
> +	invalidate_bh_lrus_cpu(cpu);
> +}
> +
>   void lru_add_drain_cpu_zone(struct zone *zone)
>   {
>   	local_lock(&lru_pvecs.lock);
> @@ -739,7 +749,7 @@ static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
>   
>   static void lru_add_drain_per_cpu(struct work_struct *dummy)
>   {
> -	lru_add_drain();
> +	lru_and_bh_lrus_drain();
>   }
>   
>   /*
> @@ -881,6 +891,7 @@ void lru_cache_disable(void)
>   	__lru_add_drain_all(true);
>   #else
>   	lru_add_drain();
> +	invalidate_bh_lrus_cpu(smp_processor_id());
>   #endif
>   }
>   
I test the patch, the regression reduced to -2.9%.
=========================================================================================
tbox_group/testcase/rootfs/kconfig/compiler/disk/fs/runtime/nr_task/time_based/rw/bs/ioengine/test_size/cpufreq_governor/ucode:
lkp-csl-2sp6/fio-basic/debian-10.4-x86_64-20200603.cgz/x86_64-rhel-8.3/gcc-9/2pmem/ext4/200s/50%/tb/randwrite/4k/libaio/200G/performance/0x5003006

commit:
   361a2a229fa31ab7f2b236b5946e434964d00762
   8cc621d2f45ddd3dc664024a647ee7adf48d79a5
   7bafb1c4e72a5e8d40cd0681b80b1ffeb31a064c  (the test patch)

361a2a229fa31ab7 8cc621d2f45ddd3dc664024a647 7bafb1c4e72a5e8d40cd0681b80
---------------- --------------------------- ---------------------------
          %stddev     %change         %stddev     %change %stddev
              \          |                \          | \
       0.01            -0.0        0.00            +0.0 0.01        
fio.latency_1000ms%
       0.27 ±  5%      -0.0        0.24 ±  7%      -0.0        0.27 ±  
5%  fio.latency_10ms%
       0.01            -0.0        0.00            +0.0 0.01        
fio.latency_2000ms%
       0.33 ±  8%      +0.1        0.44 ±  7%      +0.0        0.36 ±  
9%  fio.latency_20ms%
       0.01            -0.0        0.00            +0.0 0.01        
fio.latency_500ms%
      13.99            +6.0       19.96            +0.5       14.52 ±  
4%  fio.latency_50ms%
       0.01            -0.0        0.00            +0.0 0.01        
fio.latency_750ms%
       3.40 ± 44%      -2.2        1.17 ± 46%      -0.8        2.59 ± 
35%  fio.latency_750us%
   62087973           -21.8%   48559860            -2.8%   60364160 ±  
3%  fio.workload
       1212           -21.8%     948.29            -2.9%       1177 ±  
3%  fio.write_bw_MBps
   33073834           -11.6%   29229056            -1.3%   32636928 ±  
4%  fio.write_clat_95%_us
   34865152            -9.1%   31675733            -0.3% 34760294        
fio.write_clat_99%_us
    4794125           +27.9%    6129631            +3.0%    4936772 ±  
3%  fio.write_clat_mean_us
   13179671 ±  6%     -10.1%   11842274            -3.9%   12665456 ±  
5%  fio.write_clat_stddev
     310403           -21.8%     242761            -2.9%     301531 ±  
3%  fio.write_iops
     152653           +28.9%     196759            +3.2%     157562 ±  
3%  fio.write_slat_mean_us
    2139749            +9.3%    2338598            +1.8% 2179116        
fio.write_slat_stddev

When build kernel, there is a warning for the patch:

All warnings (new ones prefixed by >>):

>> mm/swap.c:727:6: warning: no previous prototype for 'lru_and_bh_lrus_drain' [-Wmissing-prototypes]

      727 | void lru_and_bh_lrus_drain(void)
          |      ^~~~~~~~~~~~~~~~~~~~~


vim +/lru_and_bh_lrus_drain +727 mm/swap.c

    726	
  > 727	void lru_and_bh_lrus_drain(void)
    728	{
    729		int cpu;
    730	
    731		local_lock(&lru_pvecs.lock);
    732		cpu = smp_processor_id();
    733		lru_add_drain_cpu(cpu);
    734		local_unlock(&lru_pvecs.lock);
    735		invalidate_bh_lrus_cpu(cpu);
    736	}
    737	

-- 
Zhengjun Xing


[-- Attachment #2: attachment.htm --]
[-- Type: text/html, Size: 8875 bytes --]