linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michal Hocko <mhocko@suse.com>
To: Minchan Kim <minchan@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	David Hildenbrand <david@redhat.com>,
	linux-mm <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Suren Baghdasaryan <surenb@google.com>,
	John Dias <joaodias@google.com>
Subject: Re: [RESEND][PATCH v2] mm: don't call lru draining in the nested lru_cache_disable
Date: Wed, 19 Jan 2022 10:20:22 +0100	[thread overview]
Message-ID: <YefX1t4owjlx/m5I@dhcp22.suse.cz> (raw)
In-Reply-To: <YedXhpwURNTkW1Z3@google.com>

On Tue 18-01-22 16:12:54, Minchan Kim wrote:
> On Mon, Jan 17, 2022 at 02:47:06PM +0100, Michal Hocko wrote:
> > On Thu 30-12-21 11:36:27, Minchan Kim wrote:
> > > lru_cache_disable involves IPIs to drain pagevec of each core,
> > > which sometimes takes quite long time to complete depending
> > > on cpu's business, which makes allocation too slow up to
> > > sveral hundredth milliseconds. Furthermore, the repeated draining
> > > in the alloc_contig_range makes thing worse considering caller
> > > of alloc_contig_range usually tries multiple times in the loop.
> > >
> > > This patch makes the lru_cache_disable aware of the fact the
> > > pagevec was already disabled. With that, user of alloc_contig_range
> > > can disable the lru cache in advance in their context during the
> > > repeated trial so they can avoid the multiple costly draining
> > > in cma allocation.
> > 
> > Do you have any numbers on any improvements?
> 
> The LRU draining consumed above 50% overhead for the 20M CMA alloc.

This doesn't say much about the improvement itself.
 
> > Now to the change. I do not like this much to be honest. LRU cache
> > disabling is a complex synchronization scheme implemented in
> > __lru_add_drain_all now you are stacking another level on top of that.
> > 
> > More fundamentally though. I am not sure I understand the problem TBH.
> 
> The problem is that kinds of IPI using normal prority workqueue to drain
> takes much time depending on the system CPU business.

How does this patch address that problem? The IPI has to happen at some
point as we need to sync up with pcp caches.

> > What prevents you from calling lru_cache_disable at the cma level in the
> > first place?
> 
> You meant moving the call from alloc_contig_range to caller layer?

Yes.

> So, virtio_mem_fake_online, too? It could and make sense from
> performance perspective since upper layer usually calls the
> alloc_contig_range multiple times on retrial loop.
> 
> Havid said, semantically, not good in that why upper layer should
> know how alloc_contig_range works(LRU disable is too low level stuff)
> internally but I chose the performance here.
> 
> There is an example why the stacking is needed.
> cma_alloc also can be called from outside.
> A usecase is try to call
> 
>     lru_cache_disable
>     for (order = 10; order >= 0; order) {
>         page = cma_alloc(1<<order)
>         if (page)
>             break;
>     }
>     lru_cacne_enable
> 
> Here, putting the disable lru outside of cma_alloc is
> much better than inside. That's why I put it outside.

What does prevent you from calling lru_cache_{disable,enable} this way
with the existing implementation? AFAICS calls can be nested just fine.
Or am I missing something?
-- 
Michal Hocko
SUSE Labs

  reply	other threads:[~2022-01-19  9:20 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-30 19:36 [RESEND][PATCH v2] mm: don't call lru draining in the nested lru_cache_disable Minchan Kim
2022-01-06 18:14 ` Minchan Kim
2022-01-17 13:47 ` Michal Hocko
2022-01-19  0:12   ` Minchan Kim
2022-01-19  9:20     ` Michal Hocko [this message]
2022-01-20  4:25       ` Minchan Kim
2022-01-20  8:24         ` Michal Hocko
2022-01-20 21:07           ` Minchan Kim
2022-01-21  9:59             ` Michal Hocko
2022-01-21 21:56               ` Minchan Kim
2022-01-24  9:57                 ` Michal Hocko
2022-01-24 22:22                   ` Minchan Kim
2022-01-25  9:23                     ` Michal Hocko
2022-01-25 21:06                       ` Minchan Kim
2022-01-26 12:09                         ` Michal Hocko
2022-01-20  8:42     ` David Hildenbrand
2022-01-20 21:22       ` Minchan Kim

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=YefX1t4owjlx/m5I@dhcp22.suse.cz \
    --to=mhocko@suse.com \
    --cc=akpm@linux-foundation.org \
    --cc=david@redhat.com \
    --cc=joaodias@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=minchan@kernel.org \
    --cc=surenb@google.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).