On 5/19/22 14:13, Mel Gorman wrote: > On Thu, May 19, 2022 at 12:57:01PM +0200, Vlastimil Babka wrote: >> On 5/12/22 10:50, Mel Gorman wrote: >>> The VM_BUG_ON check for a valid page can be avoided with a simple >>> change in the flow. The ZONE_BOOSTED_WATERMARK is unlikely in general >>> and even more unlikely if the page allocation failed so mark the >>> branch unlikely. >> >> Hm, so that makes a DEBUG_VM config avoid the check. On the other hand, >> it puts it on the path returning from rmqueue_pcplist() for all configs, >> and that should be the fast path. So unless things further change in the >> following patches, it doesn't seem that useful? >> > > You're right -- the fast path ends up with both a if > (page) and if (!page) checks. Andrew, can you drop the patch > mm-page_alloc-remove-unnecessary-page-==-null-check-in-rmqueue.patch from > your tree please? > > Originally the flow was important when I was writing the patch and later > became unnecessary. However, it reminded me of another problem I thought > of when writing this and then forgotten to note it in the changelog. If > the page allocation fails then ZONE_BOOSTED_WATERMARK should still be > tested and cleared before waking kswapd. It could happen if an allocation > attempt tried to fallback to another migratetype and still fail to find > a suitable page. This is true whether going through the PCP lists or not. > > So what do you think of me adding this patch to a follow-up series? LGTM. > > --8<-- > mm/page_alloc: Remove mistaken page == NULL check in rmqueue > > If a page allocation fails, the ZONE_BOOSTER_WATERMARK should be tested, > cleared and kswapd woken whether the allocation attempt was via the PCP > or directly via the buddy list. > > Remove the page == NULL so the ZONE_BOOSTED_WATERMARK bit is checked > unconditionally. As it is unlikely that ZONE_BOOSTED_WATERMARK is set, > mark the branch accordingly. > > Signed-off-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx> Acked-by: Vlastimil Babka <vbabka@xxxxxxx> > --- > mm/page_alloc.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 1c4c54503a5d..61d5bc2efffe 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3765,12 +3765,10 @@ struct page *rmqueue(struct zone *preferred_zone, > > page = rmqueue_buddy(preferred_zone, zone, order, alloc_flags, > migratetype); > - if (unlikely(!page)) > - return NULL; > > out: > /* Separate test+clear to avoid unnecessary atomics */ > - if (test_bit(ZONE_BOOSTED_WATERMARK, &zone->flags)) { > + if (unlikely(test_bit(ZONE_BOOSTED_WATERMARK, &zone->flags))) { > clear_bit(ZONE_BOOSTED_WATERMARK, &zone->flags); > wakeup_kswapd(zone, 0, 0, zone_idx(zone)); > } >