linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michal Hocko <mhocko@suse.com>
To: Shakeel Butt <shakeelb@google.com>
Cc: Vasily Averin <vvs@virtuozzo.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Vladimir Davydov <vdavydov.dev@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Mel Gorman <mgorman@techsingularity.net>,
	Roman Gushchin <guro@fb.com>, Uladzislau Rezki <urezki@gmail.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	Cgroups <cgroups@vger.kernel.org>, Linux MM <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>,
	kernel@openvz.org
Subject: Re: [PATCH mm v3] memcg: enable memory accounting in __alloc_pages_bulk
Date: Wed, 13 Oct 2021 19:16:29 +0200	[thread overview]
Message-ID: <YWcUbXfBsbNzYIad@dhcp22.suse.cz> (raw)
In-Reply-To: <CALvZod6K6UXxDrkHp=mVDV7O-fAtmRkgMDngPazBhcyDUNxy_Q@mail.gmail.com>

On Wed 13-10-21 09:41:15, Shakeel Butt wrote:
> On Tue, Oct 12, 2021 at 11:24 AM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Tue 12-10-21 09:08:38, Shakeel Butt wrote:
> > > On Tue, Oct 12, 2021 at 8:36 AM Michal Hocko <mhocko@suse.com> 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 */
> > > > +       }
> > >
> > > It is not that simple because __alloc_pages_bulk only allocate pages
> > > for empty slots in the page_array provided by the caller.
> > >
> > > The failure handling for post charging would be more complicated.
> >
> > If this is really that complicated (I haven't tried) then it would be
> > much more simple to completely skip the bulk allocator for __GFP_ACCOUNT
> > rather than add a tricky code. The bulk allocator is meant to be used
> > for ultra hot paths and memcg charging along with the reclaim doesn't
> > really fit into that model anyway. Or are there any actual users who
> > really need bulk allocator optimization and also need memcg accounting?
> 
> Bulk allocator is being used for vmalloc and we have several
> kvmalloc() with __GFP_ACCOUNT allocations.

Do we really need to use bulk allocator for these allocations?
Bulk allocator is an bypass of the page allocator for performance reason
and I can see why that can be useful but considering that the charging
path can imply some heavy lifting is all the code churn to make bulk
allocator memcg aware really worth it? Why cannot we simply skip over
bulk allocator for __GFP_ACCOUNT. That would be a trivial fix.
-- 
Michal Hocko
SUSE Labs

  reply	other threads:[~2021-10-13 17:16 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 [this message]
2021-10-13 17:30                               ` Shakeel Butt
2021-10-12 18:45                       ` Vasily Averin
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=YWcUbXfBsbNzYIad@dhcp22.suse.cz \
    --to=mhocko@suse.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=shakeelb@google.com \
    --cc=urezki@gmail.com \
    --cc=vbabka@suse.cz \
    --cc=vdavydov.dev@gmail.com \
    --cc=vvs@virtuozzo.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).