On Thu, Jan 09, 2014 at 04:04:40PM +0900, Joonsoo Kim wrote: > Hello, > > I found some weaknesses on handling migratetype during code review and > testing CMA. > > First, we don't have any synchronization method on get/set pageblock > migratetype. When we change migratetype, we hold the zone lock. So > writer-writer race doesn't exist. But while someone changes migratetype, > others can get migratetype. This may introduce totally unintended value > as migratetype. Although I haven't heard of any problem report about > that, it is better to protect properly. > This is deliberate. The migratetypes for the majority of users are advisory and aimed for fragmentation avoidance. It was important that the cost of that be kept as low as possible and the general case is that migration types change very rarely. In many cases, the zone lock is held. In other cases, such as splitting free pages, the cost is simply not justified. I doubt there is any amount of data you could add in support that would justify hammering the free fast paths (which call get_pageblock_type). > Second, (get/set)_freepage_migrate isn't used properly. I guess that it > would be introduced for per cpu page(pcp) performance, but, it is also > used by memory isolation, now. For that case, the information isn't > enough to use, so we need to fix it. > > Third, there is the problem on buddy allocator. It doesn't consider > migratetype when merging buddy, so pages from cma or isolate region can > be moved to other migratetype freelist. It makes CMA failed over and over. > To prevent it, the buddy allocator should consider migratetype if > CMA/ISOLATE is enabled. Without loioing at the patches, this is likely to add some cost to the page free fast path -- heavy cost if it's a pageblock lookup and lighter cost if you are using cached page information which is potentially stale. Why not force CMA regions to be aligned on MAX_ORDER_NR_PAGES boundary instead to avoid any possibility of merging issues? > This patchset is aimed at fixing these problems and based on v3.13-rc7. > > mm/page_alloc: synchronize get/set pageblock cost with no justification. > mm/cma: fix cma free page accounting sounds like it would be a fix but unrelated to the leader and should be seperated out on its own > mm/page_alloc: move set_freepage_migratetype() to better place Very vague. If this does something useful then it could do with a better subject. > mm/isolation: remove invalid check condition Looks harmless. > mm/page_alloc: separate interface to set/get migratetype of freepage > mm/page_alloc: store freelist migratetype to the page on buddy > properly Potentially sounds useful > mm/page_alloc: don't merge MIGRATE_(CMA|ISOLATE) pages on buddy > Sounds unnecessary if CMA regions were MAX_ORDER_NR_PAGES aligned and then the free paths would be unaffected for everybody. I didn't look at the patches because it felt like cost without any supporting justification for the patches. Superficially it looks like patch 1 needs to go away and the last patch could be done without affected !CMA users. The rest are potentially useful but there should have been some supporting data on how it helps CMA with some backup showing that the page allocation paths are not impacted as a result. -- Mel Gorman SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>