[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240405065133.GA5329@KORCO045595.samsungds.net>
Date: Fri, 5 Apr 2024 15:51:33 +0900
From: Bongkyu Kim <bongkyu7.kim@...sung.com>
To: Waiman Long <longman@...hat.com>
Cc: John Stultz <jstultz@...gle.com>, peterz@...radead.org,
mingo@...hat.com, will@...nel.org, boqun.feng@...il.com,
linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
bongkyu7.kim@...sung.com
Subject: Re: [PATCH v2 0/2] Make reader optimistic spinning optional
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@...sung.com> 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@...sung.com> wrote:
> > > > > > > This is rework of the following discussed patch.
> > > > > > > https://lore.kernel.org/all/20230613043308.GA1027@KORCO045595.samsungds.net/
> > > > > > >
> > > > > > >
> > > > > > > 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
Powered by blists - more mailing lists