On 2024/11/17 11:26, Barry Song wrote:
> On Sat, Nov 16, 2024 at 10:26 PM Chen Ridong <chenridong@xxxxxxxxxxxxxxx> wrote:
>>
>> From: Chen Ridong <chenridong@xxxxxxxxxx>
>>
>> An issue was found with the following testing step:
>> 1. Compile with CONFIG_TRANSPARENT_HUGEPAGE=y
>> 2. Mount memcg v1, and create memcg named test_memcg and set
>> usage_in_bytes=2.1G, memsw.usage_in_bytes=3G.
>> 3. Create a 1G swap file, and allocate 2.2G anon memory in test_memcg.
>>
>> It was found that:
>>
>> cat memory.usage_in_bytes
>> 2144940032
>> cat memory.memsw.usage_in_bytes
>> 2255056896
>>
>> free -h
>> total used free
>> Mem: 31Gi 2.1Gi 27Gi
>> Swap: 1.0Gi 618Mi 405Mi
>>
>> As shown above, the test_memcg used about 100M swap, but 600M+ swap memory
>> was used, which means that 500M may be wasted because other memcgs can not
>> use these swap memory.
>>
>> It can be explained as follows:
>> 1. When entering shrink_inactive_list, it isolates folios from lru from
>> tail to head. If it just takes folioN from lru(make it simple).
>>
>> inactive lru: folio1<->folio2<->folio3...<->folioN-1
>> isolated list: folioN
>>
>> 2. In shrink_page_list function, if folioN is THP(2M), it may be splited
>> and added to swap cache folio by folio. After adding to swap cache,
>> it will submit io to writeback folio to swap, which is asynchronous.
>> When shrink_page_list is finished, the isolated folios list will be
>> moved back to the head of inactive lru. The inactive lru may just look
>> like this, with 512 filioes have been move to the head of inactive lru.
>>
>> folioN512<->folioN511<->...filioN1<->folio1<->folio2...<->folioN-1
>>
>> It committed io from folioN1 to folioN512, the later folios committed
>> was added to head of the 'ret_folios' in the shrink_page_list function.
>> As a result, the order was shown as folioN512->folioN511->...->folioN1.
>>
>> 3. When folio writeback io is completed, the folio may be rotated to tail
>> of the lru one by one. It's assumed that filioN1,filioN2, ...,filioN512
>> are completed in order(commit io in this order), and they are rotated to
>> the tail of the LRU in order (filioN1<->...folioN511<->folioN512).
>> Therefore, those folios that are tail of the lru will be reclaimed as
>> soon as possible.
>>
>> folio1<->folio2<->...<->folioN-1<->filioN1<->...folioN511<->folioN512
>>
>> 4. However, shrink_page_list and folio writeback are asynchronous. If THP
>> is splited, shrink_page_list loops at least 512 times, which means that
>> shrink_page_list is not completed but some folios writeback have been
>> completed, and this may lead to failure to rotate these folios to the
>> tail of lru. The lru may look likes as below:
>>
>> folioN50<->folioN49<->...filioN1<->folio1<->folio2...<->folioN-1<->
>> folioN51<->folioN52<->...folioN511<->folioN512
>>
>> Although those folios (N1-N50) have been finished writing back, they
>> are still at the head of the lru. This is because their writeback_end
>> occurred while it were still looping in shrink_folio_list(), causing
>> folio_end_writeback()'s folio_rotate_reclaimable() to fail in moving
>> these folios, which are not in the LRU but still in the 'folio_list',
>> to the tail of the LRU.
>> When isolating folios from lru, it scans from tail to head, so it is
>> difficult to scan those folios again.
>>
>> What mentioned above may lead to a large number of folios have been added
>> to swap cache but can not be reclaimed in time, which may reduce reclaim
>> efficiency and prevent other memcgs from using this swap memory even if
>> they trigger OOM.
>>
>> To fix this issue, the folios whose writeback has been completed should be
>> move to the tail of the LRU instead of always placing them at the head of
>> the LRU when the shrink_page_list is finished. It can be realized by
>> following steps.
>> 1. In the shrink_page_list function, the folios whose are committed to
>
> It seems like there's a grammatical error here—whose something？
>
>> are added to the head of 'folio_list', which will be return to the
>> caller.
>> 2. When shrink_page_list finishes, it is known that how many folios have
>> been pageout, and they are all at the head of 'folio_list', which is
>> ready be moved back to LRU. So, in the 'move_folios_to_lru function'
>> function, if the first 'nr_io' folios (which have been pageout) have
>> been written back completely, move them to the tail of LRU. Otherwise,
>> move them to the head of the LRU.
>>
>> Signed-off-by: Chen Ridong <chenridong@xxxxxxxxxx>
>> ---
>> mm/vmscan.c | 37 +++++++++++++++++++++++++++++--------
>> 1 file changed, 29 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 76378bc257e3..04f7eab9d818 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1046,6 +1046,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>> struct folio_batch free_folios;
>> LIST_HEAD(ret_folios);
>> LIST_HEAD(demote_folios);
>> + LIST_HEAD(pageout_folios);
>> unsigned int nr_reclaimed = 0;
>> unsigned int pgactivate = 0;
>> bool do_demote_pass;
>> @@ -1061,7 +1062,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>> struct address_space *mapping;
>> struct folio *folio;
>> enum folio_references references = FOLIOREF_RECLAIM;
>> - bool dirty, writeback;
>> + bool dirty, writeback, is_pageout = false;
>> unsigned int nr_pages;
>>
>> cond_resched();
>> @@ -1384,6 +1385,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>> nr_pages = 1;
>> }
>> stat->nr_pageout += nr_pages;
>> + is_pageout = true;
>>
>> if (folio_test_writeback(folio))
>> goto keep;
>> @@ -1508,7 +1510,10 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>> keep_locked:
>> folio_unlock(folio);
>> keep:
>> - list_add(&folio->lru, &ret_folios);
>> + if (is_pageout)
>> + list_add(&folio->lru, &pageout_folios);
>> + else
>> + list_add(&folio->lru, &ret_folios);
>> VM_BUG_ON_FOLIO(folio_test_lru(folio) ||
>> folio_test_unevictable(folio), folio);
>> }
>> @@ -1551,6 +1556,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>> free_unref_folios(&free_folios);
>>
>> list_splice(&ret_folios, folio_list);
>> + list_splice(&pageout_folios, folio_list);
>
> Do we really need this pageout_folios list? is the below not sufficient?
>
> + if (nr_io > 0 &&
> + !folio_test_reclaim(folio) &&
> + !folio_test_writeback(folio))
> + lruvec_add_folio_tail(lruvec, folio);
> + else
> + lruvec_add_folio(lruvec, folio);
>
Thank you for your reply.
I think this is indeed. We want move the written-back folios to the tail
of the LRU so that them can be scanned as soon as possible to be reclaimed.
How can we know the folios have been paged out and written-back in the
move_folios_to_lru function?
It was set to 'folio_set_reclaim(folio)' when the folios were paged-out,
and it was set to 'folio_test_set_writeback(folio)' before committing
io. The 'pageout_folios' can inform the caller those folios have been
paged-out and '!folio_test_reclaim(folio) &&
!folio_test_writeback(folio)' can determine whether those folios have
been written-back.
If we don't add 'pageout_folios' here, we can only use
'!folio_test_reclaim(folio) && !folio_test_writeback(folio)' to
determine move the folios to the tail or not. This could result in
mistakenly moving those folios—which have not been paged out, but for
which the 'PG_writeback' and 'PG_reclaim' flags are not set—to the tail
of the LRU. This may lead to repeatedly scanning the same folios.
>> count_vm_events(PGACTIVATE, pgactivate);
>>
>> if (plug)
>> @@ -1826,11 +1832,14 @@ static bool too_many_isolated(struct pglist_data *pgdat, int file,
>>
>> /*
>> * move_folios_to_lru() moves folios from private @list to appropriate LRU list.
>> + * @lruvec: The LRU vector the list is moved to.
>> + * @list: The folio list are moved to lruvec
>> + * @nr_io: The first nr folios of the list that have been committed io.
>> *
>> * Returns the number of pages moved to the given lruvec.
>> */
>> static unsigned int move_folios_to_lru(struct lruvec *lruvec,
>> - struct list_head *list)
>> + struct list_head *list, unsigned int nr_io)
>> {
>> int nr_pages, nr_moved = 0;
>> struct folio_batch free_folios;
>> @@ -1880,9 +1889,21 @@ static unsigned int move_folios_to_lru(struct lruvec *lruvec,
>> * inhibits memcg migration).
>> */
>> VM_BUG_ON_FOLIO(!folio_matches_lruvec(folio, lruvec), folio);
>> - lruvec_add_folio(lruvec, folio);
>> + /*
>> + * If the folio have been committed io and writed back completely,
>> + * it should be added to the tailed to the lru, so it can
>> + * be relaimed as soon as possible.
>> + */
>> + if (nr_io > 0 &&
>> + !folio_test_reclaim(folio) &&
>> + !folio_test_writeback(folio))
>> + lruvec_add_folio_tail(lruvec, folio);
>> + else
>> + lruvec_add_folio(lruvec, folio);
>> +
>> nr_pages = folio_nr_pages(folio);
>> nr_moved += nr_pages;
>> + nr_io = nr_io > nr_pages ? (nr_io - nr_pages) : 0;
>> if (folio_test_active(folio))
>> workingset_age_nonresident(lruvec, nr_pages);
>> }
>> @@ -1960,7 +1981,7 @@ static unsigned long shrink_inactive_list(unsigned long nr_to_scan,
>> nr_reclaimed = shrink_folio_list(&folio_list, pgdat, sc, &stat, false);
>>
>> spin_lock_irq(&lruvec->lru_lock);
>> - move_folios_to_lru(lruvec, &folio_list);
>> + move_folios_to_lru(lruvec, &folio_list, stat.nr_pageout);
>>
>> __mod_lruvec_state(lruvec, PGDEMOTE_KSWAPD + reclaimer_offset(),
>> stat.nr_demoted);
>> @@ -2111,8 +2132,8 @@ static void shrink_active_list(unsigned long nr_to_scan,
>> */
>> spin_lock_irq(&lruvec->lru_lock);
>>
>> - nr_activate = move_folios_to_lru(lruvec, &l_active);
>> - nr_deactivate = move_folios_to_lru(lruvec, &l_inactive);
>> + nr_activate = move_folios_to_lru(lruvec, &l_active, 0);
>> + nr_deactivate = move_folios_to_lru(lruvec, &l_inactive, 0);
>>
>> __count_vm_events(PGDEACTIVATE, nr_deactivate);
>> __count_memcg_events(lruvec_memcg(lruvec), PGDEACTIVATE, nr_deactivate);
>> @@ -4627,7 +4648,7 @@ static int evict_folios(struct lruvec *lruvec, struct scan_control *sc, int swap
>>
>> spin_lock_irq(&lruvec->lru_lock);
>>
>> - move_folios_to_lru(lruvec, &list);
>> + move_folios_to_lru(lruvec, &list, 0);
>
> I'm not entirely convinced about using the 'nr' argument here.
> Is the goal to differentiate between two cases?
> 1. we need to take care of written-back folios
> 2. we don't need to take care of written-back folios?
>
The 'isolate_lru_folios' scans the LRU from the tail to the head. If the
folios have been scanned for shrinking, they are moved to the head of
the LRU to prevent repeated scanning. Only folios that have been paged
out and written back should be moved to the tail of the LRU, so they can
be reclaimed the next time they are scanned.
However, if these folios have been paged out but not yet written back,
they should be moved to the head of the LRU. They will then be moved to
the tail of the LRU 'when folio_end_writeback()'s
folio_rotate_reclaimable()' is invoked.
For those folios that have not been paged out, if they cannot be
reclaimed, they should be moved to the head of the LRU to avoid repeated
scanning. That is why we need 'nr_io', which indicates that these folios
have been paged out."
> Would it be a bool or better to provide separate helpers?
>
+ if (nr_io > 0 &&
+ !folio_test_writeback(folio))
+ lruvec_add_folio_tail(lruvec, folio);
+ else
+ lruvec_add_folio(lruvec, folio);
+
I think this may be sufficient. Only using 'PG_writeback' to determine
whether the folios have been written-back.
Thank you again.
Best regards,
Ridong
>>
>> walk = current->reclaim_state->mm_walk;
>> if (walk && walk->batched) {
>> --
>> 2.34.1
>>
>
> Thanks
> Barry
