Re: [PATCH v2 0/2] Make reader optimistic spinning optional

[Bongkyu Kim]

On Mon, Apr 08, 2024 at 04:15:04PM +0800, xieliujie wrote:
> 
> On 2024/4/5 14:51, Bongkyu Kim wrote:
> > On Thu, Apr 04, 2024 at 11:06:12PM -0400, Waiman Long wrote:
> > > On 4/4/24 13:44, Waiman Long wrote:
> > > > On 4/2/24 21:42, Bongkyu Kim wrote:
> > > > > On Tue, Apr 02, 2024 at 06:27:40PM -0700, John Stultz wrote:
> > > > > > On Tue, Apr 2, 2024 at 6:21 PM Bongkyu Kim
> > > > > > <bongkyu7.kim@xxxxxxxxxxx> wrote:
> > > > > > > On Tue, Apr 02, 2024 at 04:46:06PM -0700, John Stultz wrote:
> > > > > > > > On Thu, Aug 31, 2023 at 6:07 PM Bongkyu Kim
> > > > > > > > <bongkyu7.kim@xxxxxxxxxxx> wrote:
> > > > > > > > > This is rework of the following discussed patch.
> > > > > > > > > https://lore.kernel.org/all/20230613043308.GA1027@xxxxxxxxxxxxxxxxxxxxxxxxx/
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Changes from the previous patch
> > > > > > > > > - Split to revert and modify patches
> > > > > > > > > - Change according to Waiman Long's review
> > > > > > > > >       More wording to documentation part
> > > > > > > > >       Change module_param to early_param
> > > > > > > > >       Code change by Waiman Long's suggestion
> > > > > > > > > 
> > > > > > > > > In mobile environment, reader optimistic spinning is still useful
> > > > > > > > > because there're not many readers. In my test result
> > > > > > > > > at android device,
> > > > > > > > > it improves application startup time about 3.8%
> > > > > > > > > App startup time is most important factor for
> > > > > > > > > android user expriences.
> > > > > > > > > So, re-enable reader optimistic spinning by this commit. And,
> > > > > > > > > make it optional feature by cmdline.
> > > > > > > > > 
> > > > > > > > > Test result:
> > > > > > > > > This is 15 application startup performance in our exynos soc.
> > > > > > > > > - Cortex A78*2 + Cortex A55*6
> > > > > > > > > - unit: ms (lower is better)
> > > > > > > > > 
> > > > > > > > > Application             base  opt_rspin  Diff  Diff(%)
> > > > > > > > > --------------------  ------  ---------  ----  -------
> > > > > > > > > * Total(geomean)         343        330   -13    +3.8%
> > > > > > > > > --------------------  ------  ---------  ----  -------
> > > > > > > > > helloworld               110        108    -2    +1.8%
> > > > > > > > > Amazon_Seller            397        388    -9    +2.3%
> > > > > > > > > Whatsapp                 311        304    -7    +2.3%
> > > > > > > > > Simple_PDF_Reader        500        463   -37    +7.4%
> > > > > > > > > FaceApp                  330        317   -13    +3.9%
> > > > > > > > > Timestamp_Camera_Free    451        443    -8    +1.8%
> > > > > > > > > Kindle                   629        597   -32    +5.1%
> > > > > > > > > Coinbase                 243        233   -10    +4.1%
> > > > > > > > > Firefox                  425        399   -26    +6.1%
> > > > > > > > > Candy_Crush_Soda         552        538   -14    +2.5%
> > > > > > > > > Hill_Climb_Racing        245        230   -15    +6.1%
> > > > > > > > > Call_Recorder            437        426   -11    +2.5%
> > > > > > > > > Color_Fill_3D            190        180   -10    +5.3%
> > > > > > > > > eToro                    512        505    -7    +1.4%
> > > > > > > > > GroupMe                  281        266   -15    +5.3%
> > > > > > > > > 
> > > > > > > > Hey Bongkyu,
> > > > > > > >     I wanted to reach out to see what the current status of this patch
> > > > > > > > set? I'm seeing other parties trying to work around the loss of the
> > > > > > > > optimistic spinning functionality since commit 617f3ef95177
> > > > > > > > ("locking/rwsem: Remove reader optimistic spinning") as well, with
> > > > > > > > their own custom variants (providing some substantial gains), and
> > > > > > > > would really like to have a common solution.
> > > > > > > > 
> > > > > > > I didn't get an reply, so I've been waiting.
> > > > > > > Could you let me know about their patch?
> > > > > > I don't have insight/access to any other implementations, but I have
> > > > > > nudged folks to test your patch and chime in here.
> > > > > > 
> > > > > > Mostly I just wanted to share that others are also seeing performance
> > > > > > trouble from the loss of optimistic spinning, so it would be good to
> > > > > > get some sort of shared solution upstream.
> > > > > > 
> > > > > > thanks
> > > > > > -john
> > > > > > 
> > > > When this patch series was originally posted last year, we gave some
> > > > comments and suggestion on how to improve it as well as request for more
> > > > information on certain area. We were expecting a v2 with the suggested
> > > > changes, but we never got one and so it just fell off the cliff.
> > > > 
> > > > Please send a v2 with the requested change and we can continue our
> > > > discussion.
> > > The major reason that reader optimistic spinning was taken out is because of
> > > reader fragmentation especially now that we essentially wake up all the
> > > readers all at once when it is reader's turn to take the read lock. I do
> > > admit I am a bit biased toward systems with large number of CPU cores. On
> > > smaller systems with just a few CPU cores, reader optimistic spinning may
> > > help performance. So one idea that I have is that one of the command line
> > > option values is an auto mode (beside on and off) that reader optimistic
> > > spinning is enabled for, say, <= 8 CPUs, but disabled with more CPUs.
> > > 
> > > Anyway, this is just one of my ideas.
> > > 
> > > Cheers,
> > > Longman
> > > 
> > Hi Longman,
> > 
> > Including your idea, I will reconsider and resend patch.
> > 
> > Thanks,
> > Bongkyu
> Hi Bongkyu,
> 
> I have met the same problem as you. After rwsem reader optimistic spin
> disabled, DouYin(or TikTok as you know, is one of the top applicaitons),
> it's uninterruptible sleep (non-io) time increased during cold launch.
> With the patch set you provide, the blocked time can be reduce from
> 122.6ms to 59.3ms(we test it in our mobile device with SM8650 core). Now
> I'm trying to use vendor hook in GKI to restore the reader spin, and
> john said I can share my test result to you here.
> 
> Thanks,
> Liujie

