Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 19 Jul 2023 07:26:08 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Yin Fengwei <fengwei.yin@intel.com>
cc:     Yosry Ahmed <yosryahmed@google.com>, Yu Zhao <yuzhao@google.com>,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org, willy@infradead.org, david@redhat.com,
        ryan.roberts@arm.com, shy828301@gmail.com,
        Hugh Dickins <hughd@google.com>
Subject: Re: [RFC PATCH v2 3/3] mm: mlock: update mlock_pte_range to handle
 large folio
In-Reply-To: <e3044d46-3b38-dc2e-b8d2-8ec1033f85e7@intel.com>
Message-ID: <79f6822-f2f8-aba4-b517-b661d07e2d@google.com>
References: <20230712060144.3006358-1-fengwei.yin@intel.com> <20230712060144.3006358-4-fengwei.yin@intel.com> <CAOUHufYef--8MxFettL6fOGjVx2vyZHZQU6EEaTCoW0XBvuC8Q@mail.gmail.com> <CAOUHufZ6=9P_=CAOQyw0xw-3q707q-1FVV09dBNDC-hpcpj2Pg@mail.gmail.com>
 <40cbc39e-5179-c2f4-3cea-0a98395aaff1@intel.com> <CAOUHufZHyEvU-c2O6B6stM_QVMxc22zV4Szn52myYqjdZvptUA@mail.gmail.com> <16844254-7248-f557-b1eb-b8b102c877a2@intel.com> <CAJD7tkYAkVOE2caqEj_hTmm47Kex451prBQ1wKTRUiOwnDcwNA@mail.gmail.com>
 <b995e802-1500-6930-79d0-8cc4bfe89589@intel.com> <CAJD7tkZtHku-kaK02MAdgaxNzr9hQkPty=cw44R_9HdTS+Pd5w@mail.gmail.com> <CAJD7tkZWXdHwpW5AeKqmn6TVCXm1wmKr-2RN2baRJ7c4ciTJng@mail.gmail.com> <208aff10-8a32-6ab8-f03a-7f3c9d3ca0f7@intel.com>
 <CAJD7tkYT6EZMwit8C9MTftUxMmuWtn2YpZ+NSVhy0xVCYuafsg@mail.gmail.com> <438d6f6d-2571-69d9-844e-9af9e6b4f820@intel.com> <CAJD7tkYWH8umBFgmxPmeOkRF=pauVW=MvyyN+z17XMHN+q8JKg@mail.gmail.com> <e3044d46-3b38-dc2e-b8d2-8ec1033f85e7@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 19 Jul 2023, Yin Fengwei wrote:
> >>>>>>>>> Could this also happen against normal 4K page? I mean when user try to munlock
> >>>>>>>>> a normal 4K page and this 4K page is isolated. So it become unevictable page?
> >>>>>>>> Looks like it can be possible. If cpu 1 is in __munlock_folio() and
> >>>>>>>> cpu 2 is isolating the folio for any purpose:
> >>>>>>>>
> >>>>>>>> cpu1                        cpu2
> >>>>>>>>                             isolate folio
> >>>>>>>> folio_test_clear_lru() // 0
> >>>>>>>>                             putback folio // add to unevictable list
> >>>>>>>> folio_test_clear_mlocked()
> >>>>>                                folio_set_lru()
> Let's wait the response from Huge and Yu. :).

I haven't been able to give it enough thought, but I suspect you are right:
that the current __munlock_folio() is deficient when folio_test_clear_lru()
fails.

(Though it has not been reported as a problem in practice: perhaps because
so few places try to isolate from the unevictable "list".)

I forget what my order of development was, but it's likely that I first
wrote the version for our own internal kernel - which used our original
lruvec locking, which did not depend on getting PG_lru first (having got
lru_lock, it checked memcg, then tried again if that had changed).

I was uneasy with the PG_lru aspect of upstream lru_lock implementation,
but it turned out to work okay - elsewhere; but it looks as if I missed
its implication when adapting __munlock_page() for upstream.

If I were trying to fix this __munlock_folio() race myself (sorry, I'm
not), I would first look at that aspect: instead of folio_test_clear_lru()
behaving always like a trylock, could "folio_wait_clear_lru()" or whatever
spin waiting for PG_lru here?

Hugh