Re: [RFC] mm/vmscan.c: avoid possible long latency caused by too_many_isolated()

Xing Zhengjun
2021-04-30T05:57:40Z

Hi Yu,

On 4/24/2021 4:23 AM, Yu Zhao wrote:
> On Fri, Apr 23, 2021 at 02:57:07PM +0800, Xing Zhengjun wrote:
>> On 4/23/2021 1:13 AM, Yu Zhao wrote:
>>> On Thu, Apr 22, 2021 at 04:36:19PM +0800, Xing Zhengjun wrote:
>>>> Hi,
>>>>
>>>> In the system with very few file pages (nr_active_file + nr_inactive_file
>>>> < 100), it is easy to reproduce "nr_isolated_file > nr_inactive_file", then
>>>> too_many_isolated return true, shrink_inactive_list enter "msleep(100)", the
>>>> long latency will happen.
>>>>
>>>> The test case to reproduce it is very simple: allocate many huge pages(near
>>>> the DRAM size), then do free, repeat the same operation many times.
>>>> In the test case, the system with very few file pages (nr_active_file +
>>>> nr_inactive_file < 100), I have dumpped the numbers of
>>>> active/inactive/isolated file pages during the whole test(see in the
>>>> attachments) , in shrink_inactive_list "too_many_isolated" is very easy to
>>>> return true, then enter "msleep(100)",in "too_many_isolated" sc->gfp_mask is
>>>> 0x342cca ("_GFP_IO" and "__GFP_FS" is masked) , it is also very easy to
>>>> enter “inactive >>=3”, then “isolated > inactive” will be true.
>>>>
>>>> So I have a proposal to set a threshold number for the total file pages to
>>>> ignore the system with very few file pages, and then bypass the 100ms sleep.
>>>> It is hard to set a perfect number for the threshold, so I just give an
>>>> example of "256" for it.
>>>>
>>>> I appreciate it if you can give me your suggestion/comments. Thanks.
>>>
>>> Hi Zhengjun,
>>>
>>> It seems to me using the number of isolated pages to keep a lid on
>>> direct reclaimers is not a good solution. We shouldn't keep going
>>> that direction if we really want to fix the problem because migration
>>> can isolate many pages too, which in turn blocks page reclaim.
>>>
>>> Here is something works a lot better. Please give it a try. Thanks.
>>
>> Thanks, I will try it with my test cases.
>
> Thanks. I took care my sloppiness from yesterday and tested the
> following. It should apply cleanly and work well. Please let me know.
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 47946cec7584..48bb2b77389e 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -832,6 +832,7 @@ typedef struct pglist_data {
> #endif
>
> /* Fields commonly accessed by the page reclaim scanner */
> + atomic_t nr_reclaimers;
>
> /*
> * NOTE: THIS IS UNUSED IF MEMCG IS ENABLED.
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 562e87cbd7a1..3fcdfbee89c7 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1775,43 +1775,6 @@ int isolate_lru_page(struct page *page)
> return ret;
> }
>
> -/*
> - * A direct reclaimer may isolate SWAP_CLUSTER_MAX pages from the LRU list and
> - * then get rescheduled. When there are massive number of tasks doing page
> - * allocation, such sleeping direct reclaimers may keep piling up on each CPU,
> - * the LRU list will go small and be scanned faster than necessary, leading to
> - * unnecessary swapping, thrashing and OOM.
> - */
> -static int too_many_isolated(struct pglist_data *pgdat, int file,
> - struct scan_control *sc)
> -{
> - unsigned long inactive, isolated;
> -
> - if (current_is_kswapd())
> - return 0;
> -
> - if (!writeback_throttling_sane(sc))
> - return 0;
> -
> - if (file) {
> - inactive = node_page_state(pgdat, NR_INACTIVE_FILE);
> - isolated = node_page_state(pgdat, NR_ISOLATED_FILE);
> - } else {
> - inactive = node_page_state(pgdat, NR_INACTIVE_ANON);
> - isolated = node_page_state(pgdat, NR_ISOLATED_ANON);
> - }
> -
> - /*
> - * GFP_NOIO/GFP_NOFS callers are allowed to isolate more pages, so they
> - * won't get blocked by normal direct-reclaimers, forming a circular
> - * deadlock.
> - */
> - if ((sc->gfp_mask & (__GFP_IO | __GFP_FS)) == (__GFP_IO | __GFP_FS))
> - inactive >>= 3;
> -
> - return isolated > inactive;
> -}
> -
> /*
> * move_pages_to_lru() moves pages from private @list to appropriate LRU list.
> * On return, @list is reused as a list of pages to be freed by the caller.
> @@ -1911,20 +1874,6 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
> bool file = is_file_lru(lru);
> enum vm_event_item item;
> struct pglist_data *pgdat = lruvec_pgdat(lruvec);
> - bool stalled = false;
> -
> - while (unlikely(too_many_isolated(pgdat, file, sc))) {
> - if (stalled)
> - return 0;
> -
> - /* wait a bit for the reclaimer. */
> - msleep(100);
> - stalled = true;
> -
> - /* We are about to die and free our memory. Return now. */
> - if (fatal_signal_pending(current))
> - return SWAP_CLUSTER_MAX;
> - }
>
> lru_add_drain();
>
> @@ -2903,6 +2852,8 @@ static void shrink_zones(struct zonelist *zonelist, struct scan_control *sc)
> unsigned long nr_soft_scanned;
> gfp_t orig_mask;
> pg_data_t *last_pgdat = NULL;
> + bool should_retry = false;
> + int nr_cpus = num_online_cpus();
>
> /*
> * If the number of buffer_heads in the machine exceeds the maximum
> @@ -2914,9 +2865,18 @@ static void shrink_zones(struct zonelist *zonelist, struct scan_control *sc)
> sc->gfp_mask |= __GFP_HIGHMEM;
> sc->reclaim_idx = gfp_zone(sc->gfp_mask);
> }
> -
> +retry:
> for_each_zone_zonelist_nodemask(zone, z, zonelist,
> sc->reclaim_idx, sc->nodemask) {
> + /*
> + * Shrink each node in the zonelist once. If the zonelist is
> + * ordered by zone (not the default) then a node may be shrunk
> + * multiple times but in that case the user prefers lower zones
> + * being preserved.
> + */
> + if (zone->zone_pgdat == last_pgdat)
> + continue;
> +
> /*
> * Take care memory controller reclaiming has small influence
> * to global LRU.
> @@ -2941,16 +2901,28 @@ static void shrink_zones(struct zonelist *zonelist, struct scan_control *sc)
> sc->compaction_ready = true;
> continue;
> }
> + }
>
> - /*
> - * Shrink each node in the zonelist once. If the
> - * zonelist is ordered by zone (not the default) then a
> - * node may be shrunk multiple times but in that case
> - * the user prefers lower zones being preserved.
> - */
> - if (zone->zone_pgdat == last_pgdat)
> - continue;
> + /*
> + * A direct reclaimer may isolate SWAP_CLUSTER_MAX pages from
> + * the LRU list and then get rescheduled. When there are massive
> + * number of tasks doing page allocation, such sleeping direct
> + * reclaimers may keep piling up on each CPU, the LRU list will
> + * go small and be scanned faster than necessary, leading to
> + * unnecessary swapping, thrashing and OOM.
> + */
> + VM_BUG_ON(current_is_kswapd());
>
> + if (!atomic_add_unless(&zone->zone_pgdat->nr_reclaimers, 1, nr_cpus)) {
> + should_retry = true;
> + continue;
> + }
> +
> + if (last_pgdat)
> + atomic_dec(&last_pgdat->nr_reclaimers);
> + last_pgdat = zone->zone_pgdat;
> +
> + if (!cgroup_reclaim(sc)) {
> /*
> * This steals pages from memory cgroups over softlimit
> * and returns the number of reclaimed pages and
> @@ -2966,13 +2938,20 @@ static void shrink_zones(struct zonelist *zonelist, struct scan_control *sc)
> /* need some check for avoid more shrink_zone() */
> }
>
> - /* See comment about same check for global reclaim above */
> - if (zone->zone_pgdat == last_pgdat)
> - continue;
> - last_pgdat = zone->zone_pgdat;
> shrink_node(zone->zone_pgdat, sc);
> }
>
> + if (last_pgdat)
> + atomic_dec(&last_pgdat->nr_reclaimers);
> + else if (should_retry) {
> + /* wait a bit for the reclaimer. */
> + if (!schedule_timeout_killable(HZ / 10))
> + goto retry;
> +
> + /* We are about to die and free our memory. Return now. */
> + sc->nr_reclaimed += SWAP_CLUSTER_MAX;
> + }
> +
> /*
> * Restore to original mask to avoid the impact on the caller if we
> * promoted it to __GFP_HIGHMEM.
> @@ -4189,6 +4168,15 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
> set_task_reclaim_state(p, &sc.reclaim_state);
>
> if (node_pagecache_reclaimable(pgdat) > pgdat->min_unmapped_pages) {
> + int nr_cpus = num_online_cpus();
> +
> + VM_BUG_ON(current_is_kswapd());
> +
> + if (!atomic_add_unless(&pgdat->nr_reclaimers, 1, nr_cpus)) {
> + schedule_timeout_killable(HZ / 10);
> + goto out;
> + }
> +
> /*
> * Free memory by calling shrink node with increasing
> * priorities until we have enough memory freed.
> @@ -4196,8 +4184,10 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
> do {
> shrink_node(pgdat, &sc);
> } while (sc.nr_reclaimed < nr_pages && --sc.priority >= 0);
> - }
>
> + atomic_dec(&pgdat->nr_reclaimers);
> + }
> +out:
> set_task_reclaim_state(p, NULL);
> current->flags &= ~PF_SWAPWRITE;
> memalloc_noreclaim_restore(noreclaim_flag);
>

I use my compaction test case to test it, test more than 30 times can
not reproduce the 100ms sleep. I find that applies the patch, direct
reclaim path latency reduces much, but the direct compact path latency
double compares with it before.

24) | __alloc_pages_direct_compact() {
24) | try_to_compact_pages() {
24) 0.131 us | __next_zones_zonelist();
24) @ 184008.2 us | compact_zone_order();
24) 0.189 us | __next_zones_zonelist();
24) 0.547 us | compact_zone_order();
24) 0.225 us | __next_zones_zonelist();
24) 0.592 us | compact_zone_order();
24) 0.146 us | __next_zones_zonelist();
24) @ 184012.3 us | }
24) | get_page_from_freelist() {
24) 0.160 us | __zone_watermark_ok();
24) 0.140 us | __next_zones_zonelist();
24) 0.141 us | __zone_watermark_ok();
24) 0.134 us | __next_zones_zonelist();
24) 0.121 us | __zone_watermark_ok();
24) 0.123 us | __next_zones_zonelist();
24) 1.688 us | }
24) 0.130 us | ___might_sleep();
24) | __cond_resched() {
24) 0.123 us | rcu_all_qs();
24) 0.370 us | }
24) @ 184015.2 us | }
24) | /* mm_page_alloc: page=0000000000000000 pfn=0
order=9 migratetype=1
gfp_flags=GFP_HIGHUSER_MOVABLE|__GFP_NOWARN|__GFP_COMP|__GFP_THISNODE */
24) | /* memlatency: lat=184716 order=9
gfp_flags=342cca
(GFP_HIGHUSER_MOVABLE|__GFP_NOWARN|__GFP_COMP|__GFP_THISNODE|0x812a3c6000000000^@)migratetype=1
*/
/*The memlatency measures the latency of "__alloc_pages_nodemask" */

--
Zhengjun Xing