Hi Liujie,

Thanks for your sharing. I'm trying to send v3 patch like below.
https://lore.kernel.org/all/20240406081126.8030-1-bongkyu7.kim@xxxxxxxxxxx/

Bongkyu

> > 
> > 
> ________________________________
> OPPO
> 
> 本电子邮件及其附件含有OPPO公司的保密信息，仅限于邮件指明的收件人（包含个人及群组）使用。禁止任何人在未经授权的情况下以任何形式使用。如果您错收了本邮件，切勿传播、分发、复制、印刷或使用本邮件之任何部分或其所载之任何内容，并请立即以电子邮件通知发件人并删除本邮件及其附件。
> 网络通讯固有缺陷可能导致邮件被截留、修改、丢失、破坏或包含计算机病毒等不安全情况，OPPO对此类错误或遗漏而引致之任何损失概不承担责任并保留与本邮件相关之一切权利。
> 除非明确说明，本邮件及其附件无意作为在任何国家或地区之要约、招揽或承诺，亦无意作为任何交易或合同之正式确认。 发件人、其所属机构或所属机构之关联机构或任何上述机构之股东、董事、高级管理人员、员工或其他任何人（以下称“发件人”或“OPPO”）不因本邮件之误送而放弃其所享之任何权利，亦不对因故意或过失使用该等信息而引发或可能引发的损失承担任何责任。
> 文化差异披露：因全球文化差异影响，单纯以YES\OK或其他简单词汇的回复并不构成发件人对任何交易或合同之正式确认或接受，请与发件人再次确认以获得明确书面意见。发件人不对任何受文化差异影响而导致故意或错误使用该等信息所造成的任何直接或间接损害承担责任。
> This e-mail and its attachments contain confidential information from OPPO, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you are not the intended recipient, please do not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.
> Electronic communications may contain computer viruses or other defects inherently, may not be accurately and/or timely transmitted to other systems, or may be intercepted, modified ,delayed, deleted or interfered. OPPO shall not be liable for any damages that arise or may arise from such matter and reserves all rights in connection with the email.
> Unless expressly stated, this e-mail and its attachments are provided without any warranty, acceptance or promise of any kind in any country or region, nor constitute a formal confirmation or acceptance of any transaction or contract. The sender, together with its affiliates or any shareholder, director, officer, employee or any other person of any such institution (hereinafter referred to as "sender" or "OPPO") does not waive any rights and shall not be liable for any damages that arise or may arise from the intentional or negligent use of such information.
> Cultural Differences Disclosure: Due to global cultural differences, any reply with only YES\OK or other simple words does not constitute any confirmation or acceptance of any transaction or contract, please confirm with the sender again to ensure clear opinion in written form. The sender shall not be responsible for any direct or indirect damages resulting from the intentional or misuse of such information.
>