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;
From:   "Huang, Ying" <ying.huang@intel.com>
To:     Doug Anderson <dianders@chromium.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Yu Zhao <yuzhao@google.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [RFC PATCH] migrate_pages: Never block waiting for the page lock
References: <20230413182313.RFC.1.Ia86ccac02a303154a0b8bc60567e7a95d34c96d3@changeid>
        <87v8hz17o9.fsf@yhuang6-desk2.ccr.corp.intel.com>
        <CAD=FV=XPBaGGLJVG9UGoJss6EU5=esqyt=aWsp2nOm2YcVOc8g@mail.gmail.com>
Date:   Mon, 17 Apr 2023 09:14:38 +0800
In-Reply-To: <CAD=FV=XPBaGGLJVG9UGoJss6EU5=esqyt=aWsp2nOm2YcVOc8g@mail.gmail.com>
        (Doug Anderson's message of "Fri, 14 Apr 2023 08:25:49 -0700")
Message-ID: <87ildvwbr5.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Doug Anderson <dianders@chromium.org> writes:

> Hi,
>
> On Thu, Apr 13, 2023 at 8:10=E2=80=AFPM Huang, Ying <ying.huang@intel.com=
> wrote:
>>
>> Douglas Anderson <dianders@chromium.org> writes:
>>
>> > Currently when we try to do page migration and we're in "synchronous"
>> > mode (and not doing direct compaction) then we'll wait an infinite
>> > amount of time for a page lock. This does not appear to be a great
>> > idea.
>> >
>> > One issue can be seen when I put a device under extreme memory
>> > pressure. I took a sc7180-trogdor Chromebook (4GB RAM, 8GB zram
>> > swap). I ran the browser along with Android (which runs from a
>> > loopback mounted 128K block-size squashfs "disk"). I then manually ran
>> > the mmm_donut memory pressure tool [1]. The system is completely
>> > unusable both with and without this patch since there are 8 processes
>> > completely thrashing memory, but it was still interesting to look at
>> > how migration was behaving. I put some timing code in and I could see
>> > that we sometimes waited over 25 seconds (in the context of
>> > kcompactd0) for a page lock to become available. Although the 25
>> > seconds was the high mark, it was easy to see tens, hundreds, or
>> > thousands of milliseconds spent waiting on the lock.
>> >
>> > Instead of waiting, if I bailed out right away (as this patch does), I
>> > could see kcompactd0 move forward to successfully to migrate other
>> > pages instead. This seems like a better use of kcompactd's time.
>> >
>> > Thus, even though this didn't make the system any more usable in my
>> > absurd test case, it still seemed to make migration behave better and
>> > that feels like a win. It also makes the code simpler since we have
>> > one fewer special case.
>>
>> TBH, the test case is too extreme for me.
>
> That's fair. That being said, I guess the point I was trying to make
> is that waiting for this lock could take an unbounded amount of time.
> Other parts of the system sometimes hold a page lock and then do a
> blocking operation. At least in the case of kcompactd there are better
> uses of its time than waiting for any given page.
>
>> And, we have multiple "sync" mode to deal with latency requirement, for
>> example, we use MIGRATE_SYNC_LIGHT for compaction to avoid too long
>> latency.  If you have latency requirement for some users, you may
>> consider to add new "sync" mode.
>
> Sure. kcompactd_do_work() is currently using MIGRATE_SYNC_LIGHT. I
> guess my first thought would be to avoid adding a new mode and make
> MIGRATE_SYNC_LIGHT not block here. Then anyone that truly needs to
> wait for all the pages to be migrated can use the heavier sync modes.
> It seems to me like the current users of MIGRATE_SYNC_LIGHT would not
> want to block for an unbounded amount of time here. What do you think?

It appears that you can just use MIGRATE_ASYNC if you think the correct
behavior is "NOT block at all".  I found that there are more
fine-grained controls on this in compaction code, please take a look at
"enum compact_priority" and its comments.

Best Regards,
Huang, Ying