Re: [PATCH] mm/gup: don't check page lru flag before draining it

Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> · Thu, 6 Jun 2024 09:57:47 +0800

On 2024/6/5 20:20, David Hildenbrand wrote:
On 05.06.24 13:41, David Hildenbrand wrote:
On 05.06.24 13:37, Baolin Wang wrote:


On 2024/6/5 17:53, David Hildenbrand wrote:
On 05.06.24 11:41, David Hildenbrand wrote:
On 05.06.24 03:18, yangge1116 wrote:


在 2024/6/4 下午9:47, David Hildenbrand 写道:
On 04.06.24 12:48, yangge1116@xxxxxxx wrote:
From: yangge <yangge1116@xxxxxxx>

If a page is added in pagevec, its ref count increases one, remove
the page from pagevec decreases one. Page migration requires the
page is not referenced by others except page mapping. Before
migrating a page, we should try to drain the page from pagevec in
case the page is in it, however, folio_test_lru() is not sufficient
to tell whether the page is in pagevec or not, if the page is in
pagevec, the migration will fail.

Remove the condition and drain lru once to ensure the page is not
referenced by pagevec.

What you are saying is that we might have a page on which
folio_test_lru() succeeds, that was added to one of the 
cpu_fbatches,
correct?

Yes


Can you describe under which circumstances that happens?


If we call folio_activate() to move a page from inactive LRU list to
active LRU list, the page is not only in LRU list, but also in one of
the cpu_fbatches.

void folio_activate(struct folio *folio)
{
         if (folio_test_lru(folio) && !folio_test_active(folio) &&
             !folio_test_unevictable(folio)) {
             struct folio_batch *fbatch;

             folio_get(folio);
             //After this, folio is in LRU list, and its ref count 
have
increased one.

             local_lock(&cpu_fbatches.lock);
             fbatch = this_cpu_ptr(&cpu_fbatches.activate);
             folio_batch_add_and_move(fbatch, folio, 
folio_activate_fn);
             local_unlock(&cpu_fbatches.lock);
         }
}

Interesting, the !SMP variant does the folio_test_clear_lru().

It would be really helpful if we could reliably identify whether LRU
batching code has a raised reference on a folio.

We have the same scenario in
* folio_deactivate()
* folio_mark_lazyfree()

In folio_batch_move_lru() we do the folio_test_clear_lru(folio).

No expert on that code, I'm wondering if we could move the
folio_test_clear_lru() out, such that we can more reliably identify
whether a folio is on the LRU batch or not.

I'm sure there would be something extremely broken with the following
(I don't know what I'm doing ;) ), but I wonder if there would be a way
to make something like that work (and perform well enough?).

diff --git a/mm/swap.c b/mm/swap.c
index 67786cb771305..642e471c3ec5a 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -212,10 +212,6 @@ static void folio_batch_move_lru(struct 
folio_batch
*fbatch, move_fn_t move_fn)
           for (i = 0; i < folio_batch_count(fbatch); i++) {
                   struct folio *folio = fbatch->folios[i];

-               /* block memcg migration while the folio moves between
lru */
-               if (move_fn != lru_add_fn && 
!folio_test_clear_lru(folio))
-                       continue;
-
                   folio_lruvec_relock_irqsave(folio, &lruvec, &flags);
                   move_fn(lruvec, folio);

@@ -255,8 +251,9 @@ static void lru_move_tail_fn(struct lruvec *lruvec,
struct folio *folio)
     */
    void folio_rotate_reclaimable(struct folio *folio)
    {
-       if (!folio_test_locked(folio) && !folio_test_dirty(folio) &&
-           !folio_test_unevictable(folio) && folio_test_lru(folio)) {
+       if (folio_test_lru(folio) && !folio_test_locked(folio) &&
+           !folio_test_dirty(folio) && 
!folio_test_unevictable(folio) &&
+           folio_test_clear_lru(folio)) {
                   struct folio_batch *fbatch;
                   unsigned long flags;

@@ -354,7 +351,7 @@ static void folio_activate_drain(int cpu)
    void folio_activate(struct folio *folio)
    {
           if (folio_test_lru(folio) && !folio_test_active(folio) &&
-           !folio_test_unevictable(folio)) {
+           !folio_test_unevictable(folio) &&
folio_test_clear_lru(folio)) {

IMO, this seems violate the semantics of the LRU flag, since it's clear
that this folio is still in the LRU list.

Good point.

But regarding "violation": we already do clear the flag temporarily in
there, so it's rather that we make the visible time where it is cleared
"longer". (yes, it can be much longer :) )

Some random thoughts about some folio_test_lru() users:

mm/khugepaged.c: skips pages if !folio_test_lru(), but would fail skip 
it either way if there is the unexpected reference from the LRU batch!

mm/compaction.c: skips pages if !folio_test_lru(), but would fail skip 
it either way if there is the unexpected reference from the LRU batch!

mm/memory.c: would love to identify this case and to a lru_add_drain() 
to free up that reference.

mm/huge_memory.c: splitting with the additional reference will fail 
already. Maybe we'd want to drain the LRU batch.

Agree.


mm/madvise.c: skips pages if !folio_test_lru(). I wonder what happens if 
we have the same page twice in an LRU batch with different target goals ...

IIUC, LRU batch can ignore this folio since it's LRU flag is cleared by 
folio_isolate_lru(), then will call folios_put() to frop the reference.


Some other users (there are not that many that don't use it for sanity 
checks though) might likely be a bit different.

mm/page_isolation.c: fail to set pageblock migratetype to isolate if 
!folio_test_lru(), then alloc_contig_range_noprof() can be failed. But 
the original code could set pageblock migratetype to isolate, then 
calling drain_all_pages() in alloc_contig_range_noprof() to drop 
reference of the LRU batch.

mm/vmscan.c: will call lru_add_drain() before calling 
isolate_lru_folios(), so seems no impact.

BTW, we also need to look at the usage of folio_isolate_lru().

It doesn’t seem to have major obstacles, but there are many details to 
analyze :)







