Re: [memcg] 45208c9105: aim7.jobs-per-min -14.0% regression

[Shakeel Butt]

 wi

On Mon, Sep 13, 2021 at 7:13 PM Feng Tang <feng.tang@xxxxxxxxx> wrote:
>
> Hi Shakeel,
>
> On Mon, Sep 13, 2021 at 01:13:57PM -0700, Shakeel Butt wrote:
> > On Mon, Sep 13, 2021 at 1:10 PM Tejun Heo <tj@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Sep 13, 2021 at 01:09:11PM -0700, Shakeel Butt wrote:
> > > > Thanks a lot for the explanation. Are there any concerns to call
> > > > cgroup_rstat_flush_irqsafe(root_mem_cgroup->css.cgroup) in system_wq?
> > > > This will be called every 2 seconds, so, we can assume the updated
> > > > tree would be small most of the time.
> > >
> > > I can't think of a reason why this would be problematic.
> > >
> >
> > Thanks again.
> >
> > Feng, is it possible to re-run these benchmarks with
> > queue_work(system_wq) instead of queue_work(system_unbound_wq)?
>
> I just run the patch twice, and there was no obvious change, the
> hotspot is still the spinlock.
>
> Thanks,
> Feng
>
> $git-diff aa48e47e
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 4d8c9af..fa9cae9 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -683,7 +683,7 @@ void __mod_memcg_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx,
>         /* Update lruvec */
>         __this_cpu_add(pn->lruvec_stats_percpu->state[idx], val);
>         if (!(__this_cpu_inc_return(stats_flush_threshold) % MEMCG_CHARGE_BATCH))
> -               queue_work(system_unbound_wq, &stats_flush_work);
> +               queue_work(system_wq, &stats_flush_work);
>  }
>
>
> 7e1c0d6f58207e7e aa48e47e3906c332eaf1e5d7b58 1638eee6432c1a5175685a7945a
> ---------------- --------------------------- ---------------------------
>              \          |                \          |                \
>     648.62          +243.0%       2224 ±  3%    +246.1%       2244        aim7.cpu
>     588139           -13.4%     509421           -13.5%     508738        aim7.jobs-per-min
>     196.05           -13.4%     169.81           -13.5%     169.58        aim7.jobs-per-min-per-task
>       3.93 ±  3%     +62.8       66.70           +63.4       67.37        pp.child.native_queued_spin_lock_slowpath
>       3.96 ±  4%     +62.8       66.76           +63.5       67.46        pp.child._raw_spin_lock_irqsave
>       3.66 ±  5%     +62.9       66.54           +63.6       67.22        pp.child.lock_page_lruvec_irqsave
>       0.00            +0.1        0.10 ±  5%      +0.1        0.09        pp.self.queue_work_on
>       0.00            +0.2        0.18 ±  5%      +0.2        0.22        pp.self.cgroup_rstat_flush_locked
>       0.00            +0.6        0.60 ±  7%      +0.7        0.72 ±  3%  pp.self.mem_cgroup_css_rstat_flush
>       0.19            +0.7        0.86 ±  5%      +0.7        0.84 ±  4%  pp.self.cgroup_rstat_updated
>       3.92 ±  3%     +62.8       66.70           +63.4       67.37        pp.self.native_queued_spin_lock_slowpath
>
>

Thanks Feng, the spinlock in lock_page_lruvec_irqsave() is unrelated
and not taken in __mod_memcg_lruvec_state().

If removing queue_work() from __mod_memcg_lruvec_state() removes the
regression then I was expecting some improvement by replacing
system_unbound_wq with system_wq. Anyways thanks for your help. I will
run some benchmarks as well (the page fault ones from will-it-scale).