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,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 AA741CA9EA0 for ; Fri, 18 Oct 2019 16:07:56 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 41B74222C2 for ; Fri, 18 Oct 2019 16:07:55 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="jP25Ff04" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 41B74222C2 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id D9ED68E0006; Fri, 18 Oct 2019 12:07:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D4DD08E0003; Fri, 18 Oct 2019 12:07:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C64CF8E0006; Fri, 18 Oct 2019 12:07:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0215.hostedemail.com [216.40.44.215]) by kanga.kvack.org (Postfix) with ESMTP id A541D8E0003 for ; Fri, 18 Oct 2019 12:07:54 -0400 (EDT) Received: from smtpin01.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with SMTP id 3D33E181AEF09 for ; Fri, 18 Oct 2019 16:07:54 +0000 (UTC) X-FDA: 76057386468.01.judge04_7f2aae0cf4419 X-HE-Tag: judge04_7f2aae0cf4419 X-Filterd-Recvd-Size: 7044 Received: from mail-io1-f66.google.com (mail-io1-f66.google.com [209.85.166.66]) by imf04.hostedemail.com (Postfix) with ESMTP for ; Fri, 18 Oct 2019 16:07:53 +0000 (UTC) Received: by mail-io1-f66.google.com with SMTP id a1so8077062ioc.6 for ; Fri, 18 Oct 2019 09:07:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=PQ+dHPy/vYBlR37NdrG3+2EmNReq2PndbH+SjmQGjvg=; b=jP25Ff04/asHcPluB5d6AuUA8VCxNSOofzsKWJ38a7jzMHkA2usWYLeXyV30/z8z2g MHacC3IuvHhW/+yPiLfRA9qjf6n8oAyKXnHU6tuQZAE8Q6H45/JTMPnwouTJx7r7LyE3 Vrm3ful1mtlXU4AfqqSeEZX/aNF2X9xOGR1c0feDajCaJ3R2+YPVN6CqYDAueJDcCu9I N+rIVjwm41Kq7tlY3ZQtto9DXn+I4ATgop53cS37bWF+i63LJ9vOlmTIEgFocyPxB5wi w5ZWyRHuevpdPEcl9Rq4q9w/830MJhHLmavHOSGf3dTEFDS/dUFupu50JSgxeN7BHobA SHDg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=PQ+dHPy/vYBlR37NdrG3+2EmNReq2PndbH+SjmQGjvg=; b=KIiBFwo8yYAIQINizg42meD2mYa8iFjBvPByyvpYDnG/l2WaDigWAD4B2lzJ3uMF2h 9g0SJzRw1fddhjsX6slQzUjTGNmq6SvUMsl/lly5Ah457HyDI8SDwtSNybDE9VjBhf0k aTA1T76RC2jkus+OomCgEjWg1c0vkturt0quNs/rLfryrcG/P0sHy9FZCCu6Q5Kabxyb mD/cXa7T07tn4FGH6eWHVXPP/1vUpc+kHyblhOvWoKSoIzSascuXqIfAmfF2XUwESoyG HGva8BRbi9zY0xo3y7cQOmLje94SirLffGEUF0yX0jSuGeGiZ8Rl7FTtWl6UMIWuCXBI BrnA== X-Gm-Message-State: APjAAAWGfZi/n8L8bVdU1d7GJ41QVMT+tCpl0qKb6tWEoexjaC9vJotg 1XhLeqBzULnHp88yNuzTKwZEG3BS+zld2nVQviw= X-Google-Smtp-Source: APXvYqwLXKwG6d3xQnpI8tkb2zd+IaZRhz0KZzJd73crsb4ea4R8ILikT65ia9VnyBLjX05pBboqI0bPH5zYdboZ0/c= X-Received: by 2002:a5d:8b48:: with SMTP id c8mr9091128iot.64.1571414872858; Fri, 18 Oct 2019 09:07:52 -0700 (PDT) MIME-Version: 1.0 References: <1571282386-65076-1-git-send-email-zhongjiang@huawei.com> <60492b3f-c933-b54c-8551-18ebb813d7d1@nvidia.com> <5DA9C560.4010009@huawei.com> In-Reply-To: <5DA9C560.4010009@huawei.com> From: Alexander Duyck Date: Fri, 18 Oct 2019 09:07:41 -0700 Message-ID: Subject: Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero To: zhong jiang Cc: John Hubbard , Andrew Morton , Vlastimil Babka , linux-mm Content-Type: text/plain; charset="UTF-8" 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 Fri, Oct 18, 2019 at 7:00 AM zhong jiang wrote: > > On 2019/10/18 4:42, John Hubbard wrote: > > On 10/16/19 8:19 PM, zhong jiang wrote: > >> With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"' > >> compare with zero. And __get_user_pages_locked will return an long value. > >> > >> The patch use a new local variable to store the return value of > >> __get_user_pages_locked(). Then use it to compare with zero. > > Hi Zhong, > > > > The above are actually more like notes to yourself, and while those are > > good to have, but it's not yet a perfect commit description: it talks > > about how you got here, which is only part of the story. A higher level > > summary is better, and let the code itself cover the details. > > > > Like this: > > > > First line (subject): > > > > mm/gup: allow CMA migration to propagate errors back to caller > > > > Commit description: > > > > check_and_migrate_cma_pages() was recording the result of > > __get_user_pages_locked() in an unsigned "nr_pages" variable. Because > > __get_user_pages_locked() returns a signed value that can include > > negative errno values, this had the effect of hiding errors. > > > > Change check_and_migrate_cma_pages() implementation so that it > > uses a signed variable instead, and propagates the results back > > to the caller just as other gup internal functions do. > > > > This was discovered with the help of unsigned_lesser_than_zero.cocci. > > > >> Suggested-by: Andrew Morton > >> Signed-off-by: zhong jiang > >> --- > >> mm/gup.c | 8 +++++--- > >> 1 file changed, 5 insertions(+), 3 deletions(-) > >> > >> diff --git a/mm/gup.c b/mm/gup.c > >> index 8f236a3..1fe0ceb 100644 > >> --- a/mm/gup.c > >> +++ b/mm/gup.c > >> @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk, > >> bool drain_allow = true; > >> bool migrate_allow = true; > >> LIST_HEAD(cma_page_list); > >> + long ret; > > Ira pointed out that this needs initialization, see below. > > > >> > >> check_again: > >> for (i = 0; i < nr_pages;) { > >> @@ -1504,17 +1505,18 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk, > >> * again migrating any new CMA pages which we failed to isolate > >> * earlier. > >> */ > >> - nr_pages = __get_user_pages_locked(tsk, mm, start, nr_pages, > >> + ret = __get_user_pages_locked(tsk, mm, start, nr_pages, > >> pages, vmas, NULL, > >> gup_flags); > >> > >> - if ((nr_pages > 0) && migrate_allow) { > >> + nr_pages = ret; > > Although technically correct, it makes me feel odd to see the assignment > > done from signed to unsigned, *before* checking for >0. And Ira is hinting > > at the same thing, when he asks if we can return early here. See below... > > > >> + if ((ret > 0) && migrate_allow) { > >> drain_allow = true; > >> goto check_again; > >> } > >> } > >> > >> - return nr_pages; > >> + return ret; > >> } > >> #else > >> static long check_and_migrate_cma_pages(struct task_struct *tsk, > >> > > So, overall, I'd recommend this, instead: > > > > diff --git a/mm/gup.c b/mm/gup.c > > index 23a9f9c9d377..72bc027037fa 100644 > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -1443,6 +1443,7 @@ static long check_and_migrate_cma_pages(struct task_struct *tsk, > > bool drain_allow = true; > > bool migrate_allow = true; > > LIST_HEAD(cma_page_list); > > + long ret = nr_pages; > > > I think the ret should be assigned as zero. we will return ret if cma_page_list is empty. > I miss something? That wasn't the behavior before. Before if the cma_page_list was empty nr_pages would not be modified from what what passed. I believe that is the intended behavior for this since the cma_page_list seems like a workaround to migrate out any CMA pages if present in the longterm mapping. If there are no CMA pages the return result should be nr_pages. Thanks. - Alex