Re: memcg memory accounting in vmalloc is broken

On 10/7/21 12:20, Mel Gorman wrote:

On Thu, Oct 07, 2021 at 11:50:44AM +0300, Vasily Averin wrote:

quoted

On 10/7/21 11:16 AM, Michal Hocko wrote:

quoted

Cc Mel and Uladzislau

On Thu 07-10-21 10:13:23, Michal Hocko wrote:

quoted

On Thu 07-10-21 11:04:40, Vasily Averin wrote:

quoted

vmalloc was switched to __alloc_pages_bulk but it does not account the memory to memcg.

Is it known issue perhaps?

No, I think this was just overlooked. Definitely doesn't look
intentional to me.

I use following patch as a quick fix,
it helps though it is far from ideal and can be optimized.

Thanks Vasily.

This papers over the problem but it could certainly be optimized. At
minimum;

1. Test (memcg_kmem_enabled() && (gfp & __GFP_ACCOUNT)) in the
   function preamble and store the result in a bool

memcg_kmem_enabled() is a static key based check so caching that defeats its
purpose and changes it to a normal branch. That leaves gfp & __GFP_ACCOUNT,
which may perhaps still benefit from such caching.

2. Avoid the temptation to batch the accounting because if the
   accounting fails, there is no information on how many pages could be
   allocated before the limits were hit. I guess you could pre-charge the
   pages and uncharging the number of pages that failed to be allocated
   but it should be a separate patch.
3. If an allocation fails due to memcg accounting, break
   out of the loop because all remaining bulk allocations are
   also likely to fail.

As it's not vmalloc's fault, I would suggest the patch
have
Fixes: 387ba26fb1cb ("mm/page_alloc: add a bulk page allocator")
and
Cc: <redacted>

Note the Cc should just be in the patch and not mailed directly to
stable@ as it'll simply trigger a form letter about the patch having to
be merged to mainline first.