Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
From: Michal Hocko <mhocko@suse.com>
To: David Hildenbrand <david@redhat.com>
Cc: Pavel Tatashin <pasha.tatashin@soleen.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	LKML <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-mm <linux-mm@kvack.org>
Subject: Re: [PATCH] mm/memory_hotplug: drain per-cpu pages again during memory offline
Date: Fri, 4 Sep 2020 09:02:35 +0200
Message-ID: <20200904070235.GA15277@dhcp22.suse.cz> (raw)
In-Reply-To: <d89510b1-a6a2-a874-7ffc-ba7a37d4212d@redhat.com>

On Thu 03-09-20 20:31:04, David Hildenbrand wrote:
> On 03.09.20 20:23, Pavel Tatashin wrote:
> > On Thu, Sep 3, 2020 at 2:20 PM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 03.09.20 08:38, Michal Hocko wrote:
[...]
> >>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> >>> index 242c03121d73..56d4892bceb8 100644
> >>> --- a/mm/page_isolation.c
> >>> +++ b/mm/page_isolation.c
> >>> @@ -170,6 +170,14 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
> >>>   * pageblocks we may have modified and return -EBUSY to caller. This
> >>>   * prevents two threads from simultaneously working on overlapping ranges.
> >>>   *
> >>> + * Please note that there is no strong synchronization with the page allocator
> >>> + * either. Pages might be freed while their page blocks are marked ISOLATED.
> >>> + * In some cases pages might still end up on pcp lists and that would allow
> >>> + * for their allocation even when they are in fact isolated already. Depending on
> >>> + * how strong of a guarantee the caller needs drain_all_pages might be needed
> >>> + * (e.g. __offline_pages will need to call it after check for isolated range for
> >>> + * a next retry).
> >>> + *
> >>
> >> As expressed in reply to v2, I dislike this hack. There is strong
> >> synchronization, just PCP is special. Allocating from MIGRATE_ISOLATE is
> >> just plain ugly.

Completely agreed! I am not happy about that either. But I believe this
hack is the easiest way forward for stable trees and as an immediate
fix. We can build on top of that of course.

> >> Can't we temporarily disable PCP (while some pageblock in the zone is
> >> isolated, which we know e.g., due to the counter), so no new pages get
> >> put into PCP lists after draining, and re-enable after no pageblocks are
> >> isolated again? We keep draining the PCP, so it doesn't seem to be of a
> >> lot of use during that period, no? It's a performance hit already.

This is a good point.

> >> Then, we would only need exactly one drain. And we would only have to
> >> check on the free path whether PCP is temporarily disabled.
> > 
> > Hm, we could use a static branches to disable it, that would keep
> > release code just as fast, but I am worried it will make code even
> > uglier. Let's see what others in this thread think about this idea.

I know that static branches are a very effective way to enable/disable
features but I have no experience in how they perform for a very
shortlived use. Maybe that is just fine for a single place which needs
to be patched. This would be especially a problem if the static branch
is to be enabled from start_isolate_page_range because that includes all
cma allocator users.

Another alternative would be to enable/disable static branch only from
users who really care but this is quite tricky because how do you tell
you need or not? It seems that alloc_contig_range would be just fine
with a weaker semantic because it would "only" to a spurious failure.
Memory hotplug on the other hand really needs to have a point where
nobody interferes with the offlined memory so it could ask for a
stronger semantic.

Yet another option would be to make draining stronger and actually
guarantee there are no in-flight pages to be freed to the pcp list.
One way would be to tweak pcp->high and implement a strong barrier
(IPI?) to sync with all CPUs. Quite expensive, especially when there are
many draining requests (read cma users because hotplug doesn't really
matter much as it happens seldom).

So no nice&cheap solution I can think of...
-- 
Michal Hocko
SUSE Labs


  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-01 12:46 Pavel Tatashin
2020-09-01 18:37 ` David Rientjes
2020-09-02 14:01 ` Michal Hocko
2020-09-02 14:10   ` Michal Hocko
2020-09-02 14:31     ` Pavel Tatashin
2020-09-02 14:49       ` Vlastimil Babka
2020-09-02 14:08 ` Michal Hocko
2020-09-02 14:26   ` Pavel Tatashin
2020-09-02 14:55     ` Vlastimil Babka
2020-09-02 15:13       ` Michal Hocko
2020-09-02 15:40         ` Pavel Tatashin
2020-09-02 17:51         ` Vlastimil Babka
2020-09-03  6:38           ` Michal Hocko
2020-09-03 18:20             ` David Hildenbrand
2020-09-03 18:23               ` Pavel Tatashin
2020-09-03 18:31                 ` David Hildenbrand
2020-09-04  7:02                   ` Michal Hocko [this message]
2020-09-04 14:25                     ` Pavel Tatashin
2020-09-07  7:26                       ` Michal Hocko
2020-09-04  6:32                 ` Vlastimil Babka
2020-09-03  7:07 ` Michal Hocko
2020-09-03 13:43   ` Pavel Tatashin
2020-09-03 13:50 ` Vlastimil Babka

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=20200904070235.GA15277@dhcp22.suse.cz \
    --to=mhocko@suse.com \
    --cc=akpm@linux-foundation.org \
    --cc=david@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=pasha.tatashin@soleen.com \
    --cc=vbabka@suse.cz \
    /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

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git