Re: [RFC PATCH 1/1] sched/fair: ratelimit update to tg->load_avg

[Vincent Guittot]

On Mon, 21 Aug 2023 at 08:06, Aaron Lu <aaron.lu@xxxxxxxxx> wrote:
>
> On Thu, Aug 17, 2023 at 02:56:00PM +0200, Vincent Guittot wrote:
> > On Wed, 16 Aug 2023 at 04:48, Aaron Lu <aaron.lu@xxxxxxxxx> wrote:
> > >
> > > When using sysbench to benchmark Postgres in a single docker instance
> > > with sysbench's nr_threads set to nr_cpu, it is observed there are times
> > > update_cfs_group() and update_load_avg() shows noticeable overhead on
> > > a 2sockets/112core/224cpu Intel Sapphire Rapids(SPR):
> > >
> > >     13.75%    13.74%  [kernel.vmlinux]           [k] update_cfs_group
> > >     10.63%    10.04%  [kernel.vmlinux]           [k] update_load_avg
> > >
> > > Annotate shows the cycles are mostly spent on accessing tg->load_avg
> > > with update_load_avg() being the write side and update_cfs_group() being
> > > the read side. tg->load_avg is per task group and when different tasks
> > > of the same taskgroup running on different CPUs frequently access
> > > tg->load_avg, it can be heavily contended.
> > >
> > > The frequent access to tg->load_avg is due to task migration on wakeup
> > > path, e.g. when running postgres_sysbench on a 2sockets/112cores/224cpus
> > > Intel Sappire Rapids, during a 5s window, the wakeup number is 14millions
> > > and migration number is 11millions and with each migration, the task's
> > > load will transfer from src cfs_rq to target cfs_rq and each change
> > > involves an update to tg->load_avg. Since the workload can trigger as many
> > > wakeups and migrations, the access(both read and write) to tg->load_avg
> > > can be unbound. As a result, the two mentioned functions showed noticeable
> > > overhead. With netperf/nr_client=nr_cpu/UDP_RR, the problem is worse:
> > > during a 5s window, wakeup number is 21millions and migration number is
> > > 14millions; update_cfs_group() costs ~25% and update_load_avg() costs ~16%.
> > >
> > > Reduce the overhead by limiting updates to tg->load_avg to at most once
> > > per ms. After this change, the cost of accessing tg->load_avg is greatly
> > > reduced and performance improved. Detailed test results below.
> > >
> > > ==============================
> > > postgres_sysbench on SPR:
> > > 25%
> > > base:   42382ą19.8%
> > > patch:  50174ą9.5%  (noise)
> > >
> > > 50%
> > > base:   67626ą1.3%
> > > patch:  67365ą3.1%  (noise)
> > >
> > > 75%
> > > base:   100216ą1.2%
> > > patch:  112470ą0.1% +12.2%
> > >
> > > 100%
> > > base:    93671ą0.4%
> > > patch:  113563ą0.2% +21.2%
> > >
> > > ==============================
> > > hackbench on ICL:
> > > group=1
> > > base:    114912ą5.2%
> > > patch:   117857ą2.5%  (noise)
> > >
> > > group=4
> > > base:    359902ą1.6%
> > > patch:   361685ą2.7%  (noise)
> > >
> > > group=8
> > > base:    461070ą0.8%
> > > patch:   491713ą0.3% +6.6%
> > >
> > > group=16
> > > base:    309032ą5.0%
> > > patch:   378337ą1.3% +22.4%
> > >
> > > =============================
> > > hackbench on SPR:
> > > group=1
> > > base:    100768ą2.9%
> > > patch:   103134ą2.9%  (noise)
> > >
> > > group=4
> > > base:    413830ą12.5%
> > > patch:   378660ą16.6% (noise)
> > >
> > > group=8
> > > base:    436124ą0.6%
> > > patch:   490787ą3.2% +12.5%
> > >
> > > group=16
> > > base:    457730ą3.2%
> > > patch:   680452ą1.3% +48.8%
> > >
> > > ============================
> > > netperf/udp_rr on ICL
> > > 25%
> > > base:    114413ą0.1%
> > > patch:   115111ą0.0% +0.6%
> > >
> > > 50%
> > > base:    86803ą0.5%
> > > patch:   86611ą0.0%  (noise)
> > >
> > > 75%
> > > base:    35959ą5.3%
> > > patch:   49801ą0.6% +38.5%
> > >
> > > 100%
> > > base:    61951ą6.4%
> > > patch:   70224ą0.8% +13.4%
> > >
> > > ===========================
> > > netperf/udp_rr on SPR
> > > 25%
> > > base:   104954ą1.3%
> > > patch:  107312ą2.8%  (noise)
> > >
> > > 50%
> > > base:    55394ą4.6%
> > > patch:   54940ą7.4%  (noise)
> > >
> > > 75%
> > > base:    13779ą3.1%
> > > patch:   36105ą1.1% +162%
> > >
> > > 100%
> > > base:     9703ą3.7%
> > > patch:   28011ą0.2% +189%
> > >
> > > ==============================================
> > > netperf/tcp_stream on ICL (all in noise range)
> > > 25%
> > > base:    43092ą0.1%
> > > patch:   42891ą0.5%
> > >
> > > 50%
> > > base:    19278ą14.9%
> > > patch:   22369ą7.2%
> > >
> > > 75%
> > > base:    16822ą3.0%
> > > patch:   17086ą2.3%
> > >
> > > 100%
> > > base:    18216ą0.6%
> > > patch:   18078ą2.9%
> > >
> > > ===============================================
> > > netperf/tcp_stream on SPR (all in noise range)
> > > 25%
> > > base:    34491ą0.3%
> > > patch:   34886ą0.5%
> > >
> > > 50%
> > > base:    19278ą14.9%
> > > patch:   22369ą7.2%
> > >
> > > 75%
> > > base:    16822ą3.0%
> > > patch:   17086ą2.3%
> > >
> > > 100%
> > > base:    18216ą0.6%
> > > patch:   18078ą2.9%
> > >
> > > Signed-off-by: Aaron Lu <aaron.lu@xxxxxxxxx>
> > > ---
> > >  kernel/sched/fair.c  | 13 ++++++++++++-
> > >  kernel/sched/sched.h |  1 +
> > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 6e79de26a25d..ab055c72cc64 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -3664,7 +3664,8 @@ static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
> > >   */
> > >  static inline void update_tg_load_avg(struct cfs_rq *cfs_rq)
> > >  {
> > > -       long delta = cfs_rq->avg.load_avg - cfs_rq->tg_load_avg_contrib;
> > > +       long delta;
> > > +       u64 now;
> > >
> > >         /*
> > >          * No need to update load_avg for root_task_group as it is not used.
> > > @@ -3672,9 +3673,19 @@ static inline void update_tg_load_avg(struct cfs_rq *cfs_rq)
> > >         if (cfs_rq->tg == &root_task_group)
> > >                 return;
> > >
> > > +       /*
> > > +        * For migration heavy workload, access to tg->load_avg can be
> > > +        * unbound. Limit the update rate to at most once per ms.
> > > +        */
> > > +       now = sched_clock_cpu(cpu_of(rq_of(cfs_rq)));
> > > +       if (now - cfs_rq->last_update_tg_load_avg < NSEC_PER_MSEC)
> > > +               return;
> > > +
> > > +       delta = cfs_rq->avg.load_avg - cfs_rq->tg_load_avg_contrib;
> > >         if (abs(delta) > cfs_rq->tg_load_avg_contrib / 64) {
> > >                 atomic_long_add(delta, &cfs_rq->tg->load_avg);
> > >                 cfs_rq->tg_load_avg_contrib = cfs_rq->avg.load_avg;
> > > +               cfs_rq->last_update_tg_load_avg = now;
> > >         }
> > >  }
> > >
> > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > > index 19af1766df2d..228a298bf3b5 100644
> > > --- a/kernel/sched/sched.h
> > > +++ b/kernel/sched/sched.h
> > > @@ -594,6 +594,7 @@ struct cfs_rq {
> > >
> > >  #ifdef CONFIG_FAIR_GROUP_SCHED
> > >         unsigned long           tg_load_avg_contrib;
> > > +       u64                     last_update_tg_load_avg;
> >
> > Moving last_update_tg_load_avg before tg_load_avg_contrib should
> > minimize the padding on 32bits arch as long is only 4 Bytes
>
> Got it. That would be:
>
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 6a8b7b9ed089..52ee7027def9 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -593,6 +593,7 @@ struct cfs_rq {
>         } removed;
>
>  #ifdef CONFIG_FAIR_GROUP_SCHED
> +       u64                     last_update_tg_load_avg;
>         unsigned long           tg_load_avg_contrib;
>         long                    propagate;
>         long                    prop_runnable_sum;
> --
> 2.41.0
>
> >
> > Apart from this, looks good to me
>
> Thanks a lot for your review!
> Can I add your reviewed-by in my next update with the above change?

Yes with the above
Reviewed-by: Vincent Guittot <vincent.guittot@xxxxxxxxxx>

>
> Regards,
> Aaron
>
> > >         long                    propagate;
> > >         long                    prop_runnable_sum;
> > >
> > > --
> > > 2.41.0
> > >