linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vasily Averin <vvs@virtuozzo.com>
To: Michal Hocko <mhocko@suse.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>,
	Vladimir Davydov <vdavydov.dev@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Shakeel Butt <shakeelb@google.com>,
	Mel Gorman <mgorman@techsingularity.net>,
	Roman Gushchin <guro@fb.com>, Uladzislau Rezki <urezki@gmail.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	cgroups@vger.kernel.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, kernel@openvz.org
Subject: Re: [PATCH mm v3] memcg: enable memory accounting in __alloc_pages_bulk
Date: Tue, 12 Oct 2021 21:45:38 +0300	[thread overview]
Message-ID: <3467508f-0f0f-fc28-c973-ad9f0eeb9818@virtuozzo.com> (raw)
In-Reply-To: <YWWrai/ChIgycgCo@dhcp22.suse.cz>

On 12.10.2021 18:36, Michal Hocko wrote:
> On Tue 12-10-21 17:58:21, Vasily Averin wrote:
>> Enable memory accounting for bulk page allocator.
> 
> ENOCHANGELOG
>  
> And I have to say I am not very happy about the solution. It adds a very
> tricky code where it splits different charging steps apart.
> 
> Would it be just too inefficient to charge page-by-page once all pages
> are already taken away from the pcp lists? This bulk should be small so
> this shouldn't really cause massive problems. I mean something like
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index b37435c274cf..8bcd69195ef5 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5308,6 +5308,10 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
>  
>  	local_unlock_irqrestore(&pagesets.lock, flags);
>  
> +	if (memcg_kmem_enabled() && (gfp & __GFP_ACCOUNT)) {
> +		/* charge pages here */
> +	}
> +
>  	__count_zid_vm_events(PGALLOC, zone_idx(zone), nr_account);
>  	zone_statistics(ac.preferred_zoneref->zone, zone, nr_account);
> 

In general it looks like we can do it.

We can traverse via filled page_array or page_list.
For page_array we need to check is the page already accounted 
(incoming array can contain some pages already, both in the beginning and in middle)
For each taken page we can try to charge it.
If it was charges successfully -- we will process next page in list/array.
When charge fails we need to remove rest of pages from list/array and somehow release them. 
At present I do not understand how to do it correctly -- perhaps just call free_page() ?
Finally, we'll need to adjust nr_account and nr_populated properly.

I'll try to implement this tomorrow.

Thank you,
	Vasily Averin

  parent reply	other threads:[~2021-10-12 18:47 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-07  8:04 memcg memory accounting in vmalloc is broken Vasily Averin
2021-10-07  8:13 ` Michal Hocko
2021-10-07  8:16   ` Michal Hocko
2021-10-07  8:50     ` Vasily Averin
2021-10-07 10:08       ` Michal Hocko
2021-10-07 10:20       ` Mel Gorman
2021-10-07 14:02         ` Vlastimil Babka
2021-10-07 14:00       ` Vlastimil Babka
2021-10-07 14:09         ` Michal Hocko
2021-10-07 19:33       ` Vasily Averin
2021-10-08  9:23         ` [PATCH memcg] memcg: enable memory accounting in __alloc_pages_bulk Vasily Averin
2021-10-08 17:35           ` Shakeel Butt
2021-10-12 10:18             ` [PATCH mm v2] " Vasily Averin
2021-10-12 13:10               ` Mel Gorman
2021-10-12 13:40                 ` Vasily Averin
2021-10-12 14:58                   ` [PATCH mm v3] " Vasily Averin
2021-10-12 15:19                     ` Shakeel Butt
2021-10-12 15:20                     ` Mel Gorman
2021-10-12 15:36                     ` Michal Hocko
2021-10-12 16:08                       ` Shakeel Butt
2021-10-12 18:24                         ` Michal Hocko
2021-10-13 16:41                           ` Shakeel Butt
2021-10-13 17:16                             ` Michal Hocko
2021-10-13 17:30                               ` Shakeel Butt
2021-10-12 18:45                       ` Vasily Averin [this message]
2021-10-14  8:02                   ` [PATCH mm v5] " Vasily Averin
2021-10-15 21:34                     ` Andrew Morton
2021-10-16  6:04                       ` Vasily Averin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3467508f-0f0f-fc28-c973-ad9f0eeb9818@virtuozzo.com \
    --to=vvs@virtuozzo.com \
    --cc=akpm@linux-foundation.org \
    --cc=cgroups@vger.kernel.org \
    --cc=guro@fb.com \
    --cc=hannes@cmpxchg.org \
    --cc=kernel@openvz.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@techsingularity.net \
    --cc=mhocko@suse.com \
    --cc=shakeelb@google.com \
    --cc=urezki@gmail.com \
    --cc=vbabka@suse.cz \
    --cc=vdavydov.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).