| Message ID | 20181211172143.7358-2-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] mm: migration: Factor out code to compute expected number of page references | expand |
On Tue, Dec 11, 2018 at 06:21:38PM +0100, Jan Kara wrote: > Factor out function to compute number of expected page references in > migrate_page_move_mapping(). Note that we move hpage_nr_pages() and > page_has_private() checks from under xas_lock_irq() however this is safe > since we hold page lock. > > Signed-off-by: Jan Kara <jack@suse.cz> Acked-by: Mel Gorman <mgorman@suse.de>
On Tue, Dec 11, 2018 at 06:21:38PM +0100, Jan Kara wrote: > Factor out function to compute number of expected page references in > migrate_page_move_mapping(). Note that we move hpage_nr_pages() and > page_has_private() checks from under xas_lock_irq() however this is safe > since we hold page lock. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > mm/migrate.c | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index f7e4bfdc13b7..789c7bc90a0c 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -428,6 +428,22 @@ static inline bool buffer_migrate_lock_buffers(struct buffer_head *head, > } > #endif /* CONFIG_BLOCK */ > > +static int expected_page_refs(struct page *page) > +{ > + int expected_count = 1; > + > + /* > + * Device public or private pages have an extra refcount as they are > + * ZONE_DEVICE pages. > + */ > + expected_count += is_device_private_page(page); > + expected_count += is_device_public_page(page); > + if (page->mapping) > + expected_count += hpage_nr_pages(page) + page_has_private(page); > + > + return expected_count; > +} > + I noticed during testing that THP allocation success rates under the mmtests configuration global-dhp__workload_thpscale-madvhugepage-xfs were terrible with massive latencies introduced somewhere in the series. I haven't tried chasing it down as it's relatively late but this block looked odd and I missed it the first time. This page->mapping test is relevant for the "Anonymous page without mapping" check but I think it's wrong. An anonymous page without mapping doesn't have a NULL mapping, it sets PAGE_MAPPING_ANON and the field can be special in other ways. I think you meant to use page_mapping(page) here, not page->mapping?
On Fri 14-12-18 15:10:46, Mel Gorman wrote: > On Tue, Dec 11, 2018 at 06:21:38PM +0100, Jan Kara wrote: > > Factor out function to compute number of expected page references in > > migrate_page_move_mapping(). Note that we move hpage_nr_pages() and > > page_has_private() checks from under xas_lock_irq() however this is safe > > since we hold page lock. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > mm/migrate.c | 27 +++++++++++++++++---------- > > 1 file changed, 17 insertions(+), 10 deletions(-) > > > > diff --git a/mm/migrate.c b/mm/migrate.c > > index f7e4bfdc13b7..789c7bc90a0c 100644 > > --- a/mm/migrate.c > > +++ b/mm/migrate.c > > @@ -428,6 +428,22 @@ static inline bool buffer_migrate_lock_buffers(struct buffer_head *head, > > } > > #endif /* CONFIG_BLOCK */ > > > > +static int expected_page_refs(struct page *page) > > +{ > > + int expected_count = 1; > > + > > + /* > > + * Device public or private pages have an extra refcount as they are > > + * ZONE_DEVICE pages. > > + */ > > + expected_count += is_device_private_page(page); > > + expected_count += is_device_public_page(page); > > + if (page->mapping) > > + expected_count += hpage_nr_pages(page) + page_has_private(page); > > + > > + return expected_count; > > +} > > + > > I noticed during testing that THP allocation success rates under the > mmtests configuration global-dhp__workload_thpscale-madvhugepage-xfs were > terrible with massive latencies introduced somewhere in the series. I > haven't tried chasing it down as it's relatively late but this block > looked odd and I missed it the first time. Interesting. I've run config-global-dhp__workload_thpscale and that didn't show anything strange. But the numbers were fluctuating a lot both with and without my patches applied. I'll have a look if I can reproduce this sometime next week and look what could be causing the delays. > This page->mapping test is relevant for the "Anonymous page without > mapping" check but I think it's wrong. An anonymous page without mapping > doesn't have a NULL mapping, it sets PAGE_MAPPING_ANON and the field can > be special in other ways. I think you meant to use page_mapping(page) > here, not page->mapping? Yes, that's a bug. It should have been page_mapping(page). Thanks for catching this. Honza
On Fri, Dec 14, 2018 at 04:53:11PM +0100, Jan Kara wrote: > > I noticed during testing that THP allocation success rates under the > > mmtests configuration global-dhp__workload_thpscale-madvhugepage-xfs were > > terrible with massive latencies introduced somewhere in the series. I > > haven't tried chasing it down as it's relatively late but this block > > looked odd and I missed it the first time. > > Interesting. I've run config-global-dhp__workload_thpscale and that didn't > show anything strange. But the numbers were fluctuating a lot both with and > without my patches applied. I'll have a look if I can reproduce this > sometime next week and look what could be causing the delays. > Ah, it's the difference between madvise and !madvise. The configuration you used does very little compaction as it neither wakes kswapd of kcompactd. It just falls back to base pages to limit fault latency so you wouldn't have hit the same paths of interest. > > This page->mapping test is relevant for the "Anonymous page without > > mapping" check but I think it's wrong. An anonymous page without mapping > > doesn't have a NULL mapping, it sets PAGE_MAPPING_ANON and the field can > > be special in other ways. I think you meant to use page_mapping(page) > > here, not page->mapping? > > Yes, that's a bug. It should have been page_mapping(page). Thanks for > catching this. > My pleasure, should have spotted it the first time around :/
On Fri 14-12-18 16:24:28, Mel Gorman wrote: > On Fri, Dec 14, 2018 at 04:53:11PM +0100, Jan Kara wrote: > > > This page->mapping test is relevant for the "Anonymous page without > > > mapping" check but I think it's wrong. An anonymous page without mapping > > > doesn't have a NULL mapping, it sets PAGE_MAPPING_ANON and the field can > > > be special in other ways. I think you meant to use page_mapping(page) > > > here, not page->mapping? > > > > Yes, that's a bug. It should have been page_mapping(page). Thanks for > > catching this. > > > > My pleasure, should have spotted it the first time around :/ And after fixing this, I can see the success rates for global-dhp__workload_thpscale-madvhugepage to go back to the original values. So that was indeed the problem. I'll send the fixup to Andrew. Honza
diff --git a/mm/migrate.c b/mm/migrate.c index f7e4bfdc13b7..789c7bc90a0c 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -428,6 +428,22 @@ static inline bool buffer_migrate_lock_buffers(struct buffer_head *head, } #endif /* CONFIG_BLOCK */ +static int expected_page_refs(struct page *page) +{ + int expected_count = 1; + + /* + * Device public or private pages have an extra refcount as they are + * ZONE_DEVICE pages. + */ + expected_count += is_device_private_page(page); + expected_count += is_device_public_page(page); + if (page->mapping) + expected_count += hpage_nr_pages(page) + page_has_private(page); + + return expected_count; +} + /* * Replace the page in the mapping. * @@ -444,14 +460,7 @@ int migrate_page_move_mapping(struct address_space *mapping, XA_STATE(xas, &mapping->i_pages, page_index(page)); struct zone *oldzone, *newzone; int dirty; - int expected_count = 1 + extra_count; - - /* - * Device public or private pages have an extra refcount as they are - * ZONE_DEVICE pages. - */ - expected_count += is_device_private_page(page); - expected_count += is_device_public_page(page); + int expected_count = expected_page_refs(page) + extra_count; if (!mapping) { /* Anonymous page without mapping */ @@ -471,8 +480,6 @@ int migrate_page_move_mapping(struct address_space *mapping, newzone = page_zone(newpage); xas_lock_irq(&xas); - - expected_count += hpage_nr_pages(page) + page_has_private(page); if (page_count(page) != expected_count || xas_load(&xas) != page) { xas_unlock_irq(&xas); return -EAGAIN;
Factor out function to compute number of expected page references in migrate_page_move_mapping(). Note that we move hpage_nr_pages() and page_has_private() checks from under xas_lock_irq() however this is safe since we hold page lock. Signed-off-by: Jan Kara <jack@suse.cz> --- mm/migrate.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)