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

[Waiman Long]

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