Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932115AbdHWOUG (ORCPT ); Wed, 23 Aug 2017 10:20:06 -0400 Received: from out1-smtp.messagingengine.com ([66.111.4.25]:48751 "EHLO out1-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754067AbdHWOUF (ORCPT ); Wed, 23 Aug 2017 10:20:05 -0400 X-ME-Sender: X-Sasl-enc: bmG8OZVtEIi8yPuSvP30AJ3tMZjbc69ln2T0Xc4PEz+K 1503498004 From: "Zi Yan" To: "Naoya Horiguchi" Cc: "Greg Kroah-Hartman" , "Kirill A . Shutemov" , "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" Subject: Re: [RFC PATCH 1/4] mm: madvise: read loop's step size beforehand in madvise_inject_error(), prepare for THP support. Date: Wed, 23 Aug 2017 10:20:02 -0400 Message-ID: <41F3B393-6FDA-4CF9-A790-A1B4B4FDFA58@sent.com> In-Reply-To: <20170823074933.GA3527@hori1.linux.bs1.fc.nec.co.jp> References: <20170815015216.31827-1-zi.yan@sent.com> <20170815015216.31827-2-zi.yan@sent.com> <20170823074933.GA3527@hori1.linux.bs1.fc.nec.co.jp> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=_MailMate_DC67ABC2-6491-42B3-A507-F5B10F8E0871_="; micalg=pgp-sha512; protocol="application/pgp-signature" X-Mailer: MailMate (2.0BETAr6090) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4692 Lines: 135 This is an OpenPGP/MIME signed message (RFC 3156 and 4880). --=_MailMate_DC67ABC2-6491-42B3-A507-F5B10F8E0871_= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable On 23 Aug 2017, at 3:49, Naoya Horiguchi wrote: > On Mon, Aug 14, 2017 at 09:52:13PM -0400, Zi Yan wrote: >> From: Zi Yan >> >> The loop in madvise_inject_error() reads its step size from a page >> after it is soft-offlined. It works because the page is: >> 1) a hugetlb page: the page size does not change; >> 2) a base page: the page size does not change; >> 3) a THP: soft-offline always splits THPs, thus, it is OK to use >> PAGE_SIZE as step size. >> >> It will be a problem when soft-offline supports THP migrations. >> When a THP is migrated without split during soft-offlining, the THP >> is split after migration, thus, before and after soft-offlining page >> sizes do not match. This causes a THP to be unnecessarily soft-lined, >> at most, 511 times, wasting free space. > > Hi Zi Yan, > > Thank you for the suggestion. > > I think that when madvise(MADV_SOFT_OFFLINE) is called with some range > over more than one 4kB page, the caller clearly intends to call > soft_offline_page() over all 4kB pages within the range in order to > simulate the multiple soft-offline events. Please note that the caller > only knows that specific pages are half-broken, and expect that all suc= h > pages are offlined. So the end result should be same, whether the given= > range is backed by thp or not. > But if the given virtual address is backed by a THP and the THP is soft-o= fflined without splitting (enabled by following patches), the old for-loop will c= ause extra 511 THPs being soft-offlined. For example, the caller wants to offline VPN 0-511, which is backed by a = THP whose address range is PFN 0-511. In the first iteration of the for-loop, get_user_pages_fast(VPN0, ...) will return the THP and soft_offline_page(= ) will offline the THP, replacing it with a new THP, say PFN 512-1023, so VPN 0-511 is backed by = PFN 512-1023. But the original THP will be split after it is freed, thus, for-loop will= not end at this moment, but continues to offline VPN1, which leads to PFN 512-102= 3 being offlined and replaced by another THP, say 1024-1535. This will go on and end up wi= th 511 extra THPs are offlined. That is why we need to this patch to tell whether the THP is offlined as a whole or just its head page is offlined.= Let me know if it is still not clear to you. Or I missed something. >> >> Signed-off-by: Zi Yan >> --- >> mm/madvise.c | 21 ++++++++++++++++++--- >> 1 file changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/mm/madvise.c b/mm/madvise.c >> index 47d8d8a25eae..49f6774db259 100644 >> --- a/mm/madvise.c >> +++ b/mm/madvise.c >> @@ -612,19 +612,22 @@ static long madvise_remove(struct vm_area_struct= *vma, >> static int madvise_inject_error(int behavior, >> unsigned long start, unsigned long end) >> { >> - struct page *page; >> + struct page *page =3D NULL; >> + unsigned long page_size =3D PAGE_SIZE; >> >> if (!capable(CAP_SYS_ADMIN)) >> return -EPERM; >> >> - for (; start < end; start +=3D PAGE_SIZE << >> - compound_order(compound_head(page))) { >> + for (; start < end; start +=3D page_size) { >> int ret; >> >> ret =3D get_user_pages_fast(start, 1, 0, &page); >> if (ret !=3D 1) >> return ret; >> >> + page_size =3D (PAGE_SIZE << compound_order(compound_head(page))) - >> + (PAGE_SIZE * (page - compound_head(page))); >> + > > Assigning a value which is not 4kB or some hugepage size into page_size= > might be confusing because that's not what the name says. You can intro= duce > 'next' virtual address and ALIGN() might be helpful to calculate it. Like: next =3D ALIGN(start, PAGE_SIZE<