lkml.org 
[lkml]   [2020]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v21 14/19] mm/lru: introduce TestClearPageLRU
    From
    Date
    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
    >

    \
     
     \ /
      Last update: 2020-11-12 12:25    [W:5.576 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site