On Tue, Sep 8, 2020 at 4:41 PM Hugh Dickins <hu...@google.com> wrote: >
<snip> > [PATCH v18 28/32] mm/compaction: Drop locked from isolate_migratepages_block > Most of this consists of replacing "locked" by "lruvec", which is good: > but please fold those changes back into 20/32 (or would it be 17/32? > I've not yet looked into the relationship between those two), so we > can then see more clearly what change this 28/32 (will need renaming!) > actually makes, to use lruvec_holds_page_lru_lock(). That may be a > good change, but it's mixed up with the "locked"->"lruvec" at present, > and I think you could have just used lruvec for locked all along > (but of course there's a place where you'll need new_lruvec too). I am good with my patch being folded in. No need to keep it separate. > [PATCH v18 29/32] mm: Identify compound pages sooner in > isolate_migratepages_block > NAK. I agree that isolate_migratepages_block() looks nicer this way, but > take a look at prep_new_page() in mm/page_alloc.c: post_alloc_hook() is > where set_page_refcounted() changes page->_refcount from 0 to 1, allowing > a racing get_page_unless_zero() to succeed; then later prep_compound_page() > is where PageHead and PageTails get set. So there's a small race window in > which this patch could deliver a compound page when it should not. So the main motivation for the patch was to avoid the case where we are having to reset the LRU flag. One question I would have is what if we swapped the code block with the __isolate_lru_page_prepare section? WIth that we would be taking a reference on the page, then verifying the LRU flag is set, and then testing for compound page flag bit. Would doing that close the race window since the LRU flag being set should indicate that the allocation has already been completed has it not? > [PATCH v18 30/32] mm: Drop use of test_and_set_skip in favor of just setting > skip > I haven't looked at this yet (but recall that per-memcg lru_lock can > change the point at which compaction should skip a contended lock: IIRC > the current kernel needs nothing extra, whereas some earlier kernels did > need extra; but when I look at 30/32, may find these remarks irrelevant). > > [PATCH v18 31/32] mm: Add explicit page decrement in exception path for > isolate_lru_pages > The title of this patch is definitely wrong: there was an explicit page > decrement there before (put_page), now it's wrapping it up inside a > WARN_ON(). We usually prefer to avoid doing functional operations > inside WARN/BUGs, but I think I'll overlook that - anyone else worried? > The comment is certainly better than what was there before: yes, this > warning reflects the difficulty we have in thinking about the > TestClearPageLRU protocol: which I'm still not sold on, but > agree we should proceed with. With a change in title, perhaps > "mm: add warning where TestClearPageLRU failed on freeable page"? > Acked-by: Hugh Dickins <hu...@google.com> I can update that and resubmit it if needed. I know there were also some suggestions from Matthew.
