| Message ID | 644a666e1e177c57a39a4c37c6e2ca64052b9d7e.1629008158.git.baolin.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Some cleanup for page migration | expand |
On Sun, Aug 15, 2021 at 02:23:03PM +0800, Baolin Wang wrote: > We've got the expected count for anonymous page or file page by > expected_page_refs() at the beginning of migrate_page_move_mapping(), > thus we should move the page count validation a little forward to > reduce duplicated code. > > Moreover the i_pages lock is not used to guarantee the page refcount > safety in migrate_page_move_mapping(), so we can move the getting page > count out of the i_pages lock. Since now the migration page has > established a migration pte under the page lock now, with the page > refcount freezing validation, to ensure that the page references > meet the migration requirement. I remain unconvinced by this. Looking at folio_migrate_mapping() a little more deeply, I don't understand why we first check folio_ref_count() and then attempt to free the refcount. Why not just try to freeze it directly? ie instead of your patch, this: +++ b/mm/migrate.c @@ -403,13 +403,8 @@ int folio_migrate_mapping(struct address_space *mapping, newzone = folio_zone(newfolio); xas_lock_irq(&xas); - if (folio_ref_count(folio) != expected_count || - xas_load(&xas) != folio) { - xas_unlock_irq(&xas); - return -EAGAIN; - } - - if (!folio_ref_freeze(folio, expected_count)) { + if (xas_load(&xas) != folio || + !folio_ref_freeze(folio, expected_count)) { xas_unlock_irq(&xas); return -EAGAIN; } And since we've got the lock on the page, how can somebody else be removing it from the page cache? I think that xas_load() can be removed too. So even more simply, +++ b/mm/migrate.c @@ -403,12 +403,6 @@ int folio_migrate_mapping(struct address_space *mapping, newzone = folio_zone(newfolio); xas_lock_irq(&xas); - if (folio_ref_count(folio) != expected_count || - xas_load(&xas) != folio) { - xas_unlock_irq(&xas); - return -EAGAIN; - } - if (!folio_ref_freeze(folio, expected_count)) { xas_unlock_irq(&xas); return -EAGAIN; but I'm not really set up to test page migration. Does your test suite test migrating file-backed pages?
On 2021/8/15 18:34, Matthew Wilcox wrote: > On Sun, Aug 15, 2021 at 02:23:03PM +0800, Baolin Wang wrote: >> We've got the expected count for anonymous page or file page by >> expected_page_refs() at the beginning of migrate_page_move_mapping(), >> thus we should move the page count validation a little forward to >> reduce duplicated code. >> >> Moreover the i_pages lock is not used to guarantee the page refcount >> safety in migrate_page_move_mapping(), so we can move the getting page >> count out of the i_pages lock. Since now the migration page has >> established a migration pte under the page lock now, with the page >> refcount freezing validation, to ensure that the page references >> meet the migration requirement. > > I remain unconvinced by this. > > Looking at folio_migrate_mapping() a little more deeply, I don't > understand why we first check folio_ref_count() and then attempt > to free the refcount. Why not just try to freeze it directly? > > ie instead of your patch, this: > > +++ b/mm/migrate.c > @@ -403,13 +403,8 @@ int folio_migrate_mapping(struct address_space *mapping, > newzone = folio_zone(newfolio); > > xas_lock_irq(&xas); > - if (folio_ref_count(folio) != expected_count || > - xas_load(&xas) != folio) { > - xas_unlock_irq(&xas); > - return -EAGAIN; > - } > - > - if (!folio_ref_freeze(folio, expected_count)) { > + if (xas_load(&xas) != folio || > + !folio_ref_freeze(folio, expected_count)) { > xas_unlock_irq(&xas); > return -EAGAIN; > } I think this is reasonable, like what we've done in __remove_mapping(). > And since we've got the lock on the page, how can somebody else be > removing it from the page cache? I think that xas_load() can be > removed too. So even more simply, Good point, this is more simply. > > +++ b/mm/migrate.c > @@ -403,12 +403,6 @@ int folio_migrate_mapping(struct address_space *mapping, > newzone = folio_zone(newfolio); > > xas_lock_irq(&xas); > - if (folio_ref_count(folio) != expected_count || > - xas_load(&xas) != folio) { > - xas_unlock_irq(&xas); > - return -EAGAIN; > - } > - > if (!folio_ref_freeze(folio, expected_count)) { > xas_unlock_irq(&xas); > return -EAGAIN; > > but I'm not really set up to test page migration. Does your test suite > test migrating file-backed pages? Yes, I've tested above changes, and the mapped file pages migration works well. So can I resend this patch set with your Suggested-by tag? Thanks.
diff --git a/mm/migrate.c b/mm/migrate.c index c6eb2a8..8e9282f 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -386,11 +386,10 @@ int folio_migrate_mapping(struct address_space *mapping, int expected_count = expected_page_refs(mapping, &folio->page) + extra_count; long nr = folio_nr_pages(folio); - if (!mapping) { - /* Anonymous page without mapping */ - if (folio_ref_count(folio) != expected_count) - return -EAGAIN; + if (folio_ref_count(folio) != expected_count) + return -EAGAIN; + if (!mapping) { /* No turning back from here */ newfolio->index = folio->index; newfolio->mapping = folio->mapping; @@ -404,8 +403,7 @@ int folio_migrate_mapping(struct address_space *mapping, newzone = folio_zone(newfolio); xas_lock_irq(&xas); - if (folio_ref_count(folio) != expected_count || - xas_load(&xas) != folio) { + if (xas_load(&xas) != folio) { xas_unlock_irq(&xas); return -EAGAIN; }
We've got the expected count for anonymous page or file page by expected_page_refs() at the beginning of migrate_page_move_mapping(), thus we should move the page count validation a little forward to reduce duplicated code. Moreover the i_pages lock is not used to guarantee the page refcount safety in migrate_page_move_mapping(), so we can move the getting page count out of the i_pages lock. Since now the migration page has established a migration pte under the page lock now, with the page refcount freezing validation, to ensure that the page references meet the migration requirement. Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> --- mm/migrate.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)