From: "Yin, Fengwei" <fengwei.yin@intel.com> To: Linus Torvalds <torvalds@linux-foundation.org>, kernel test robot <oliver.sang@intel.com> Cc: Mikulas Patocka <mpatocka@redhat.com>, <lkp@lists.01.org>, <lkp@intel.com>, Matthew Wilcox <willy@infradead.org>, <linux-kernel@vger.kernel.org>, <regressions@lists.linux.dev> Subject: Re: [LKP] Re: d4252071b9: fxmark.ssd_ext4_no_jnl_DWTL_54_directio.works/sec -26.5% regression Date: Thu, 8 Sep 2022 16:55:47 +0800 [thread overview] Message-ID: <0214b84c-71fe-2133-b69d-1e39a19cc468@intel.com> (raw) In-Reply-To: <CAHk-=wgG=mttS-m2OLcnsTwia2roHR2b-DxXXG-tbDH8_cUNiA@mail.gmail.com> Hi Linus, On 9/1/2022 12:46 AM, Linus Torvalds wrote: > So I think we can just do this: > > --- a/include/linux/buffer_head.h > +++ b/include/linux/buffer_head.h > @@ -137,12 +137,14 @@ BUFFER_FNS(Defer_Completion, defer_completion) > > static __always_inline void set_buffer_uptodate(struct buffer_head *bh) > { > - /* > - * make it consistent with folio_mark_uptodate > - * pairs with smp_load_acquire in buffer_uptodate > - */ > - smp_mb__before_atomic(); > - set_bit(BH_Uptodate, &bh->b_state); > + if (!test_bit(BH_Uptodate, &bh->b_state)) { > + /* > + * make it consistent with folio_mark_uptodate > + * pairs with smp_load_acquire in buffer_uptodate > + */ > + smp_mb__before_atomic(); > + set_bit(BH_Uptodate, &bh->b_state); > + } > } > > static __always_inline void clear_buffer_uptodate(struct buffer_head *bh) > > and re-introduce the original code (maybe extend that comment to talk > about this "only first up-to-date matters". Test result: commit: e394ff83bbca1c72427b1feb5c6b9d4dad832f01 -> parent of bad commit d4252071b97d2027d246f6a82cbee4d52f618b47 -> bad commit 6812880b7af46dc2a78ad2069e5279adcbfd5133 -> The fixing patch commit e394ff83bbca1c72 d4252071b97d2027d246f6a82cb 6812880b7af46dc2a78ad2069e5 ---------------- --------------------------- --------------------------- %stddev %change %stddev %change %stddev \ | \ | \ 0.01 ± 3% +30.7% 0.01 ± 4% -2.5% 0.01 ± 3% fxmark.ssd_ext4_no_jnl_DWTL_18_directio.secs 0.14 ± 3% +29.3% 0.18 ± 5% -4.9% 0.13 ± 6% fxmark.ssd_ext4_no_jnl_DWTL_18_directio.sys_sec 20.21 ± 4% +16.6% 23.58 -7.7% 18.66 ± 5% fxmark.ssd_ext4_no_jnl_DWTL_18_directio.sys_util 3377886 ± 3% -23.4% 2586083 ± 4% +2.6% 3466796 ± 3% fxmark.ssd_ext4_no_jnl_DWTL_18_directio.works/sec 0.06 +15.9% 0.07 ± 2% +2.7% 0.06 fxmark.ssd_ext4_no_jnl_DWTL_2_directio.real_sec 0.03 +24.9% 0.04 ± 3% +3.4% 0.03 fxmark.ssd_ext4_no_jnl_DWTL_2_directio.secs 0.07 +23.8% 0.09 ± 5% -4.8% 0.07 ± 7% fxmark.ssd_ext4_no_jnl_DWTL_2_directio.sys_sec 55.34 ± 3% +7.1% 59.26 ± 3% -7.3% 51.28 ± 7% fxmark.ssd_ext4_no_jnl_DWTL_2_directio.sys_util 738881 -19.9% 592194 ± 3% -3.3% 714200 fxmark.ssd_ext4_no_jnl_DWTL_2_directio.works/sec 38.31 ± 3% -20.0% 30.64 ± 9% -15.8% 32.27 ± 8% fxmark.ssd_ext4_no_jnl_DWTL_4_directio.idle_util 0.02 +30.0% 0.03 ± 5% +4.6% 0.02 fxmark.ssd_ext4_no_jnl_DWTL_4_directio.secs 0.08 ± 5% +32.0% 0.11 +16.0% 0.10 ± 12% fxmark.ssd_ext4_no_jnl_DWTL_4_directio.sys_sec 41.65 ± 2% +22.1% 50.86 ± 4% +6.7% 44.43 ± 7% fxmark.ssd_ext4_no_jnl_DWTL_4_directio.sys_util 1163070 -22.9% 896752 ± 5% -4.4% 1111656 fxmark.ssd_ext4_no_jnl_DWTL_4_directio.works/sec 1.32 ± 18% -16.6% 1.10 ± 22% -22.0% 1.03 ± 3% fxmark.ssd_ext4_no_jnl_DWTL_54_directio.irq_util 0.00 ± 2% +25.1% 0.01 ± 8% -1.2% 0.00 ± 4% fxmark.ssd_ext4_no_jnl_DWTL_54_directio.secs 0.24 ± 3% +37.5% 0.33 ± 4% +16.7% 0.28 ± 2% fxmark.ssd_ext4_no_jnl_DWTL_54_directio.sys_sec 11.85 ± 4% +30.8% 15.50 ± 4% +21.7% 14.41 ± 6% fxmark.ssd_ext4_no_jnl_DWTL_54_directio.sys_util 5031984 ± 2% -19.5% 4049295 ± 8% +1.3% 5099506 ± 4% fxmark.ssd_ext4_no_jnl_DWTL_54_directio.works/sec 0.00 ± 3% +37.5% 0.01 ± 11% +13.8% 0.00 ± 9% fxmark.ssd_ext4_no_jnl_DWTL_72_directio.secs 0.33 ± 9% +29.0% 0.43 ± 8% +12.0% 0.37 ± 8% fxmark.ssd_ext4_no_jnl_DWTL_72_directio.sys_sec 12.11 ± 10% +21.9% 14.76 ± 6% +12.5% 13.63 ± 7% fxmark.ssd_ext4_no_jnl_DWTL_72_directio.sys_util 5533529 ± 4% -26.3% 4078851 ± 12% -11.5% 4896500 ± 8% fxmark.ssd_ext4_no_jnl_DWTL_72_directio.works/sec The test result looks good (only test with 72 process doesn't restore to same level as original result). You may notice the test with 36 process are not showed here. It is because of the lkp script problem and we are working on it. Checked the test result of 36 process manually: e394ff83bbca1c72: avg = 4267358.820936666 -> the parent of bad commit d4252071b97d2027: avg = 3821149.8479718883 -> the bad commit 6812880b7af46dc2: avg = 4724775.219265333 -> the fixing patch commit It looks good also. > > HOWEVER. > > I'd love to hear if you have a clear profile change, and to see > exactly which set_buffer_uptodate() is *so* important. Honestly, I > didn't expect the buffer head functions to even really matter much any > more, with pretty much all IO being about the page cache.. Unfortunately, the valid profile data was not captured yet. We will keep working on it and share out once we get the valid profile data. Thanks. Regards Yin, Fengwei
WARNING: multiple messages have this Message-ID (diff)
From: Yin, Fengwei <fengwei.yin@intel.com> To: lkp@lists.01.org Subject: Re: d4252071b9: fxmark.ssd_ext4_no_jnl_DWTL_54_directio.works/sec -26.5% regression Date: Thu, 08 Sep 2022 16:55:47 +0800 [thread overview] Message-ID: <0214b84c-71fe-2133-b69d-1e39a19cc468@intel.com> (raw) In-Reply-To: <CAHk-=wgG=mttS-m2OLcnsTwia2roHR2b-DxXXG-tbDH8_cUNiA@mail.gmail.com> [-- Attachment #1: Type: text/plain, Size: 5536 bytes --] Hi Linus, On 9/1/2022 12:46 AM, Linus Torvalds wrote: > So I think we can just do this: > > --- a/include/linux/buffer_head.h > +++ b/include/linux/buffer_head.h > @@ -137,12 +137,14 @@ BUFFER_FNS(Defer_Completion, defer_completion) > > static __always_inline void set_buffer_uptodate(struct buffer_head *bh) > { > - /* > - * make it consistent with folio_mark_uptodate > - * pairs with smp_load_acquire in buffer_uptodate > - */ > - smp_mb__before_atomic(); > - set_bit(BH_Uptodate, &bh->b_state); > + if (!test_bit(BH_Uptodate, &bh->b_state)) { > + /* > + * make it consistent with folio_mark_uptodate > + * pairs with smp_load_acquire in buffer_uptodate > + */ > + smp_mb__before_atomic(); > + set_bit(BH_Uptodate, &bh->b_state); > + } > } > > static __always_inline void clear_buffer_uptodate(struct buffer_head *bh) > > and re-introduce the original code (maybe extend that comment to talk > about this "only first up-to-date matters". Test result: commit: e394ff83bbca1c72427b1feb5c6b9d4dad832f01 -> parent of bad commit d4252071b97d2027d246f6a82cbee4d52f618b47 -> bad commit 6812880b7af46dc2a78ad2069e5279adcbfd5133 -> The fixing patch commit e394ff83bbca1c72 d4252071b97d2027d246f6a82cb 6812880b7af46dc2a78ad2069e5 ---------------- --------------------------- --------------------------- %stddev %change %stddev %change %stddev \ | \ | \ 0.01 ± 3% +30.7% 0.01 ± 4% -2.5% 0.01 ± 3% fxmark.ssd_ext4_no_jnl_DWTL_18_directio.secs 0.14 ± 3% +29.3% 0.18 ± 5% -4.9% 0.13 ± 6% fxmark.ssd_ext4_no_jnl_DWTL_18_directio.sys_sec 20.21 ± 4% +16.6% 23.58 -7.7% 18.66 ± 5% fxmark.ssd_ext4_no_jnl_DWTL_18_directio.sys_util 3377886 ± 3% -23.4% 2586083 ± 4% +2.6% 3466796 ± 3% fxmark.ssd_ext4_no_jnl_DWTL_18_directio.works/sec 0.06 +15.9% 0.07 ± 2% +2.7% 0.06 fxmark.ssd_ext4_no_jnl_DWTL_2_directio.real_sec 0.03 +24.9% 0.04 ± 3% +3.4% 0.03 fxmark.ssd_ext4_no_jnl_DWTL_2_directio.secs 0.07 +23.8% 0.09 ± 5% -4.8% 0.07 ± 7% fxmark.ssd_ext4_no_jnl_DWTL_2_directio.sys_sec 55.34 ± 3% +7.1% 59.26 ± 3% -7.3% 51.28 ± 7% fxmark.ssd_ext4_no_jnl_DWTL_2_directio.sys_util 738881 -19.9% 592194 ± 3% -3.3% 714200 fxmark.ssd_ext4_no_jnl_DWTL_2_directio.works/sec 38.31 ± 3% -20.0% 30.64 ± 9% -15.8% 32.27 ± 8% fxmark.ssd_ext4_no_jnl_DWTL_4_directio.idle_util 0.02 +30.0% 0.03 ± 5% +4.6% 0.02 fxmark.ssd_ext4_no_jnl_DWTL_4_directio.secs 0.08 ± 5% +32.0% 0.11 +16.0% 0.10 ± 12% fxmark.ssd_ext4_no_jnl_DWTL_4_directio.sys_sec 41.65 ± 2% +22.1% 50.86 ± 4% +6.7% 44.43 ± 7% fxmark.ssd_ext4_no_jnl_DWTL_4_directio.sys_util 1163070 -22.9% 896752 ± 5% -4.4% 1111656 fxmark.ssd_ext4_no_jnl_DWTL_4_directio.works/sec 1.32 ± 18% -16.6% 1.10 ± 22% -22.0% 1.03 ± 3% fxmark.ssd_ext4_no_jnl_DWTL_54_directio.irq_util 0.00 ± 2% +25.1% 0.01 ± 8% -1.2% 0.00 ± 4% fxmark.ssd_ext4_no_jnl_DWTL_54_directio.secs 0.24 ± 3% +37.5% 0.33 ± 4% +16.7% 0.28 ± 2% fxmark.ssd_ext4_no_jnl_DWTL_54_directio.sys_sec 11.85 ± 4% +30.8% 15.50 ± 4% +21.7% 14.41 ± 6% fxmark.ssd_ext4_no_jnl_DWTL_54_directio.sys_util 5031984 ± 2% -19.5% 4049295 ± 8% +1.3% 5099506 ± 4% fxmark.ssd_ext4_no_jnl_DWTL_54_directio.works/sec 0.00 ± 3% +37.5% 0.01 ± 11% +13.8% 0.00 ± 9% fxmark.ssd_ext4_no_jnl_DWTL_72_directio.secs 0.33 ± 9% +29.0% 0.43 ± 8% +12.0% 0.37 ± 8% fxmark.ssd_ext4_no_jnl_DWTL_72_directio.sys_sec 12.11 ± 10% +21.9% 14.76 ± 6% +12.5% 13.63 ± 7% fxmark.ssd_ext4_no_jnl_DWTL_72_directio.sys_util 5533529 ± 4% -26.3% 4078851 ± 12% -11.5% 4896500 ± 8% fxmark.ssd_ext4_no_jnl_DWTL_72_directio.works/sec The test result looks good (only test with 72 process doesn't restore to same level as original result). You may notice the test with 36 process are not showed here. It is because of the lkp script problem and we are working on it. Checked the test result of 36 process manually: e394ff83bbca1c72: avg = 4267358.820936666 -> the parent of bad commit d4252071b97d2027: avg = 3821149.8479718883 -> the bad commit 6812880b7af46dc2: avg = 4724775.219265333 -> the fixing patch commit It looks good also. > > HOWEVER. > > I'd love to hear if you have a clear profile change, and to see > exactly which set_buffer_uptodate() is *so* important. Honestly, I > didn't expect the buffer head functions to even really matter much any > more, with pretty much all IO being about the page cache.. Unfortunately, the valid profile data was not captured yet. We will keep working on it and share out once we get the valid profile data. Thanks. Regards Yin, Fengwei
next prev parent reply other threads:[~2022-09-08 8:56 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-08-31 7:21 d4252071b9: fxmark.ssd_ext4_no_jnl_DWTL_54_directio.works/sec -26.5% regression kernel test robot 2022-08-31 7:21 ` kernel test robot 2022-08-31 16:46 ` Linus Torvalds 2022-08-31 16:46 ` Linus Torvalds 2022-09-08 8:55 ` Yin, Fengwei [this message] 2022-09-08 8:55 ` Yin, Fengwei 2022-09-08 12:14 ` [LKP] " Linus Torvalds 2022-09-08 12:14 ` Linus Torvalds 2022-09-08 13:25 ` [LKP] " Yin, Fengwei 2022-09-08 13:25 ` Yin, Fengwei 2022-09-08 16:50 ` d4252071b9: fxmark.ssd_ext4_no_jnl_DWTL_54_directio.works/sec -26.5% regression #forregzbot Thorsten Leemhuis
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=0214b84c-71fe-2133-b69d-1e39a19cc468@intel.com \ --to=fengwei.yin@intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=lkp@intel.com \ --cc=lkp@lists.01.org \ --cc=mpatocka@redhat.com \ --cc=oliver.sang@intel.com \ --cc=regressions@lists.linux.dev \ --cc=torvalds@linux-foundation.org \ --cc=willy@infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.