Re: [PATCH v2] mm: memcg: release the css reference when a stock slot empties

On Tue 18-08-26 21:01:35, Song Hu wrote:
> consume_stock() can drive a stock slot's nr_pages to zero while its
> cached[] pointer stays set, so the slot keeps pinning the css
> reference that refill_stock() took.  The offlining drain only
> flushes slots with cached pages, so the reference is never released
> unless the slot happens to be displaced by an unrelated charge or
> by CPU hotplug, and the memcg lingers in the dying state - up to
> NR_MEMCG_STOCK (7) of them per CPU under container churn.
> 
> Keeping the slot populated past the last page only saves a
> css_get()/css_put() pair on the next charge of the same memcg, and
> costs more than that: the offlining drain has to know about empty
> slots, and refill_stock() cannot reuse them either, so a charge
> under a different memcg evicts a live batch through the drain_idx
> rotation instead.
> 
> Drop the reference in consume_stock() when the slot empties.
> Empty slots stop existing, so is_memcg_drain_needed() and the drain
> path stay as they are, and refill_stock() reuses emptied slots
> directly.  The cost is one refcount pair per emptied slot, at most
> once per MEMCG_CHARGE_BATCH pages.
> 
> Fixes: d1a05b6973c7 ("memcg: do not try to drain per-cpu caches without pages")
> Signed-off-by: Song Hu <[email protected]>

Acked-by: Michal Hocko <[email protected]>
Thanks!

> ---
> 
> Changes since v1: drop the reference at consume time instead of flushing
> empty slots from the offlining drain, as discussed with Michal Hocko and
> Joshua Hahn (full-stock drain cost, refill reuse).
> 
> v1: https://lore.kernel.org/all/[email protected]/
> 
>  mm/memcontrol.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 17da1f43b7d3..1271d390b617 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2140,7 +2140,12 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
>  
>  		stock_pages = READ_ONCE(stock->nr_pages[i]);
>  		if (stock_pages >= nr_pages) {
> -			WRITE_ONCE(stock->nr_pages[i], stock_pages - nr_pages);
> +			stock_pages -= nr_pages;
> +			WRITE_ONCE(stock->nr_pages[i], stock_pages);
> +			if (!stock_pages) {
> +				css_put(&memcg->css);
> +				WRITE_ONCE(stock->cached[i], NULL);
> +			}
>  			ret = true;
>  		}
>  		break;
> -- 
> 2.43.0

-- 
Michal Hocko
SUSE Labs