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

[Waiman Long]

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.

Thanks,
Longman