On Tue 12-11-19 09:04:01, Michal Hocko wrote: > On Tue 12-11-19 06:09:25, Yang Shi wrote: > > When doing migration if the freed page is met, we just return without > > migrating it since it is pointless to migrate a freed page. But, the > > current code did two things before handling freed page: > > > > 1. Return -ENOMEM if the page is THP and THP migration is not supported. > > 2. Allocate target page unconditionally. > > > > Both makes not too much sense. If we handle freed page at the first place > > we don't have to worry about allocating/freeing target page and split > > THP at all. > > > > For example (worst case) if we are trying to migrate a freed THP without > > THP migration supported, the migrate_pages() would just split the THP then > > retry to migrate base pages one by one by pointless allocating and freeing > > pages, this is just waste of time. > > > > I didn't run into any actual problem with the current code (or I may > > just not notice it yet), it was found by visual inspection. > > It would be preferable to accompany a change like this with some actual > numbers. A race with page freeing should be a very rare situation. Maybe > it is not under some workloads but that would better be checked and > documented. I also do not like to do page state changes for THP > migration without a support. I cannot really say this is 100% correct > from top of my head and I do not see a sufficient justification to go > and chase all those tiny details because that is time consuming. And I forgot to mention one thing. I wouldn't be really opposed to moving the allocation after the race check because that makes sense even when the race is rare but moving the thp support check down is far from clear without a much better justification. -- Michal Hocko SUSE Labs
