From: David Hildenbrand <david@redhat.com> To: Nitesh Narayan Lal <nitesh@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 12:34:04 +0200 [thread overview] Message-ID: <32f61f87-6205-5001-866c-a84e20fc9d85@redhat.com> (raw) In-Reply-To: <d39504c9-93bd-b8f7-e119-84baac5a42d4@redhat.com> >>>> +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 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? If it's not needed, I'd say drop it (eventually add a comment). -- Thanks, David / dhildenb
WARNING: multiple messages have this Message-ID (diff)
From: David Hildenbrand <david@redhat.com> To: Nitesh Narayan Lal <nitesh@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 12:34:04 +0200 [thread overview] Message-ID: <32f61f87-6205-5001-866c-a84e20fc9d85@redhat.com> (raw) In-Reply-To: <d39504c9-93bd-b8f7-e119-84baac5a42d4@redhat.com> >>>> +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 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? If it's not needed, I'd say drop it (eventually add a comment). -- Thanks, David / dhildenb --------------------------------------------------------------------- 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:34 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 2019-08-13 10:30 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-13 10:34 ` David Hildenbrand [this message] 2019-08-13 10:34 ` 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=32f61f87-6205-5001-866c-a84e20fc9d85@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: 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.