From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E6041C3F68F for ; Mon, 20 Jan 2020 14:12:00 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 96E9F22522 for ; Mon, 20 Jan 2020 14:12:00 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lca.pw header.i=@lca.pw header.b="Fyx8XxKT" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 96E9F22522 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=lca.pw Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 3E4A26B066A; Mon, 20 Jan 2020 09:12:00 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 396936B066B; Mon, 20 Jan 2020 09:12:00 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 284196B066C; Mon, 20 Jan 2020 09:12:00 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0221.hostedemail.com [216.40.44.221]) by kanga.kvack.org (Postfix) with ESMTP id 1360C6B066A for ; Mon, 20 Jan 2020 09:12:00 -0500 (EST) Received: from smtpin14.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with SMTP id B45C6181AEF00 for ; Mon, 20 Jan 2020 14:11:59 +0000 (UTC) X-FDA: 76398201558.14.steam26_4b8d34924ba2d X-HE-Tag: steam26_4b8d34924ba2d X-Filterd-Recvd-Size: 8952 Received: from mail-qt1-f195.google.com (mail-qt1-f195.google.com [209.85.160.195]) by imf07.hostedemail.com (Postfix) with ESMTP for ; Mon, 20 Jan 2020 14:11:59 +0000 (UTC) Received: by mail-qt1-f195.google.com with SMTP id 5so27749705qtz.1 for ; Mon, 20 Jan 2020 06:11:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=lca.pw; s=google; h=mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=jGThDTnuhXaKd8DUWOBQzCaaD/3GsSO5W0D8Fuquqn0=; b=Fyx8XxKTcB983BEZFuMKhi0v0oJEKtHTRfaSigJ+fwp7PGmQ++wUXsjZdf+nCKqCcn dHWFBaYViWbhwgjjxbHD1UaRCcHU9ibSPb94wiCAwpUkYdb2fkJdyrvI9lDZxEP5cvLE EF3Y5aGybIxQ2K5xY166/G1u97pWIgPUiocb7YdosKnGsaZSiVEXzislnebp+lTv0UxW SCNhTHI+wbc4d1THfcQcZ/JM4pmgVoTdDmBg8i7jwQNWsp82G9lEKyIer7ww9sz7nI57 YAyhiTRIWo7TYPl4Z22Ced0DxxvCHHn/jjgqm+TKXDIRtJZSReOasohKg1oRvIzUijtZ UqQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=jGThDTnuhXaKd8DUWOBQzCaaD/3GsSO5W0D8Fuquqn0=; b=Vzuv/TBU5eFu2mk6hIIdwaKo8WpELH0KMP94KXy7SDUHSZLqgvwRaeKDm6fXhZLx5s EmU1FCZ7+ScoRPzFavbKUSegyf66Ks64gnu8UKpt1Uyw9uYRXpuMuyUf2UouAEda0IWQ EEIJXubaebL+22Z5sImsmbGK/Vxi3RbjeDJbf0W1WEu9dThTNcbLui7UiVAt3P4AEqm3 inN5YbPOmdCCE4ZR/Ase557zYkFlOpoxr4TIWervkmXo33iBFO4gqvY+znq0wHBz5sOI AZ42Qx87hVEio7Ujz00wjhQznA6KZ2kKxHn5FGt4P48y82VpMDTfVFYmz4xGNp+OzOti 82ag== X-Gm-Message-State: APjAAAU/0nRsUn9VmgkDsRpBfN8PF7XQbaSnTF1djUG6ak/yAWc5jlNG zJIH2Uq+gcayGgyfatSRw8B0OQ== X-Google-Smtp-Source: APXvYqwEfQwhex2/D/3RnAvsEIatvig33icdQFY0W8PcUteW/YXKF+TjYilF7ScjCcpAed2cmkBrqw== X-Received: by 2002:ac8:4306:: with SMTP id z6mr20382763qtm.178.1579529518484; Mon, 20 Jan 2020 06:11:58 -0800 (PST) Received: from [192.168.1.153] (pool-71-184-117-43.bstnma.fios.verizon.net. [71.184.117.43]) by smtp.gmail.com with ESMTPSA id t15sm15639623qkg.49.2020.01.20.06.11.57 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 20 Jan 2020 06:11:57 -0800 (PST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.40.2.2.4\)) Subject: Re: [PATCH -mm v2] mm/page_isolation: fix potential warning from user From: Qian Cai In-Reply-To: <74aebdfe-e727-acd6-e664-6e63948a68ae@redhat.com> Date: Mon, 20 Jan 2020 09:11:55 -0500 Cc: Andrew Morton , Michal Hocko , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <20200120131909.813-1-cai@lca.pw> <8c56268d-9b8a-f62e-eca9-7707852a2aaf@redhat.com> <96675869-3815-4E98-8492-1D84F5B42AED@lca.pw> <74aebdfe-e727-acd6-e664-6e63948a68ae@redhat.com> To: David Hildenbrand X-Mailer: Apple Mail (2.3608.40.2.2.4) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: > On Jan 20, 2020, at 9:01 AM, David Hildenbrand = wrote: >=20 > On 20.01.20 14:56, Qian Cai wrote: >>=20 >>=20 >>> On Jan 20, 2020, at 8:38 AM, David Hildenbrand = wrote: >>>=20 >>> On 20.01.20 14:30, David Hildenbrand wrote: >>>> On 20.01.20 14:19, Qian Cai wrote: >>>>> It makes sense to call the WARN_ON_ONCE(zone_idx(zone) =3D=3D = ZONE_MOVABLE) >>>>> from start_isolate_page_range(), but should avoid triggering it = from >>>>> userspace, i.e, from is_mem_section_removable() because it could = be a >>>>> DoS if warn_on_panic is set. >>>>>=20 >>>>> While at it, simplify the code a bit by removing an unnecessary = jump >>>>> label and a local variable, so set_migratetype_isolate() could = really >>>>> return a bool. >>>>>=20 >>>>> Suggested-by: Michal Hocko >>>>> Signed-off-by: Qian Cai >>>>> --- >>>>>=20 >>>>> v2: Improve the commit log. >>>>> Warn for all start_isolate_page_range() users not just = offlining. >>>>>=20 >>>>> mm/page_alloc.c | 11 ++++------- >>>>> mm/page_isolation.c | 30 +++++++++++++++++------------- >>>>> 2 files changed, 21 insertions(+), 20 deletions(-) >>>>>=20 >>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>>>> index 621716a25639..3c4eb750a199 100644 >>>>> --- a/mm/page_alloc.c >>>>> +++ b/mm/page_alloc.c >>>>> @@ -8231,7 +8231,7 @@ struct page *has_unmovable_pages(struct zone = *zone, struct page *page, >>>>> if (is_migrate_cma(migratetype)) >>>>> return NULL; >>>>>=20 >>>>> - goto unmovable; >>>>> + return page; >>>>> } >>>>>=20 >>>>> for (; iter < pageblock_nr_pages; iter++) { >>>>> @@ -8241,7 +8241,7 @@ struct page *has_unmovable_pages(struct zone = *zone, struct page *page, >>>>> page =3D pfn_to_page(pfn + iter); >>>>>=20 >>>>> if (PageReserved(page)) >>>>> - goto unmovable; >>>>> + return page; >>>>>=20 >>>>> /* >>>>> * If the zone is movable and we have ruled out all = reserved >>>>> @@ -8261,7 +8261,7 @@ struct page *has_unmovable_pages(struct zone = *zone, struct page *page, >>>>> unsigned int skip_pages; >>>>>=20 >>>>> if = (!hugepage_migration_supported(page_hstate(head))) >>>>> - goto unmovable; >>>>> + return page; >>>>>=20 >>>>> skip_pages =3D compound_nr(head) - (page - = head); >>>>> iter +=3D skip_pages - 1; >>>>> @@ -8303,12 +8303,9 @@ struct page *has_unmovable_pages(struct = zone *zone, struct page *page, >>>>> * is set to both of a memory hole page and a _used_ = kernel >>>>> * page at boot. >>>>> */ >>>>> - goto unmovable; >>>>> + return page; >>>>> } >>>>> return NULL; >>>>> -unmovable: >>>>> - WARN_ON_ONCE(zone_idx(zone) =3D=3D ZONE_MOVABLE); >>>>> - return pfn_to_page(pfn + iter); >>>>> } >>>>>=20 >>>>> #ifdef CONFIG_CONTIG_ALLOC >>>>> diff --git a/mm/page_isolation.c b/mm/page_isolation.c >>>>> index e70586523ca3..31f5516f5d54 100644 >>>>> --- a/mm/page_isolation.c >>>>> +++ b/mm/page_isolation.c >>>>> @@ -15,12 +15,12 @@ >>>>> #define CREATE_TRACE_POINTS >>>>> #include >>>>>=20 >>>>> -static int set_migratetype_isolate(struct page *page, int = migratetype, int isol_flags) >>>>> +static bool set_migratetype_isolate(struct page *page, int = migratetype, >>>>> + int isol_flags) >>>>=20 >>>> Why this change? >>>>=20 >>>>> { >>>>> - struct page *unmovable =3D NULL; >>>>> + struct page *unmovable =3D ERR_PTR(-EBUSY); >>>>=20 >>>> Also, why this change? >>>>=20 >>>>> struct zone *zone; >>>>> unsigned long flags; >>>>> - int ret =3D -EBUSY; >>>>>=20 >>>>> zone =3D page_zone(page); >>>>>=20 >>>>> @@ -49,21 +49,25 @@ static int set_migratetype_isolate(struct page = *page, int migratetype, int isol_ >>>>> = NULL); >>>>>=20 >>>>> __mod_zone_freepage_state(zone, -nr_pages, mt); >>>>> - ret =3D 0; >>>>> } >>>>>=20 >>>>> out: >>>>> spin_unlock_irqrestore(&zone->lock, flags); >>>>> - if (!ret) >>>>> + >>>>> + if (!unmovable) { >>>>> drain_all_pages(zone); >>>>> - else if ((isol_flags & REPORT_FAILURE) && unmovable) >>>>> - /* >>>>> - * printk() with zone->lock held will guarantee to = trigger a >>>>> - * lockdep splat, so defer it here. >>>>> - */ >>>>> - dump_page(unmovable, "unmovable page"); >>>>> - >>>>> - return ret; >>>>> + } else { >>>>> + WARN_ON_ONCE(zone_idx(zone) =3D=3D ZONE_MOVABLE); >>>>> + >>>>> + if ((isol_flags & REPORT_FAILURE) && !IS_ERR(unmovable)) >>>>> + /* >>>>=20 >>>> Why this change? (!IS_ERR) >>>>=20 >>>>=20 >>>> Some things here look unrelated - or I am missing something :) >>>>=20 >>>=20 >>> FWIW, I'd prefer this change without any such cleanups (e.g., I = don't >>> like returning a bool from this function and the IS_ERR handling, = makes >>> the function harder to read than before) >>=20 >> What is Michal or Andrew=E2=80=99s opinion? BTW, a bonus point to = return a bool >> is that it helps the code robustness in general, as UBSAN will be = able to >> catch any abuse. >>=20 >=20 > A return type of bool on a function that does not test a property > ("has_...", "is"...") is IMHO confusing. That is fine. It could be renamed to set_migratetype_is_isolate() or is_set_migratetype_isolate() which seems pretty minor because we have no consistency in the naming of this in linux kernel at all, i.e., many existing bool function names without those test of properties.=20 >=20 > If we have an int, it is clear that "0" means "success". With a bool > (true/false), it is not clear. >=20 > --=20 > Thanks, >=20 > David / dhildenb >=20