| Message ID | 1565078411-27082-2-git-send-email-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] mm/migrate: clean up useless code in migrate_vma_collect_pmd() | expand |
On Tue, Aug 06, 2019 at 04:00:10PM +0800, Pingfan Liu wrote: > MIGRATE_PFN_MIGRATE marks a valid pfn, further more, suitable to migrate. > As for hole, there is no valid pfn, not to mention migration. > > Before this patch, hole has already relied on the following code to be > filtered out. Hence it is more reasonable to see hole as invalid source > page. > migrate_vma_prepare() > { > struct page *page = migrate_pfn_to_page(migrate->src[i]); > > if (!page || (migrate->src[i] & MIGRATE_PFN_MIGRATE)) > \_ this condition > } NAK you break the API, MIGRATE_PFN_MIGRATE is use for 2 things, first it allow the collection code to mark entry that can be migrated, then it use by driver to allow driver to skip migration for some entry (for whatever reason the driver might have), we still need to keep the entry and not clear it so that we can cleanup thing (ie remove migration pte entry). > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> > Cc: "Jérôme Glisse" <jglisse@redhat.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Mel Gorman <mgorman@techsingularity.net> > Cc: Jan Kara <jack@suse.cz> > Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Mike Kravetz <mike.kravetz@oracle.com> > Cc: Andrea Arcangeli <aarcange@redhat.com> > Cc: Matthew Wilcox <willy@infradead.org> > To: linux-mm@kvack.org > Cc: linux-kernel@vger.kernel.org > --- > mm/migrate.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index c2ec614..832483f 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -2136,10 +2136,9 @@ static int migrate_vma_collect_hole(unsigned long start, > unsigned long addr; > > for (addr = start & PAGE_MASK; addr < end; addr += PAGE_SIZE) { > - migrate->src[migrate->npages] = MIGRATE_PFN_MIGRATE; > + migrate->src[migrate->npages] = 0; > migrate->dst[migrate->npages] = 0; > migrate->npages++; > - migrate->cpages++; > } > > return 0; > @@ -2228,8 +2227,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp, > pfn = pte_pfn(pte); > > if (pte_none(pte)) { > - mpfn = MIGRATE_PFN_MIGRATE; > - migrate->cpages++; > + mpfn = 0; > goto next; > } > > -- > 2.7.5 >
On Thu, Aug 15, 2019 at 01:22:22PM -0400, Jerome Glisse wrote: > On Tue, Aug 06, 2019 at 04:00:10PM +0800, Pingfan Liu wrote: > > MIGRATE_PFN_MIGRATE marks a valid pfn, further more, suitable to migrate. > > As for hole, there is no valid pfn, not to mention migration. > > > > Before this patch, hole has already relied on the following code to be > > filtered out. Hence it is more reasonable to see hole as invalid source > > page. > > migrate_vma_prepare() > > { > > struct page *page = migrate_pfn_to_page(migrate->src[i]); > > > > if (!page || (migrate->src[i] & MIGRATE_PFN_MIGRATE)) > > \_ this condition > > } > > NAK you break the API, MIGRATE_PFN_MIGRATE is use for 2 things, > first it allow the collection code to mark entry that can be > migrated, then it use by driver to allow driver to skip migration > for some entry (for whatever reason the driver might have), we > still need to keep the entry and not clear it so that we can > cleanup thing (ie remove migration pte entry). Thanks for your kindly review. I read the code again. Maybe I miss something. But as my understanding, for hole, there is no pte. As the current code migrate_vma_collect_pmd() { if (pmd_none(*pmdp)) return migrate_vma_collect_hole(start, end, walk); ... make_migration_entry() } We do not install migration entry for hole, then no need to remove migration pte entry. And on the driver side, there is way to migrate a hole. The driver just skip it by drivers/gpu/drm/nouveau/nouveau_dmem.c: if (!spage || !(src_pfns[i] & MIGRATE_PFN_MIGRATE)) ^^^^ Finally, in migrate_vma_finalize(), for a hole, if (!page) { if (newpage) { unlock_page(newpage); put_page(newpage); } continue; } And we do not rely on remove_migration_ptes(page, newpage, false); to restore the orignal pte (and it is impossible). Thanks, Pingfan
On Fri, Aug 16, 2019 at 11:02:22PM +0800, Pingfan Liu wrote: > On Thu, Aug 15, 2019 at 01:22:22PM -0400, Jerome Glisse wrote: > > On Tue, Aug 06, 2019 at 04:00:10PM +0800, Pingfan Liu wrote: > > > MIGRATE_PFN_MIGRATE marks a valid pfn, further more, suitable to migrate. > > > As for hole, there is no valid pfn, not to mention migration. > > > > > > Before this patch, hole has already relied on the following code to be > > > filtered out. Hence it is more reasonable to see hole as invalid source > > > page. > > > migrate_vma_prepare() > > > { > > > struct page *page = migrate_pfn_to_page(migrate->src[i]); > > > > > > if (!page || (migrate->src[i] & MIGRATE_PFN_MIGRATE)) > > > \_ this condition > > > } > > > > NAK you break the API, MIGRATE_PFN_MIGRATE is use for 2 things, > > first it allow the collection code to mark entry that can be > > migrated, then it use by driver to allow driver to skip migration > > for some entry (for whatever reason the driver might have), we > > still need to keep the entry and not clear it so that we can > > cleanup thing (ie remove migration pte entry). > Thanks for your kindly review. > > I read the code again. Maybe I miss something. But as my understanding, > for hole, there is no pte. > As the current code migrate_vma_collect_pmd() > { > if (pmd_none(*pmdp)) > return migrate_vma_collect_hole(start, end, walk); > ... > make_migration_entry() > } > > We do not install migration entry for hole, then no need to remove > migration pte entry. > > And on the driver side, there is way to migrate a hole. The driver just > skip it by > drivers/gpu/drm/nouveau/nouveau_dmem.c: if (!spage || !(src_pfns[i] & MIGRATE_PFN_MIGRATE)) > ^^^^ > Finally, in migrate_vma_finalize(), for a hole, > if (!page) { > if (newpage) { > unlock_page(newpage); > put_page(newpage); > } > continue; > } > And we do not rely on remove_migration_ptes(page, newpage, false); to > restore the orignal pte (and it is impossible). > Hello, do you have any comment? I think most of important, hole does not use the 'MIGRATE_PFN_MIGRATE' API. Hole has not pte, and there is no way to 'remove migration pte entry'. Further more, we can know the failure on the source side, no need to defer it to driver side. By this way, [3/3] can unify the code. Thanks, Pingfan
diff --git a/mm/migrate.c b/mm/migrate.c index c2ec614..832483f 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -2136,10 +2136,9 @@ static int migrate_vma_collect_hole(unsigned long start, unsigned long addr; for (addr = start & PAGE_MASK; addr < end; addr += PAGE_SIZE) { - migrate->src[migrate->npages] = MIGRATE_PFN_MIGRATE; + migrate->src[migrate->npages] = 0; migrate->dst[migrate->npages] = 0; migrate->npages++; - migrate->cpages++; } return 0; @@ -2228,8 +2227,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp, pfn = pte_pfn(pte); if (pte_none(pte)) { - mpfn = MIGRATE_PFN_MIGRATE; - migrate->cpages++; + mpfn = 0; goto next; }
MIGRATE_PFN_MIGRATE marks a valid pfn, further more, suitable to migrate. As for hole, there is no valid pfn, not to mention migration. Before this patch, hole has already relied on the following code to be filtered out. Hence it is more reasonable to see hole as invalid source page. migrate_vma_prepare() { struct page *page = migrate_pfn_to_page(migrate->src[i]); if (!page || (migrate->src[i] & MIGRATE_PFN_MIGRATE)) \_ this condition } Signed-off-by: Pingfan Liu <kernelfans@gmail.com> Cc: "Jérôme Glisse" <jglisse@redhat.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Mel Gorman <mgorman@techsingularity.net> Cc: Jan Kara <jack@suse.cz> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> Cc: Michal Hocko <mhocko@suse.com> Cc: Mike Kravetz <mike.kravetz@oracle.com> Cc: Andrea Arcangeli <aarcange@redhat.com> Cc: Matthew Wilcox <willy@infradead.org> To: linux-mm@kvack.org Cc: linux-kernel@vger.kernel.org --- mm/migrate.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)