Messages in this thread | | | Subject | Re: [PATCH v21 14/19] mm/lru: introduce TestClearPageLRU | From | Vlastimil Babka <> | Date | Thu, 12 Nov 2020 12:24:17 +0100 |
| |
On 11/12/20 3:03 AM, Hugh Dickins wrote: > On Wed, 11 Nov 2020, Vlastimil Babka wrote: >> On 11/5/20 9:55 AM, Alex Shi wrote: >> >> > --- a/mm/vmscan.c >> > +++ b/mm/vmscan.c >> > @@ -1542,7 +1542,7 @@ unsigned int reclaim_clean_pages_from_list(struct >> > zone *zone, >> > */ >> > int __isolate_lru_page(struct page *page, isolate_mode_t mode) >> > { >> > - int ret = -EINVAL; >> > + int ret = -EBUSY; >> > /* Only take pages on the LRU. */ >> > if (!PageLRU(page)) >> > @@ -1552,8 +1552,6 @@ int __isolate_lru_page(struct page *page, >> > isolate_mode_t mode) >> > if (PageUnevictable(page) && !(mode & ISOLATE_UNEVICTABLE)) >> > return ret; >> > - ret = -EBUSY; >> >> I'm not sure why this change is here, looks unrelated to the patch? >> >> Oh I see, you want to prevent the BUG() in isolate_lru_pages(). > > Yes, I suggested this part of the patch to Alex, when I hit that BUG(). > >> >> But due to that, the PageUnevictable check was also affected unintentionally. >> But I don't think it's that important to BUG() when we run into >> PageUnevictable unexpectedly, so that's probably ok. > > Not unintentional. __isolate_lru_page(), or __isolate_lru_page_prepare(), > is a silly function, used by two callers whose requirements are almost > entirely disjoint. The ISOLATE_UNEVICTABLE case is only for compaction.c, > which takes no interest in -EINVAL versus -EBUSY, and has no such BUG(). > > I think it dates back to lumpy reclaim days, and it probably made more > sense back then.
Ah, thanks for explaining.
>> >> But with that, we can just make __isolate_lru_page() a bool function and >> remove the ugly switch in isolate_lru_pages()? > > I agree that the switch statement in isolate_lru_pages() seems pointless > now, and can be turned into an if{}else{}. But that cleanup is a > diversion from this particular TestClearPageLRU patch, and I think from > the whole series (checking final state of the patchset, yes, the switch > is still there - though I think there have been variant series which > removed it). > > Can we please leave that cleanup until after the series has gone in?
Sure thing!
The patch seems functionally fine, so
Acked-by: Vlastimil Babka <vbabka@suse.cz>
> I think several of us have cleanups or optimization that we want to > follow (I had one that inlines what isolate_migratepages_block() wanted > of __isolate_lru_page() into that function, so simplifying what vmscan.c > needs; perhaps that can now eliminate it completely, I've not tried > recently). But there was a point at which the series was growing > ten patches per release as we all added our bits and pieces on top, > it got harder and harder to review the whole, and further from > getting the basics in: I do push back against that tendency. > > Hugh >
| |