| Message ID | 20210106084739.63318-2-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some bugs about HugeTLB code | expand |
On Wed 06-01-21 16:47:34, Muchun Song wrote: > If the refcount is one when it is migrated, it means that the page > was freed from under us. So we are done and do not need to migrate. Is this common enough that it would warrant the explicit check for each migration?
On Thu, Jan 7, 2021 at 12:13 AM Michal Hocko <mhocko@suse.com> wrote: > > On Wed 06-01-21 16:47:34, Muchun Song wrote: > > If the refcount is one when it is migrated, it means that the page > > was freed from under us. So we are done and do not need to migrate. > > Is this common enough that it would warrant the explicit check for each > migration? Are you worried about the overhead caused by the check? Thanks. > -- > Michal Hocko > SUSE Labs
On Thu 07-01-21 10:52:21, Muchun Song wrote: > On Thu, Jan 7, 2021 at 12:13 AM Michal Hocko <mhocko@suse.com> wrote: > > > > On Wed 06-01-21 16:47:34, Muchun Song wrote: > > > If the refcount is one when it is migrated, it means that the page > > > was freed from under us. So we are done and do not need to migrate. > > > > Is this common enough that it would warrant the explicit check for each > > migration? > > Are you worried about the overhead caused by the check? Thanks. I am not as worried as I would like to see some actual justification for this optimization.
On Thu, Jan 7, 2021 at 7:16 PM Michal Hocko <mhocko@suse.com> wrote: > > On Thu 07-01-21 10:52:21, Muchun Song wrote: > > On Thu, Jan 7, 2021 at 12:13 AM Michal Hocko <mhocko@suse.com> wrote: > > > > > > On Wed 06-01-21 16:47:34, Muchun Song wrote: > > > > If the refcount is one when it is migrated, it means that the page > > > > was freed from under us. So we are done and do not need to migrate. > > > > > > Is this common enough that it would warrant the explicit check for each > > > migration? > > > > Are you worried about the overhead caused by the check? Thanks. > > I am not as worried as I would like to see some actual justification for > this optimization. OK. The HugeTLB page can be freed after it was isolated but before being migrated. Right? If we catch this case, it is an optimization. I do this just like unmap_and_move() does for a base page. > -- > Michal Hocko > SUSE Labs
On Thu 07-01-21 19:24:59, Muchun Song wrote: > On Thu, Jan 7, 2021 at 7:16 PM Michal Hocko <mhocko@suse.com> wrote: > > > > On Thu 07-01-21 10:52:21, Muchun Song wrote: > > > On Thu, Jan 7, 2021 at 12:13 AM Michal Hocko <mhocko@suse.com> wrote: > > > > > > > > On Wed 06-01-21 16:47:34, Muchun Song wrote: > > > > > If the refcount is one when it is migrated, it means that the page > > > > > was freed from under us. So we are done and do not need to migrate. > > > > > > > > Is this common enough that it would warrant the explicit check for each > > > > migration? > > > > > > Are you worried about the overhead caused by the check? Thanks. > > > > I am not as worried as I would like to see some actual justification for > > this optimization. > > OK. The HugeTLB page can be freed after it was isolated but before > being migrated. Right? If we catch this case, it is an optimization. > I do this just like unmap_and_move() does for a base page. If your only justification is that this path to be consistent with the regular pages then make it explicit in the changelog. Please think of people reading this code in the future and scratching their heads whether this is something that can be altered and fail to find the reasoning. Was this a huge optimization and some workload might suffer? Can this happen in the first place or how likely it is?
diff --git a/mm/migrate.c b/mm/migrate.c index 4385f2fb5d18..a6631c4eb6a6 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1279,6 +1279,12 @@ static int unmap_and_move_huge_page(new_page_t get_new_page, return -ENOSYS; } + if (page_count(hpage) == 1) { + /* page was freed from under us. So we are done. */ + putback_active_hugepage(hpage); + return MIGRATEPAGE_SUCCESS; + } + new_hpage = get_new_page(hpage, private); if (!new_hpage) return -ENOMEM;