| Message ID | 20181218204656.4297-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm, page_isolation: remove drain_all_pages() in set_migratetype_isolate() | expand |
On 18.12.18 21:46, Wei Yang wrote: > Below is a brief call flow for __offline_pages() and > alloc_contig_range(): > > __offline_pages()/alloc_contig_range() > start_isolate_page_range() > set_migratetype_isolate() > drain_all_pages() > drain_all_pages() > > Current logic is: isolate and drain pcp list for each pageblock and > drain pcp list again. This is not necessary and we could just drain pcp > list once after isolate this whole range. > > The reason is start_isolate_page_range() will set the migrate type of > a range to MIGRATE_ISOLATE. After doing so, this range will never be > allocated from Buddy, neither to a real user nor to pcp list. > > Since drain_all_pages() is zone based, by reduce times of > drain_all_pages() also reduce some contention on this particular zone. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> Yes, as far as I can see, when a MIGRATE_ISOLATE page gets freed, it will not go onto the pcp list again. However, start_isolate_page_range() is also called via alloc_contig_range(). Are you sure we can effectively drop the drain_all_pages() for that call path? > > --- > v2: adjust changelog with MIGRATE_ISOLATE effects for the isolated range > --- > mm/page_isolation.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/mm/page_isolation.c b/mm/page_isolation.c > index 43e085608846..f44c0e333bed 100644 > --- a/mm/page_isolation.c > +++ b/mm/page_isolation.c > @@ -83,8 +83,6 @@ static int set_migratetype_isolate(struct page *page, int migratetype, > } > > spin_unlock_irqrestore(&zone->lock, flags); > - if (!ret) > - drain_all_pages(zone); > return ret; > } > >
On Tue, Dec 18, 2018 at 10:14:25PM +0100, David Hildenbrand wrote: >On 18.12.18 21:46, Wei Yang wrote: >> Below is a brief call flow for __offline_pages() and >> alloc_contig_range(): >> >> __offline_pages()/alloc_contig_range() >> start_isolate_page_range() >> set_migratetype_isolate() >> drain_all_pages() >> drain_all_pages() >> >> Current logic is: isolate and drain pcp list for each pageblock and >> drain pcp list again. This is not necessary and we could just drain pcp >> list once after isolate this whole range. >> >> The reason is start_isolate_page_range() will set the migrate type of >> a range to MIGRATE_ISOLATE. After doing so, this range will never be >> allocated from Buddy, neither to a real user nor to pcp list. >> >> Since drain_all_pages() is zone based, by reduce times of >> drain_all_pages() also reduce some contention on this particular zone. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > >Yes, as far as I can see, when a MIGRATE_ISOLATE page gets freed, it >will not go onto the pcp list again. > >However, start_isolate_page_range() is also called via >alloc_contig_range(). Are you sure we can effectively drop the >drain_all_pages() for that call path? > alloc_contig_range() does following now: - isolate page range - do reclaim and migration - drain lru - drain pcp list If step 2 fails, it will not drain lru and pcp list. I don't see we have to drain pcp list before step 2. And after this change, it will save some effort if step 2 fails. >> >> --- >> v2: adjust changelog with MIGRATE_ISOLATE effects for the isolated range >> --- >> mm/page_isolation.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/mm/page_isolation.c b/mm/page_isolation.c >> index 43e085608846..f44c0e333bed 100644 >> --- a/mm/page_isolation.c >> +++ b/mm/page_isolation.c >> @@ -83,8 +83,6 @@ static int set_migratetype_isolate(struct page *page, int migratetype, >> } >> >> spin_unlock_irqrestore(&zone->lock, flags); >> - if (!ret) >> - drain_all_pages(zone); >> return ret; >> } >> >> > > >-- > >Thanks, > >David / dhildenb
On 18.12.18 22:49, Wei Yang wrote: > On Tue, Dec 18, 2018 at 10:14:25PM +0100, David Hildenbrand wrote: >> On 18.12.18 21:46, Wei Yang wrote: >>> Below is a brief call flow for __offline_pages() and >>> alloc_contig_range(): >>> >>> __offline_pages()/alloc_contig_range() >>> start_isolate_page_range() >>> set_migratetype_isolate() >>> drain_all_pages() >>> drain_all_pages() >>> >>> Current logic is: isolate and drain pcp list for each pageblock and >>> drain pcp list again. This is not necessary and we could just drain pcp >>> list once after isolate this whole range. >>> >>> The reason is start_isolate_page_range() will set the migrate type of >>> a range to MIGRATE_ISOLATE. After doing so, this range will never be >>> allocated from Buddy, neither to a real user nor to pcp list. >>> >>> Since drain_all_pages() is zone based, by reduce times of >>> drain_all_pages() also reduce some contention on this particular zone. >>> >>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> >> Yes, as far as I can see, when a MIGRATE_ISOLATE page gets freed, it >> will not go onto the pcp list again. >> >> However, start_isolate_page_range() is also called via >> alloc_contig_range(). Are you sure we can effectively drop the >> drain_all_pages() for that call path? >> > > alloc_contig_range() does following now: > > - isolate page range > - do reclaim and migration > - drain lru > - drain pcp list > > If step 2 fails, it will not drain lru and pcp list. > > I don't see we have to drain pcp list before step 2. And after this > change, it will save some effort if step 2 fails. Sorry, I missed that you actually documented the "alloc_contig_range" scenario in you patch description. My fault! Acked-by: David Hildenbrand <david@redhat.com>
On Wed, Dec 19, 2018 at 04:46:56AM +0800, Wei Yang wrote: > Below is a brief call flow for __offline_pages() and > alloc_contig_range(): > > __offline_pages()/alloc_contig_range() > start_isolate_page_range() > set_migratetype_isolate() > drain_all_pages() > drain_all_pages() > > Current logic is: isolate and drain pcp list for each pageblock and > drain pcp list again. This is not necessary and we could just drain pcp > list once after isolate this whole range. > > The reason is start_isolate_page_range() will set the migrate type of > a range to MIGRATE_ISOLATE. After doing so, this range will never be > allocated from Buddy, neither to a real user nor to pcp list. > > Since drain_all_pages() is zone based, by reduce times of > drain_all_pages() also reduce some contention on this particular zone. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> It is a bit late and I hope I did not miss anything, but looks good to me. Reviewed-by: Oscar Salvador <osalvador@suse.de> Thanks!
On Wed 19-12-18 04:46:56, Wei Yang wrote: > Below is a brief call flow for __offline_pages() and > alloc_contig_range(): > > __offline_pages()/alloc_contig_range() > start_isolate_page_range() > set_migratetype_isolate() > drain_all_pages() > drain_all_pages() > > Current logic is: isolate and drain pcp list for each pageblock and > drain pcp list again. This is not necessary and we could just drain pcp > list once after isolate this whole range. > > The reason is start_isolate_page_range() will set the migrate type of > a range to MIGRATE_ISOLATE. After doing so, this range will never be > allocated from Buddy, neither to a real user nor to pcp list. But it is important to note that those pages still can be allocated from the pcp lists until we do drain_all_pages(). One thing that I really do not like about this patch (and I believe I have mentioned that previously) that you rely on callers to do the right thing. The proper fix would be to do the draining in start_isolate_page_range and remove them from callers. Also what does prevent start_isolate_page_range to work on multiple zones? At least contiguous allocator can do that in principle. So no I do not like this patch, it is not an improvement.
On Wed, Dec 19, 2018 at 10:51:10AM +0100, Michal Hocko wrote: > On Wed 19-12-18 04:46:56, Wei Yang wrote: > > Below is a brief call flow for __offline_pages() and > > alloc_contig_range(): > > > > __offline_pages()/alloc_contig_range() > > start_isolate_page_range() > > set_migratetype_isolate() > > drain_all_pages() > > drain_all_pages() > > > > Current logic is: isolate and drain pcp list for each pageblock and > > drain pcp list again. This is not necessary and we could just drain pcp > > list once after isolate this whole range. > > > > The reason is start_isolate_page_range() will set the migrate type of > > a range to MIGRATE_ISOLATE. After doing so, this range will never be > > allocated from Buddy, neither to a real user nor to pcp list. > > But it is important to note that those pages still can be allocated from > the pcp lists until we do drain_all_pages(). I had the same fear, but then I saw that move_freepages_block()->move_freepages() moves the pages to a new list: <-- list_move(&page->lru, &zone->free_area[order].free_list[migratetype]); --> But looking at it again, I see that this is only for BuddyPages, so I guess that pcp-pages do not really get unlinked, so we could still allocate them. Uhmf, I missed that.
On Wed 19-12-18 04:46:56, Wei Yang wrote: [...] > Since drain_all_pages() is zone based, by reduce times of > drain_all_pages() also reduce some contention on this particular zone. I forgot to add. As said before this is a really weak justification. If there is really some contention then I would like to see some numbers backing that claim. A proper justification would be that reallying on draining in callers just sucks. As we can see we are doing that suboptimally based on a weak understanding of the functionality. So it makes sense to remove that draining and rely on the isolation code do the right thing. Then it is a clear cleanup.
On Wed, Dec 19, 2018 at 10:51:10AM +0100, Michal Hocko wrote: >On Wed 19-12-18 04:46:56, Wei Yang wrote: >> Below is a brief call flow for __offline_pages() and >> alloc_contig_range(): >> >> __offline_pages()/alloc_contig_range() >> start_isolate_page_range() >> set_migratetype_isolate() >> drain_all_pages() >> drain_all_pages() >> >> Current logic is: isolate and drain pcp list for each pageblock and >> drain pcp list again. This is not necessary and we could just drain pcp >> list once after isolate this whole range. >> >> The reason is start_isolate_page_range() will set the migrate type of >> a range to MIGRATE_ISOLATE. After doing so, this range will never be >> allocated from Buddy, neither to a real user nor to pcp list. > >But it is important to note that those pages still can be allocated from >the pcp lists until we do drain_all_pages(). > >One thing that I really do not like about this patch (and I believe I >have mentioned that previously) that you rely on callers to do the right >thing. The proper fix would be to do the draining in >start_isolate_page_range and remove them from callers. Also what does Well, I don't really understand this meaning previously. So you prefer set_migratetype_isolate() do the drain instead of the caller (__offline_pages) do the drain. Is my understanding correct? >prevent start_isolate_page_range to work on multiple zones? At least >contiguous allocator can do that in principle. As the comment mentioned, in current implementation the range must be in one zone. > >So no I do not like this patch, it is not an improvement. >-- >Michal Hocko >SUSE Labs
On Wed 19-12-18 13:29:34, Wei Yang wrote: > On Wed, Dec 19, 2018 at 10:51:10AM +0100, Michal Hocko wrote: > >On Wed 19-12-18 04:46:56, Wei Yang wrote: > >> Below is a brief call flow for __offline_pages() and > >> alloc_contig_range(): > >> > >> __offline_pages()/alloc_contig_range() > >> start_isolate_page_range() > >> set_migratetype_isolate() > >> drain_all_pages() > >> drain_all_pages() > >> > >> Current logic is: isolate and drain pcp list for each pageblock and > >> drain pcp list again. This is not necessary and we could just drain pcp > >> list once after isolate this whole range. > >> > >> The reason is start_isolate_page_range() will set the migrate type of > >> a range to MIGRATE_ISOLATE. After doing so, this range will never be > >> allocated from Buddy, neither to a real user nor to pcp list. > > > >But it is important to note that those pages still can be allocated from > >the pcp lists until we do drain_all_pages(). > > > >One thing that I really do not like about this patch (and I believe I > >have mentioned that previously) that you rely on callers to do the right > >thing. The proper fix would be to do the draining in > >start_isolate_page_range and remove them from callers. Also what does > > Well, I don't really understand this meaning previously. > > So you prefer set_migratetype_isolate() do the drain instead of the > caller (__offline_pages) do the drain. Is my understanding correct? Either set_migratetype_isolate or start_isolate_page_range. The later only if this is guaranteed that we cannot intemix zones in the range. > >prevent start_isolate_page_range to work on multiple zones? At least > >contiguous allocator can do that in principle. > > As the comment mentioned, in current implementation the range must be in > one zone. I do not see anything like that documented for set_migratetype_isolate.
On Wed, Dec 19, 2018 at 10:57:19AM +0100, Oscar Salvador wrote: >On Wed, Dec 19, 2018 at 10:51:10AM +0100, Michal Hocko wrote: >> On Wed 19-12-18 04:46:56, Wei Yang wrote: >> > Below is a brief call flow for __offline_pages() and >> > alloc_contig_range(): >> > >> > __offline_pages()/alloc_contig_range() >> > start_isolate_page_range() >> > set_migratetype_isolate() >> > drain_all_pages() >> > drain_all_pages() >> > >> > Current logic is: isolate and drain pcp list for each pageblock and >> > drain pcp list again. This is not necessary and we could just drain pcp >> > list once after isolate this whole range. >> > >> > The reason is start_isolate_page_range() will set the migrate type of >> > a range to MIGRATE_ISOLATE. After doing so, this range will never be >> > allocated from Buddy, neither to a real user nor to pcp list. >> >> But it is important to note that those pages still can be allocated from >> the pcp lists until we do drain_all_pages(). > >I had the same fear, but then I saw that move_freepages_block()->move_freepages() moves >the pages to a new list: > ><-- >list_move(&page->lru, > &zone->free_area[order].free_list[migratetype]); >--> > > >But looking at it again, I see that this is only for BuddyPages, so I guess >that pcp-pages do not really get unlinked, so we could still allocate them. Well, I think you are right. But with this in mind, current code looks buggy. Between has_unmovable_pages() and drain_all_pages(), others still could allocate pages on pcp list, right? This means we thought we have isolated the range, but not. So even we do drain_all_pages(), we still missed some pages in this range. > >Uhmf, I missed that. > >-- >Oscar Salvador >SUSE L3
On Wed, Dec 19, 2018 at 02:40:56PM +0100, Michal Hocko wrote: >On Wed 19-12-18 13:29:34, Wei Yang wrote: >> On Wed, Dec 19, 2018 at 10:51:10AM +0100, Michal Hocko wrote: >> >On Wed 19-12-18 04:46:56, Wei Yang wrote: >> >> Below is a brief call flow for __offline_pages() and >> >> alloc_contig_range(): >> >> >> >> __offline_pages()/alloc_contig_range() >> >> start_isolate_page_range() >> >> set_migratetype_isolate() >> >> drain_all_pages() >> >> drain_all_pages() >> >> >> >> Current logic is: isolate and drain pcp list for each pageblock and >> >> drain pcp list again. This is not necessary and we could just drain pcp >> >> list once after isolate this whole range. >> >> >> >> The reason is start_isolate_page_range() will set the migrate type of >> >> a range to MIGRATE_ISOLATE. After doing so, this range will never be >> >> allocated from Buddy, neither to a real user nor to pcp list. >> > >> >But it is important to note that those pages still can be allocated from >> >the pcp lists until we do drain_all_pages(). >> > >> >One thing that I really do not like about this patch (and I believe I >> >have mentioned that previously) that you rely on callers to do the right >> >thing. The proper fix would be to do the draining in >> >start_isolate_page_range and remove them from callers. Also what does >> >> Well, I don't really understand this meaning previously. >> >> So you prefer set_migratetype_isolate() do the drain instead of the >> caller (__offline_pages) do the drain. Is my understanding correct? > >Either set_migratetype_isolate or start_isolate_page_range. The later >only if this is guaranteed that we cannot intemix zones in the range. > >> >prevent start_isolate_page_range to work on multiple zones? At least >> >contiguous allocator can do that in principle. >> >> As the comment mentioned, in current implementation the range must be in >> one zone. > >I do not see anything like that documented for set_migratetype_isolate. The comment is not on set_migratetype_isolate, but for its two (grandparent) callers: __offline_pages alloc_contig_range >-- >Michal Hocko >SUSE Labs
On Wed 19-12-18 13:56:35, Wei Yang wrote: > On Wed, Dec 19, 2018 at 02:40:56PM +0100, Michal Hocko wrote: > >On Wed 19-12-18 13:29:34, Wei Yang wrote: [...] > >> As the comment mentioned, in current implementation the range must be in > >> one zone. > > > >I do not see anything like that documented for set_migratetype_isolate. > > The comment is not on set_migratetype_isolate, but for its two > (grandparent) callers: > > __offline_pages > alloc_contig_range But those are consumers while the main api here is start_isolate_page_range. What happens if we grow a new user? Go over the same problems? See the difference? Please try to look at these things from a higher level. We really do not want micro optimise on behalf of a sane API. Unless there is a very good reason to do that - e.g. when the performance difference is really huge.
On Wed 19-12-18 13:53:07, Wei Yang wrote: > On Wed, Dec 19, 2018 at 10:57:19AM +0100, Oscar Salvador wrote: > >On Wed, Dec 19, 2018 at 10:51:10AM +0100, Michal Hocko wrote: > >> On Wed 19-12-18 04:46:56, Wei Yang wrote: > >> > Below is a brief call flow for __offline_pages() and > >> > alloc_contig_range(): > >> > > >> > __offline_pages()/alloc_contig_range() > >> > start_isolate_page_range() > >> > set_migratetype_isolate() > >> > drain_all_pages() > >> > drain_all_pages() > >> > > >> > Current logic is: isolate and drain pcp list for each pageblock and > >> > drain pcp list again. This is not necessary and we could just drain pcp > >> > list once after isolate this whole range. > >> > > >> > The reason is start_isolate_page_range() will set the migrate type of > >> > a range to MIGRATE_ISOLATE. After doing so, this range will never be > >> > allocated from Buddy, neither to a real user nor to pcp list. > >> > >> But it is important to note that those pages still can be allocated from > >> the pcp lists until we do drain_all_pages(). > > > >I had the same fear, but then I saw that move_freepages_block()->move_freepages() moves > >the pages to a new list: > > > ><-- > >list_move(&page->lru, > > &zone->free_area[order].free_list[migratetype]); > >--> > > > > > >But looking at it again, I see that this is only for BuddyPages, so I guess > >that pcp-pages do not really get unlinked, so we could still allocate them. > > Well, I think you are right. But with this in mind, current code looks > buggy. > > Between has_unmovable_pages() and drain_all_pages(), others still could > allocate pages on pcp list, right? This means we thought we have > isolated the range, but not. THere is no guarantee in that regards and I believe there is also no demand for such a strong semantic. Or I do not see it at least.
On Wed, Dec 19, 2018 at 03:13:43PM +0100, Michal Hocko wrote: >On Wed 19-12-18 13:53:07, Wei Yang wrote: >> On Wed, Dec 19, 2018 at 10:57:19AM +0100, Oscar Salvador wrote: >> >On Wed, Dec 19, 2018 at 10:51:10AM +0100, Michal Hocko wrote: >> >> On Wed 19-12-18 04:46:56, Wei Yang wrote: >> >> > Below is a brief call flow for __offline_pages() and >> >> > alloc_contig_range(): >> >> > >> >> > __offline_pages()/alloc_contig_range() >> >> > start_isolate_page_range() >> >> > set_migratetype_isolate() >> >> > drain_all_pages() >> >> > drain_all_pages() >> >> > >> >> > Current logic is: isolate and drain pcp list for each pageblock and >> >> > drain pcp list again. This is not necessary and we could just drain pcp >> >> > list once after isolate this whole range. >> >> > >> >> > The reason is start_isolate_page_range() will set the migrate type of >> >> > a range to MIGRATE_ISOLATE. After doing so, this range will never be >> >> > allocated from Buddy, neither to a real user nor to pcp list. >> >> >> >> But it is important to note that those pages still can be allocated from >> >> the pcp lists until we do drain_all_pages(). >> > >> >I had the same fear, but then I saw that move_freepages_block()->move_freepages() moves >> >the pages to a new list: >> > >> ><-- >> >list_move(&page->lru, >> > &zone->free_area[order].free_list[migratetype]); >> >--> >> > >> > >> >But looking at it again, I see that this is only for BuddyPages, so I guess >> >that pcp-pages do not really get unlinked, so we could still allocate them. >> >> Well, I think you are right. But with this in mind, current code looks >> buggy. >> >> Between has_unmovable_pages() and drain_all_pages(), others still could >> allocate pages on pcp list, right? This means we thought we have >> isolated the range, but not. > >THere is no guarantee in that regards and I believe there is also no >demand for such a strong semantic. Or I do not see it at least. If there is no demand for this, allocating page before drain_all_pages() is reasonable. The time gap between isolating page and drain_all_pages is fine. Then I am confused about the objection to this patch. Finally, we drain all the pages in pcp list and the range is isolated. >-- >Michal Hocko >SUSE Labs
On Wed 19-12-18 14:33:27, Wei Yang wrote: [...] > Then I am confused about the objection to this patch. Finally, we drain > all the pages in pcp list and the range is isolated. Please read my emails more carefully. As I've said, the only reason to do care about draining is to remove it from where it doesn't belong.
On Wed, Dec 19, 2018 at 03:12:35PM +0100, Michal Hocko wrote: >On Wed 19-12-18 13:56:35, Wei Yang wrote: >> On Wed, Dec 19, 2018 at 02:40:56PM +0100, Michal Hocko wrote: >> >On Wed 19-12-18 13:29:34, Wei Yang wrote: >[...] >> >> As the comment mentioned, in current implementation the range must be in >> >> one zone. >> > >> >I do not see anything like that documented for set_migratetype_isolate. >> >> The comment is not on set_migratetype_isolate, but for its two >> (grandparent) callers: >> >> __offline_pages >> alloc_contig_range > >But those are consumers while the main api here is >start_isolate_page_range. What happens if we grow a new user? >Go over the same problems? See the difference? I didn't intend to fight for my patch, just want to clarify current implementation :-) > >Please try to look at these things from a higher level. We really do not >want micro optimise on behalf of a sane API. Unless there is a very good >reason to do that - e.g. when the performance difference is really huge. Well, actually I get your idea and agree with you not rely on the caller to drain the page is the proper way to handle this. Again, I just want to clarify current situation and try to find a proper way to make it better. Maybe I lost some point, while I am willing get feedback and suggestions from all of you. >-- >Michal Hocko >SUSE Labs
On Wed, Dec 19, 2018 at 03:39:27PM +0100, Michal Hocko wrote: >On Wed 19-12-18 14:33:27, Wei Yang wrote: >[...] >> Then I am confused about the objection to this patch. Finally, we drain >> all the pages in pcp list and the range is isolated. > >Please read my emails more carefully. As I've said, the only reason to >do care about draining is to remove it from where it doesn't belong. I go through the thread again and classify two main opinions from you and Oscar. 1) We can still allocate pages in a specific range from pcp list even we have already isolate this range. 2) We shouldn't rely on caller to drain pages and set_migratetype_isolate() may handle a range cross zones. I understand the second one and agree it is not proper to rely on caller and make the assumption on range for set_migratetype_isolate(). My confusion comes from the first one. As you and Oscar both mentioned this and Oscar said "I had the same fear", this makes me think current implementation is buggy. But your following reply said this is not. This means current approach works fine. If the above understanding is correct, and combining with previous discussion, the improvement we can do is to remove the drain_all_pages() in __offline_pages()/alloc_contig_range(). By doing so, the pcp list drain doesn't rely on caller and the isolation/drain on each pageblock ensures pcp list will not contain any page in this range now and future. This imply the drain_all_pages() in __offline_pages()/alloc_contig_range() is not necessary. Is my understanding correct? > >-- >Michal Hocko >SUSE Labs
On Thu 20-12-18 15:58:03, Wei Yang wrote: > On Wed, Dec 19, 2018 at 03:39:27PM +0100, Michal Hocko wrote: > >On Wed 19-12-18 14:33:27, Wei Yang wrote: > >[...] > >> Then I am confused about the objection to this patch. Finally, we drain > >> all the pages in pcp list and the range is isolated. > > > >Please read my emails more carefully. As I've said, the only reason to > >do care about draining is to remove it from where it doesn't belong. > > I go through the thread again and classify two main opinions from you > and Oscar. > > 1) We can still allocate pages in a specific range from pcp list even we > have already isolate this range. > 2) We shouldn't rely on caller to drain pages and > set_migratetype_isolate() may handle a range cross zones. > > I understand the second one and agree it is not proper to rely on caller > and make the assumption on range for set_migratetype_isolate(). > > My confusion comes from the first one. As you and Oscar both mentioned > this and Oscar said "I had the same fear", this makes me think current > implementation is buggy. But your following reply said this is not. This > means current approach works fine. > > If the above understanding is correct, and combining with previous > discussion, the improvement we can do is to remove the drain_all_pages() > in __offline_pages()/alloc_contig_range(). By doing so, the pcp list > drain doesn't rely on caller and the isolation/drain on each pageblock > ensures pcp list will not contain any page in this range now and future. > This imply the drain_all_pages() in > __offline_pages()/alloc_contig_range() is not necessary. > > Is my understanding correct? Yes
On Thu, Dec 20, 2018 at 05:23:02PM +0100, Michal Hocko wrote: >On Thu 20-12-18 15:58:03, Wei Yang wrote: >> On Wed, Dec 19, 2018 at 03:39:27PM +0100, Michal Hocko wrote: >> >On Wed 19-12-18 14:33:27, Wei Yang wrote: >> >[...] >> >> Then I am confused about the objection to this patch. Finally, we drain >> >> all the pages in pcp list and the range is isolated. >> > >> >Please read my emails more carefully. As I've said, the only reason to >> >do care about draining is to remove it from where it doesn't belong. >> >> I go through the thread again and classify two main opinions from you >> and Oscar. >> >> 1) We can still allocate pages in a specific range from pcp list even we >> have already isolate this range. >> 2) We shouldn't rely on caller to drain pages and >> set_migratetype_isolate() may handle a range cross zones. >> >> I understand the second one and agree it is not proper to rely on caller >> and make the assumption on range for set_migratetype_isolate(). >> >> My confusion comes from the first one. As you and Oscar both mentioned >> this and Oscar said "I had the same fear", this makes me think current >> implementation is buggy. But your following reply said this is not. This >> means current approach works fine. >> >> If the above understanding is correct, and combining with previous >> discussion, the improvement we can do is to remove the drain_all_pages() >> in __offline_pages()/alloc_contig_range(). By doing so, the pcp list >> drain doesn't rely on caller and the isolation/drain on each pageblock >> ensures pcp list will not contain any page in this range now and future. >> This imply the drain_all_pages() in >> __offline_pages()/alloc_contig_range() is not necessary. >> >> Is my understanding correct? > >Yes Thanks for your clarification:-) I would come up with a patch to remove this one. > >-- >Michal Hocko >SUSE Labs
diff --git a/mm/page_isolation.c b/mm/page_isolation.c index 43e085608846..f44c0e333bed 100644 --- a/mm/page_isolation.c +++ b/mm/page_isolation.c @@ -83,8 +83,6 @@ static int set_migratetype_isolate(struct page *page, int migratetype, } spin_unlock_irqrestore(&zone->lock, flags); - if (!ret) - drain_all_pages(zone); return ret; }
Below is a brief call flow for __offline_pages() and alloc_contig_range(): __offline_pages()/alloc_contig_range() start_isolate_page_range() set_migratetype_isolate() drain_all_pages() drain_all_pages() Current logic is: isolate and drain pcp list for each pageblock and drain pcp list again. This is not necessary and we could just drain pcp list once after isolate this whole range. The reason is start_isolate_page_range() will set the migrate type of a range to MIGRATE_ISOLATE. After doing so, this range will never be allocated from Buddy, neither to a real user nor to pcp list. Since drain_all_pages() is zone based, by reduce times of drain_all_pages() also reduce some contention on this particular zone. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- v2: adjust changelog with MIGRATE_ISOLATE effects for the isolated range --- mm/page_isolation.c | 2 -- 1 file changed, 2 deletions(-)