| Message ID | 1562307161-30554-1-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/isolate: Drop pre-validating migrate type in undo_isolate_page_range() | expand |
On Fri, Jul 05, 2019 at 11:42:41AM +0530, Anshuman Khandual wrote: > unset_migratetype_isolate() already validates under zone lock that a given > page has already been isolated as MIGRATE_ISOLATE. There is no need for > another check before. Hence just drop this redundant validation. > > Cc: Oscar Salvador <osalvador@suse.de> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Qian Cai <cai@lca.pw> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: linux-mm@kvack.org > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > Is there any particular reason to do this migratetype pre-check without zone > lock before calling unsert_migrate_isolate() ? If not this should be removed. I have seen this kinda behavior-checks all over the kernel. I guess that one of the main goals is to avoid lock contention, so we check if the page has the right migratetype, and then we check it again under the lock to see whether that has changed. e.g: simultaneous calls to undo_isolate_page_range But I am not sure if the motivation behind was something else, as the changelog that added this code was quite modest. Anyway, how did you come across with this? Do things get speed up without this check? Or what was the motivation to remove it? thanks > > mm/page_isolation.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/page_isolation.c b/mm/page_isolation.c > index e3638a5bafff..f529d250c8a5 100644 > --- a/mm/page_isolation.c > +++ b/mm/page_isolation.c > @@ -243,7 +243,7 @@ int undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, > pfn < end_pfn; > pfn += pageblock_nr_pages) { > page = __first_valid_page(pfn, pageblock_nr_pages); > - if (!page || !is_migrate_isolate_page(page)) > + if (!page) > continue; > unset_migratetype_isolate(page, migratetype); > } > -- > 2.20.1 >
On 07/05/2019 01:29 PM, Oscar Salvador wrote: > On Fri, Jul 05, 2019 at 11:42:41AM +0530, Anshuman Khandual wrote: >> unset_migratetype_isolate() already validates under zone lock that a given >> page has already been isolated as MIGRATE_ISOLATE. There is no need for >> another check before. Hence just drop this redundant validation. >> >> Cc: Oscar Salvador <osalvador@suse.de> >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: Qian Cai <cai@lca.pw> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: linux-mm@kvack.org >> Cc: linux-kernel@vger.kernel.org >> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> Is there any particular reason to do this migratetype pre-check without zone >> lock before calling unsert_migrate_isolate() ? If not this should be removed. > > I have seen this kinda behavior-checks all over the kernel. > I guess that one of the main goals is to avoid lock contention, so we check > if the page has the right migratetype, and then we check it again under the lock > to see whether that has changed. So the worst case when it becomes redundant might not affect the performance much ? > > e.g: simultaneous calls to undo_isolate_page_range Right. > > But I am not sure if the motivation behind was something else, as the changelog > that added this code was quite modest. Agreed. > > Anyway, how did you come across with this? > Do things get speed up without this check? Or what was the motivation to remove it? Detected this during a code audit. I figured it can help save some cycles. The other call site start_isolate_page_range() does not check migrate type because the page block is guaranteed to be MIGRATE_ISOLATE ? I am not sure if a non-lock check first in this case is actually improving performance. In which case should we just leave the check as is ?
On Fri 05-07-19 11:42:41, Anshuman Khandual wrote: > unset_migratetype_isolate() already validates under zone lock that a given > page has already been isolated as MIGRATE_ISOLATE. There is no need for > another check before. Hence just drop this redundant validation. unset_migratetype_isolate take zone lock and it is always preferable to skip not take this lock if we know it would be pointless. Besides that undo_isolate_page_range is a slow path so a nano optimizing it is not worth it. > Cc: Oscar Salvador <osalvador@suse.de> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Qian Cai <cai@lca.pw> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: linux-mm@kvack.org > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > Is there any particular reason to do this migratetype pre-check without zone > lock before calling unsert_migrate_isolate() ? If not this should be removed. > > mm/page_isolation.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/page_isolation.c b/mm/page_isolation.c > index e3638a5bafff..f529d250c8a5 100644 > --- a/mm/page_isolation.c > +++ b/mm/page_isolation.c > @@ -243,7 +243,7 @@ int undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, > pfn < end_pfn; > pfn += pageblock_nr_pages) { > page = __first_valid_page(pfn, pageblock_nr_pages); > - if (!page || !is_migrate_isolate_page(page)) > + if (!page) > continue; > unset_migratetype_isolate(page, migratetype); > } > -- > 2.20.1
diff --git a/mm/page_isolation.c b/mm/page_isolation.c index e3638a5bafff..f529d250c8a5 100644 --- a/mm/page_isolation.c +++ b/mm/page_isolation.c @@ -243,7 +243,7 @@ int undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, pfn < end_pfn; pfn += pageblock_nr_pages) { page = __first_valid_page(pfn, pageblock_nr_pages); - if (!page || !is_migrate_isolate_page(page)) + if (!page) continue; unset_migratetype_isolate(page, migratetype); }
unset_migratetype_isolate() already validates under zone lock that a given page has already been isolated as MIGRATE_ISOLATE. There is no need for another check before. Hence just drop this redundant validation. Cc: Oscar Salvador <osalvador@suse.de> Cc: Michal Hocko <mhocko@suse.com> Cc: Qian Cai <cai@lca.pw> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: linux-mm@kvack.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- Is there any particular reason to do this migratetype pre-check without zone lock before calling unsert_migrate_isolate() ? If not this should be removed. mm/page_isolation.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)