From: Nitesh Narayan Lal <nitesh@redhat.com> To: David Hildenbrand <david@redhat.com>, Alexander Duyck <alexander.duyck@gmail.com> Cc: 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, pagupta@redhat.com, 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: Tue, 13 Aug 2019 06:30:41 -0400 [thread overview] Message-ID: <d39504c9-93bd-b8f7-e119-84baac5a42d4@redhat.com> (raw) In-Reply-To: <ca362045-9668-18ff-39b0-de91fa72e73c@redhat.com> On 8/12/19 4:05 PM, David Hildenbrand wrote: >>> --- >>> include/linux/mmzone.h | 11 ++ >>> include/linux/page_reporting.h | 63 +++++++ >>> mm/Kconfig | 6 + >>> mm/Makefile | 1 + >>> mm/page_alloc.c | 42 ++++- >>> mm/page_reporting.c | 332 +++++++++++++++++++++++++++++++++ >>> 6 files changed, 448 insertions(+), 7 deletions(-) >>> create mode 100644 include/linux/page_reporting.h >>> create mode 100644 mm/page_reporting.c >>> >>> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h >>> index d77d717c620c..ba5f5b508f25 100644 >>> --- a/include/linux/mmzone.h >>> +++ b/include/linux/mmzone.h >>> @@ -559,6 +559,17 @@ struct zone { >>> /* Zone statistics */ >>> atomic_long_t vm_stat[NR_VM_ZONE_STAT_ITEMS]; >>> atomic_long_t vm_numa_stat[NR_VM_NUMA_STAT_ITEMS]; >>> +#ifdef CONFIG_PAGE_REPORTING >>> + /* Pointer to the bitmap in PAGE_REPORTING_MIN_ORDER granularity */ >>> + unsigned long *bitmap; >>> + /* Preserve start and end PFN in case they change due to hotplug */ >>> + unsigned long base_pfn; >>> + unsigned long end_pfn; >>> + /* Free pages of granularity PAGE_REPORTING_MIN_ORDER */ >>> + atomic_t free_pages; >>> + /* Number of bits required in the bitmap */ >>> + unsigned long nbits; >>> +#endif >>> } ____cacheline_internodealigned_in_smp; >> Okay, so the original thing this patch set had going for it was that >> it was non-invasive. However, now you are adding a bunch of stuff to >> the zone. That kind of loses the non-invasive argument for this patch >> set compared to mine. >> > Adding something to "struct zone" is certainly less invasive than core > buddy modifications, just saying (I agree that this is suboptimal. I > would have guessed that all that's needed is a pointer to some private > structure here). I think having just a pointer to a private structure makes sense here. If I am not wrong then I can probably make an allocation for it for each populated zone at the time I enable page reporting. > However, the migratetype thingy below looks fishy to me. > >> If we are going to continue further with this patch set then it might >> be worth looking into dynamically allocating the space you need for >> this block. At a minimum you could probably look at making the bitmap >> an RCU based setup so you could define the base and end along with the >> bitmap. It would probably help to resolve the hotplug issues you still >> need to address. > Yeah, I guess that makes sense. > > [...] >>> + >>> +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. > [...] >> So as per your comments in the cover page, the two functions above >> should also probably be plugged into the zone resizing logic somewhere >> so if a zone is resized the bitmap is adjusted. >> >>> +/** >>> + * zone_reporting_init - For each zone initializes the page reporting fields >>> + * and allocates the respective bitmap. >>> + * >>> + * This function returns 0 on successful initialization, -ENOMEM otherwise. >>> + */ >>> +static int zone_reporting_init(void) >>> +{ >>> + struct zone *zone; >>> + int ret; >>> + >>> + for_each_populated_zone(zone) { >>> +#ifdef CONFIG_ZONE_DEVICE >>> + /* we can not report pages which are not in the buddy */ >>> + if (zone_idx(zone) == ZONE_DEVICE) >>> + continue; >>> +#endif >> I'm pretty sure this isn't needed since I don't think the ZONE_DEVICE >> zone will be considered "populated". >> > I think you are right (although it's confusing, we will have present > sections part of a zone but the zone has no present_pages - screams like > a re factoring - leftover from ZONE_DEVICE introduction). I think in that case it is safe to have this check here. What do you guys suggest? > -- Thanks Nitesh
WARNING: multiple messages have this Message-ID (diff)
From: Nitesh Narayan Lal <nitesh@redhat.com> To: David Hildenbrand <david@redhat.com>, Alexander Duyck <alexander.duyck@gmail.com> Cc: 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, pagupta@redhat.com, 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: [virtio-dev] Re: [RFC][Patch v12 1/2] mm: page_reporting: core infrastructure Date: Tue, 13 Aug 2019 06:30:41 -0400 [thread overview] Message-ID: <d39504c9-93bd-b8f7-e119-84baac5a42d4@redhat.com> (raw) In-Reply-To: <ca362045-9668-18ff-39b0-de91fa72e73c@redhat.com> On 8/12/19 4:05 PM, David Hildenbrand wrote: >>> --- >>> include/linux/mmzone.h | 11 ++ >>> include/linux/page_reporting.h | 63 +++++++ >>> mm/Kconfig | 6 + >>> mm/Makefile | 1 + >>> mm/page_alloc.c | 42 ++++- >>> mm/page_reporting.c | 332 +++++++++++++++++++++++++++++++++ >>> 6 files changed, 448 insertions(+), 7 deletions(-) >>> create mode 100644 include/linux/page_reporting.h >>> create mode 100644 mm/page_reporting.c >>> >>> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h >>> index d77d717c620c..ba5f5b508f25 100644 >>> --- a/include/linux/mmzone.h >>> +++ b/include/linux/mmzone.h >>> @@ -559,6 +559,17 @@ struct zone { >>> /* Zone statistics */ >>> atomic_long_t vm_stat[NR_VM_ZONE_STAT_ITEMS]; >>> atomic_long_t vm_numa_stat[NR_VM_NUMA_STAT_ITEMS]; >>> +#ifdef CONFIG_PAGE_REPORTING >>> + /* Pointer to the bitmap in PAGE_REPORTING_MIN_ORDER granularity */ >>> + unsigned long *bitmap; >>> + /* Preserve start and end PFN in case they change due to hotplug */ >>> + unsigned long base_pfn; >>> + unsigned long end_pfn; >>> + /* Free pages of granularity PAGE_REPORTING_MIN_ORDER */ >>> + atomic_t free_pages; >>> + /* Number of bits required in the bitmap */ >>> + unsigned long nbits; >>> +#endif >>> } ____cacheline_internodealigned_in_smp; >> Okay, so the original thing this patch set had going for it was that >> it was non-invasive. However, now you are adding a bunch of stuff to >> the zone. That kind of loses the non-invasive argument for this patch >> set compared to mine. >> > Adding something to "struct zone" is certainly less invasive than core > buddy modifications, just saying (I agree that this is suboptimal. I > would have guessed that all that's needed is a pointer to some private > structure here). I think having just a pointer to a private structure makes sense here. If I am not wrong then I can probably make an allocation for it for each populated zone at the time I enable page reporting. > However, the migratetype thingy below looks fishy to me. > >> If we are going to continue further with this patch set then it might >> be worth looking into dynamically allocating the space you need for >> this block. At a minimum you could probably look at making the bitmap >> an RCU based setup so you could define the base and end along with the >> bitmap. It would probably help to resolve the hotplug issues you still >> need to address. > Yeah, I guess that makes sense. > > [...] >>> + >>> +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. > [...] >> So as per your comments in the cover page, the two functions above >> should also probably be plugged into the zone resizing logic somewhere >> so if a zone is resized the bitmap is adjusted. >> >>> +/** >>> + * zone_reporting_init - For each zone initializes the page reporting fields >>> + * and allocates the respective bitmap. >>> + * >>> + * This function returns 0 on successful initialization, -ENOMEM otherwise. >>> + */ >>> +static int zone_reporting_init(void) >>> +{ >>> + struct zone *zone; >>> + int ret; >>> + >>> + for_each_populated_zone(zone) { >>> +#ifdef CONFIG_ZONE_DEVICE >>> + /* we can not report pages which are not in the buddy */ >>> + if (zone_idx(zone) == ZONE_DEVICE) >>> + continue; >>> +#endif >> I'm pretty sure this isn't needed since I don't think the ZONE_DEVICE >> zone will be considered "populated". >> > I think you are right (although it's confusing, we will have present > sections part of a zone but the zone has no present_pages - screams like > a re factoring - leftover from ZONE_DEVICE introduction). I think in that case it is safe to have this check here. What do you guys suggest? > -- Thanks Nitesh --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
next prev parent reply other threads:[~2019-08-13 10:31 UTC|newest] Thread overview: 77+ 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 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-12 13:12 ` [RFC][Patch v12 1/2] mm: page_reporting: core infrastructure Nitesh Narayan Lal 2019-08-12 13:12 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-12 18:47 ` Alexander Duyck 2019-08-12 18:47 ` [virtio-dev] " Alexander Duyck 2019-08-12 18:47 ` Alexander Duyck 2019-08-12 20:04 ` Nitesh Narayan Lal 2019-08-12 20:04 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-20 14:11 ` Nitesh Narayan Lal 2019-08-20 14:11 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-12 20:05 ` David Hildenbrand 2019-08-12 20:05 ` [virtio-dev] " David Hildenbrand 2019-08-13 10:30 ` Nitesh Narayan Lal [this message] 2019-08-13 10:30 ` Nitesh Narayan Lal 2019-08-13 10:34 ` David Hildenbrand 2019-08-13 10:34 ` [virtio-dev] " David Hildenbrand 2019-08-13 10:42 ` Nitesh Narayan Lal 2019-08-13 10:42 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-13 10:44 ` David Hildenbrand 2019-08-13 10:44 ` [virtio-dev] " David Hildenbrand 2019-08-13 23:14 ` Alexander Duyck 2019-08-13 23:14 ` [virtio-dev] " Alexander Duyck 2019-08-13 23:14 ` Alexander Duyck 2019-08-14 7:07 ` David Hildenbrand 2019-08-14 7:07 ` [virtio-dev] " David Hildenbrand 2019-08-14 12:49 ` Nitesh Narayan Lal 2019-08-14 12:49 ` Nitesh Narayan Lal 2019-08-14 15:49 ` Nitesh Narayan Lal 2019-08-14 15:49 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-14 16:11 ` Alexander Duyck 2019-08-14 16:11 ` [virtio-dev] " Alexander Duyck 2019-08-14 16:11 ` Alexander Duyck 2019-08-15 13:15 ` Nitesh Narayan Lal 2019-08-15 13:15 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-15 19:22 ` Nitesh Narayan Lal 2019-08-15 19:22 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-15 23:00 ` Alexander Duyck 2019-08-15 23:00 ` [virtio-dev] " Alexander Duyck 2019-08-15 23:00 ` Alexander Duyck 2019-08-16 18:35 ` Nitesh Narayan Lal 2019-08-16 18:35 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-30 15:15 ` Nitesh Narayan Lal 2019-08-30 15:15 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-30 15:31 ` Alexander Duyck 2019-08-30 15:31 ` [virtio-dev] " Alexander Duyck 2019-08-30 15:31 ` Alexander Duyck 2019-08-30 16:05 ` Nitesh Narayan Lal 2019-08-30 16:05 ` [virtio-dev] " Nitesh Narayan Lal 2019-09-04 8:40 ` David Hildenbrand 2019-09-04 8:40 ` David Hildenbrand 2019-10-10 20:36 ` Alexander Duyck 2019-10-10 20:36 ` [virtio-dev] " Alexander Duyck 2019-10-10 20:36 ` Alexander Duyck 2019-10-11 11:02 ` Nitesh Narayan Lal 2019-10-11 11:02 ` [virtio-dev] " 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-12 13:12 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-14 10:29 ` Cornelia Huck 2019-08-14 10:29 ` [virtio-dev] " Cornelia Huck 2019-08-14 11:47 ` Nitesh Narayan Lal 2019-08-14 11:47 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-14 13:42 ` Cornelia Huck 2019-08-14 13:42 ` [virtio-dev] " Cornelia Huck 2019-08-14 14:01 ` Nitesh Narayan Lal 2019-08-14 14:01 ` [virtio-dev] " 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 ` [virtio-dev] " 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 13:13 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-12 15:18 ` Alexander Duyck 2019-08-12 15:18 ` [virtio-dev] " Alexander Duyck 2019-08-12 15:18 ` Alexander Duyck 2019-08-12 15:26 ` Nitesh Narayan Lal 2019-08-12 15:26 ` [virtio-dev] " Nitesh Narayan Lal 2019-09-11 12:30 ` [RFC][PATCH v12 0/2] mm: Support for " David Hildenbrand 2019-09-11 12:30 ` [virtio-dev] " 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=d39504c9-93bd-b8f7-e119-84baac5a42d4@redhat.com \ --to=nitesh@redhat.com \ --cc=aarcange@redhat.com \ --cc=alexander.duyck@gmail.com \ --cc=cohuck@redhat.com \ --cc=dave.hansen@intel.com \ --cc=david@redhat.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=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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.