From: David Hildenbrand <david@redhat.com>
To: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Nitesh Narayan Lal <nitesh@redhat.com>,
kvm list <kvm@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
linux-mm <linux-mm@kvack.org>,
virtio-dev@lists.oasis-open.org,
Paolo Bonzini <pbonzini@redhat.com>,
lcapitulino@redhat.com, Pankaj Gupta <pagupta@redhat.com>,
"Wang, Wei W" <wei.w.wang@intel.com>,
Yang Zhang <yang.zhang.wz@gmail.com>,
Rik van Riel <riel@surriel.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
dodgen@google.com, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
dhildenb@redhat.com, Andrea Arcangeli <aarcange@redhat.com>,
john.starks@microsoft.com, Dave Hansen <dave.hansen@intel.com>,
Michal Hocko <mhocko@suse.com>,
cohuck@redhat.com
Subject: Re: [RFC][Patch v12 1/2] mm: page_reporting: core infrastructure
Date: Wed, 14 Aug 2019 09:07:23 +0200 [thread overview]
Message-ID: <3409b5cc-59e0-28d2-de5f-aa7cadaf434c@redhat.com> (raw)
In-Reply-To: <CAKgT0UfaaHrEaS2wsbdTuzCdCtSrM4Tx79w=dP8HPEnq+T7rtQ@mail.gmail.com>
On 14.08.19 01:14, Alexander Duyck wrote:
> On Tue, Aug 13, 2019 at 3:34 AM David Hildenbrand <david@redhat.com> wrote:
>>
>>>>>> +static int process_free_page(struct page *page,
>>>>>> + struct page_reporting_config *phconf, int count)
>>>>>> +{
>>>>>> + int mt, order, ret = 0;
>>>>>> +
>>>>>> + mt = get_pageblock_migratetype(page);
>>>>>> + order = page_private(page);
>>>>>> + ret = __isolate_free_page(page, order);
>>>>>> +
>>>> I just started looking into the wonderful world of
>>>> isolation/compaction/migration.
>>>>
>>>> I don't think saving/restoring the migratetype is correct here. AFAIK,
>>>> MOVABLE/UNMOVABLE/RECLAIMABLE is just a hint, doesn't mean that e.g.,
>>>> movable pages and up in UNMOVABLE or ordinary kernel allocations on
>>>> MOVABLE. So that shouldn't be an issue - I guess.
>>>>
>>>> 1. You should never allocate something that is no
>>>> MOVABLE/UNMOVABLE/RECLAIMABLE. Especially not, if you have ISOLATE or
>>>> CMA here. There should at least be a !is_migrate_isolate_page() check
>>>> somewhere
>>>>
>>>> 2. set_migratetype_isolate() takes the zone lock, so to avoid racing
>>>> with isolation code, you have to hold the zone lock. Your code seems to
>>>> do that, so at least you cannot race against isolation.
>>>>
>>>> 3. You could end up temporarily allocating something in the
>>>> ZONE_MOVABLE. The pages you allocate are, however, not movable. There
>>>> would have to be a way to make alloc_contig_range()/offlining code
>>>> properly wait until the pages have been processed. Not sure about the
>>>> real implications, though - too many details in the code (I wonder if
>>>> Alex' series has a way of dealing with that)
>>>>
>>>> When you restore the migratetype, you could suddenly overwrite e.g.,
>>>> ISOLATE, which feels wrong.
>>>
>>>
>>> I was triggering an occasional CPU stall bug earlier, with saving and restoring
>>> the migratetype I was able to fix it.
>>> But I will further look into this to figure out if it is really required.
>>>
>>
>> You should especially look into handling isolated/cma pages. Maybe that
>> was the original issue. Alex seems to have added that in his latest
>> series (skipping isolated/cma pageblocks completely) as well.
>
> So as far as skipping isolated pageblocks, I get the reason for
> skipping isolated, but why would we need to skip CMA? I had made the
> change I did based on comments you had made earlier. But while working
> on some of the changes to address isolation better and looking over
> several spots in the code it seems like CMA is already being used as
> an allocation fallback for MIGRATE_MOVABLE. If that is the case
> wouldn't it make sense to allow pulling pages and reporting them while
> they are in the free_list?
I was assuming that CMA is also to be skipped because "static int
fallbacks[MIGRATE_TYPES][4]" in mm/page_alloc.c doesn't handle CMA at
all, meaning we should never fallback to CMA or from CMA to another type
- at least when stealing pages from another migratetype. So it smells
like MIGRATE_CMA is static -> the area is marked once and will never be
converted to something else (except MIGRATE_ISOLATE temporarily).
I assume you are talking about gfp_to_alloc_flags()/prepare_alloc_pages():
#ifdef CONFIG_CMA
if (gfpflags_to_migratetype(gfp_mask) == MIGRATE_MOVABLE)
alloc_flags |= ALLOC_CMA;
#endif
Yeah, this looks like MOVABLE allocations can fallback to CMA
pageblocks. And from what I read, "CMA may use its own migratetype
(MIGRATE_CMA) which behaves similarly to ZONE_MOVABLE but can be put in
arbitrary places."
So I think you are right, it could be that it is safe to temporarily
pull out CMA pages (in contrast to isolated pages) - assuming it is fine
to have temporary unmovable allocations on them (different discussion).
(I am learning about the details as we discuss :) )
The important part would then be to never allocate from the isolated
pageblocks and to never overwrite MIGRATE_ISOLATE.
--
Thanks,
David / dhildenb
next prev parent reply other threads:[~2019-08-14 7:07 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-12 13:12 [RFC][PATCH v12 0/2] mm: Support for page reporting Nitesh Narayan Lal
2019-08-12 13:12 ` [RFC][Patch v12 1/2] mm: page_reporting: core infrastructure Nitesh Narayan Lal
2019-08-12 18:47 ` Alexander Duyck
2019-08-12 20:04 ` Nitesh Narayan Lal
2019-08-20 14:11 ` Nitesh Narayan Lal
2019-08-12 20:05 ` David Hildenbrand
2019-08-13 10:30 ` Nitesh Narayan Lal
2019-08-13 10:34 ` David Hildenbrand
2019-08-13 10:42 ` Nitesh Narayan Lal
2019-08-13 10:44 ` David Hildenbrand
2019-08-13 23:14 ` Alexander Duyck
2019-08-14 7:07 ` David Hildenbrand [this message]
2019-08-14 12:49 ` [virtio-dev] " Nitesh Narayan Lal
2019-08-14 15:49 ` Nitesh Narayan Lal
2019-08-14 16:11 ` Alexander Duyck
2019-08-15 13:15 ` Nitesh Narayan Lal
2019-08-15 19:22 ` Nitesh Narayan Lal
2019-08-15 23:00 ` Alexander Duyck
2019-08-16 18:35 ` Nitesh Narayan Lal
2019-08-30 15:15 ` Nitesh Narayan Lal
2019-08-30 15:31 ` Alexander Duyck
2019-08-30 16:05 ` Nitesh Narayan Lal
2019-09-04 8:40 ` [virtio-dev] " David Hildenbrand
2019-10-10 20:36 ` Alexander Duyck
2019-10-11 11:02 ` Nitesh Narayan Lal
2019-08-12 13:12 ` [RFC][Patch v12 2/2] virtio-balloon: interface to support free page reporting Nitesh Narayan Lal
2019-08-14 10:29 ` Cornelia Huck
2019-08-14 11:47 ` Nitesh Narayan Lal
2019-08-14 13:42 ` Cornelia Huck
2019-08-14 14:01 ` Nitesh Narayan Lal
2019-08-12 13:13 ` [QEMU Patch 1/2] virtio-balloon: adding bit for page reporting support Nitesh Narayan Lal
2019-08-12 13:13 ` [QEMU Patch 2/2] virtio-balloon: support for handling page reporting Nitesh Narayan Lal
2019-08-12 15:18 ` Alexander Duyck
2019-08-12 15:26 ` Nitesh Narayan Lal
2019-09-11 12:30 ` [RFC][PATCH v12 0/2] mm: Support for " David Hildenbrand
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=3409b5cc-59e0-28d2-de5f-aa7cadaf434c@redhat.com \
--to=david@redhat.com \
--cc=aarcange@redhat.com \
--cc=alexander.duyck@gmail.com \
--cc=cohuck@redhat.com \
--cc=dave.hansen@intel.com \
--cc=dhildenb@redhat.com \
--cc=dodgen@google.com \
--cc=john.starks@microsoft.com \
--cc=konrad.wilk@oracle.com \
--cc=kvm@vger.kernel.org \
--cc=lcapitulino@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@suse.com \
--cc=mst@redhat.com \
--cc=nitesh@redhat.com \
--cc=pagupta@redhat.com \
--cc=pbonzini@redhat.com \
--cc=riel@surriel.com \
--cc=virtio-dev@lists.oasis-open.org \
--cc=wei.w.wang@intel.com \
--cc=yang.zhang.wz@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).