On 3/14/22 00:25, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@xxxxxxxxxx>
>
> check_pcp_refill() is used from rmqueue_bulk() while zone spinlock
> is held.
>
> This used to be fine because check_pcp_refill() was testing only the
> head page, while its 'struct page' was very hot in the cpu caches.
>
> With ("mm/page_alloc: check high-order pages for corruption during PCP
> operations") check_pcp_refill() will add latencies for high order pages.
>
> We can defer the calls to check_pcp_refill() after the zone
> spinlock has been released.
>
> Signed-off-by: Eric Dumazet <edumazet@xxxxxxxxxx>
> Cc: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
> Cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> Cc: Shakeel Butt <shakeelb@xxxxxxxxxx>
> Cc: David Rientjes <rientjes@xxxxxxxxxx>
> Cc: Vlastimil Babka <vbabka@xxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxxxx>
> Cc: Wei Xu <weixugc@xxxxxxxxxx>
> Cc: Greg Thelen <gthelen@xxxxxxxxxx>
> Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> ---
> mm/page_alloc.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c9ebf0635d592c6f58df9793ce9fa213371a9a7f..5f0531c11ad668b1c4426ebddc17821aca824783 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3024,7 +3024,9 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
> unsigned long count, struct list_head *list,
> int migratetype, unsigned int alloc_flags)
> {
> + struct page *page, *tmp;
> int i, allocated = 0;
> + int free_cma_pages = 0;
>
> /*
> * local_lock_irq held so equivalent to spin_lock_irqsave for
Right so this should be AFAIU enough for
__mod_zone_page_state(NR_FREE_CMA_PAGES) at the end to be OK outside of
zone->lock.
> @@ -3032,14 +3034,10 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
> */
> spin_lock(&zone->lock);
> for (i = 0; i < count; ++i) {
> - struct page *page = __rmqueue(zone, order, migratetype,
> - alloc_flags);
> + page = __rmqueue(zone, order, migratetype, alloc_flags);
> if (unlikely(page == NULL))
> break;
>
> - if (unlikely(check_pcp_refill(page)))
> - continue;
> -
> /*
> * Split buddy pages returned by expand() are received here in
> * physical page order. The page is added to the tail of
> @@ -3052,9 +3050,6 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
> */
> list_add_tail(&page->lru, list);
> allocated++;
> - if (is_migrate_cma(get_pcppage_migratetype(page)))
> - __mod_zone_page_state(zone, NR_FREE_CMA_PAGES,
> - -(1 << order));
> }
>
> /*
> @@ -3065,6 +3060,16 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
> */
> __mod_zone_page_state(zone, NR_FREE_PAGES, -(i << order));
> spin_unlock(&zone->lock);
You could maybe even exchange those two lines as well?
> + list_for_each_entry_safe(page, tmp, list, lru) {
> + if (unlikely(check_pcp_refill(page))) {
> + list_del(&page->lru);
> + allocated--;
> + } else if (is_migrate_cma(get_pcppage_migratetype(page))) {
> + free_cma_pages++;
I think in line with the (IMHO correct) decreasing NR_FREE_CMA_PAGES
regardless of the check result, we should also count free_cma_pages here
unconditionally before doing check_pcp_refill().
> + }
> + }
> + if (free_cma_pages)
> + __mod_zone_page_state(zone, NR_FREE_CMA_PAGES, -(free_cma_pages << order));
> return allocated;
> }
>
