Re: [PATCH 0/8 RFC] mm/memcontrol, page_counter: move stock from mem_cgroup to page_counter

[Michal Hocko <mhocko@suse.com>]

On Fri 10-04-26 14:06:54, Joshua Hahn wrote:
> Memcg currently keeps a "stock" of 64 pages per-cpu to cache pre-charged
> allocations, allowing small allocations and frees to avoid walking the
> expensive mem_cgroup hierarchy traversal on each charge. This design
> introduces a fastpath to charge/uncharge, but has several limitations:
> 
> 1. Each CPU can track up to 7 (NR_MEMCG_STOCK) mem_cgroups. When more
>    than 7 mem_cgroups are actively charging on a single CPU, a random
>    victim is evicted, and its associated stock is drained, which
>    triggers unnecessary hierarchy walks.
> 
>    Note that previously there used to be a 1-1 mapping between CPU and
>    memcg stock; it was bumped up to 7 in f735eebe55f8f ("multi-memcg
>    percpu charge cache") because it was observed that stock would
>    frequently get flushed and refilled.

All true but it is quite important to note that this all is bounded to
nr_online_cpus*NR_MEMCG_STOCK*MEMCG_CHARGE_BATCH. You are proposing to
increase this to s@NR_MEMCG_STOCK@nr_leaf_cgroups@. In invornments with
many cpus and and directly charged cgroups this can be considerable
hidden overcharge. Have you considered that and evaluated potential
impact?

> 2. Stock management is tightly coupled to struct mem_cgroup, which
>    makes it difficult to add a new page_counter to struct mem_cgroup
>    and do its own stock management, since each operation has to be
>    duplicated.

Could you expand why this is a problem we need to address?

> 3. Each stock slot requires a css reference, as well as a traversal
>    overhead on every stock operation to check which cpu-memcg we are
>    trying to consume stock for.

Why is this a problem?
 
Please also be more explicit what kind of workloads are going to benefit
from this change. The existing caching scheme is simple and ineffective
but is it worth improving (likely your points 2 and 3 could clarify that)?

All that being said, I like the resulting code which is much easier to
follow. The caching is nicely transparent in the charging path which is
a plus. My main worry is that caching has caused some confusion in the
past and this change will amplify that by the scaling the amount of
cached charge. This needs to be really carefully evaluated.
-- 
Michal Hocko
SUSE Labs