Re: [PATCH v2] memcg: trim the per-cpu charge stock instead of draining it

On Wed 19-08-26 18:20:10, Shakeel Butt wrote:
> Joy reported that an application generating a request/response traffic
> pattern spends 44.6% to 57.0% of CPU in the memcg charge/uncharge path
> for a range of message sizes, against 0.27% to 0.71% outside that range.
> Running from the root memcg, where socket memory accounting is skipped,
> recovers the performance.
> 
> Tracing the charge path showed that the application generates a pattern
> where the write syscall charges one page and the read syscall uncharges
> two pages on the same CPU. This hits a corner case in the memcg percpu
> stock code that thrashes the stock continuously.
> 
> In the memcg percpu stock code, MEMCG_CHARGE_BATCH (64) is both the high
> watermark and the emptying target, i.e. on a request to charge one page
> the kernel charges MEMCG_CHARGE_BATCH pages and caches
> (MEMCG_CHARGE_BATCH - 1) of them in the percpu stock. The following
> uncharge of 2 pages takes the cached count to (MEMCG_CHARGE_BATCH + 1),
> and refill_stock() then empties the cache completely. With such a
> pattern the percpu stock becomes completely ineffective.
> 
> Instead of a single boundary point for charges, use the technique the
> page allocator uses for its own percpu caches, which keeps the watermark
> and the emptying target apart: nr_pcp_free() frees between batch and
> high - batch pages, leaving at least pcp->batch on the list. Add a high
> watermark MEMCG_STOCK_HIGH and, once the cached count goes over it,
> return only the pages above MEMCG_STOCK_LOW. The watermarks are
> MEMCG_CHARGE_BATCH apart, so a page_counter update still covers a full
> batch.
> 
> For now, keep MEMCG_STOCK_HIGH same as MEMCG_CHARGE_BATCH and in
> future we will reevaluate if it makes sense to increase it.
> 
> Reported-by: Joy Chaoyue Xiong <[email protected]>
> Signed-off-by: Shakeel Butt <[email protected]>

Acked-by: Michal Hocko <[email protected]>
One minor nit

> ---
> 
> Changes since v1:
> http://lore.kernel.org/[email protected]
> - Kept the upper limit of memcg stock same as before (Michal)
> 
>  mm/memcontrol.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 17da1f43b7d3..58e4d23cf5e0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
[...]
> @@ -2254,9 +2264,12 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
>  			empty_slot = i;
>  		if (memcg == READ_ONCE(stock->cached[i])) {
>  			stock_pages = READ_ONCE(stock->nr_pages[i]) + nr_pages;
> +			if (stock_pages > MEMCG_STOCK_HIGH) {
> +				memcg_uncharge(memcg,
> +					       stock_pages - MEMCG_STOCK_LOW);
> +				stock_pages = MEMCG_STOCK_LOW;

I would find it easier to read to keep the update in sync with
memcg_uncharge, i.e.
				stock_pages = WRITE_ONCE(stock_pages - MEMCG_STOCK_LOW)
> +			}
>  			WRITE_ONCE(stock->nr_pages[i], stock_pages);
> -			if (stock_pages > MEMCG_CHARGE_BATCH)
> -				drain_stock(stock, i);
>  			success = true;
>  			break;
>  		}
> -- 
> 2.53.0-Meta

-- 
Michal Hocko
SUSE Labs