Received: by 2002:a05:6a10:f3d0:0:0:0:0 with SMTP id a16csp258844pxv; Wed, 30 Jun 2021 05:07:53 -0700 (PDT) X-Google-Smtp-Source: ABdhPJx24UYjAA6wk6h3uWOgd5nGhBgyiVdi3EbjHqm+vtzJwkyqtePYI7WJMsihw2vGmXyMNztf X-Received: by 2002:a6b:7702:: with SMTP id n2mr7612582iom.1.1625054873297; Wed, 30 Jun 2021 05:07:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1625054873; cv=none; d=google.com; s=arc-20160816; b=edqkR191D6K6rSPh+CzmgCtKEKbfqIcwd7fTUwWRhoknVKifK6vtjs3artVyrDnyPY pstwvUTjNiAKOwVd2HbMH64zrgn7e0cMk86zY1wxDFpMNHkmQkAeVDSQtjXEmE6bgV+x dw4xK5u56UcISGeye9ajrvnEwj+HriQkc4+us9jupSNNuC33Y162uE5ZLpL2S3rJWvxW aWy8reY+y3NjMWP2JlxwYI+5mJHnNNikRDP2o0UBcEdfg3Pp8L6iX5H+ewfOI2q0q7+x Aj7o/7EvtPNZmlYk5/CugHARPxH3XzwJOXBl7sZ/hvauQj0x/6sSExQsTEd+0MINw7S4 vMUA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:user-agent:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=EbDFL+G5FCe2apAbQJ0MVoVD382S35TzLWoFylkFZjo=; b=IGB3JzJXsBs+H08sTrX3j1TMo98wQ8WzL52JRo1OVyu+sU9prmMACwvQ30c2m5wXpr z2JKFGPxmwcCcyi3YtzjR3jp0z3Tr0zoug6ApY+s7xI8CuhzvjFG4RtW/YgB4DXXBeI1 UMZ06+/6Vy7U9HFZZldByzfG1o091tFfvy+3lL9EVzdlVcdpwLo1fUWbCcW49W9QeFwB B5pdC1A0TRgEtu9uiK1eFySS48yaUm/nmG6nCTnAtB1eq2TgjThU94xN9wJ7FwEzAT7a UWtuAogUlXCG43eMhYUAAJlh1mbAAEiBvhrFeWeEKSRd/9CDswSbdLz7Dhx2HQwFLD9l QHUA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-nfs-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id i9si22441158ioq.17.2021.06.30.05.07.26; Wed, 30 Jun 2021 05:07:53 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-nfs-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234426AbhF3MI3 (ORCPT + 99 others); Wed, 30 Jun 2021 08:08:29 -0400 Received: from outbound-smtp14.blacknight.com ([46.22.139.231]:45151 "EHLO outbound-smtp14.blacknight.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234349AbhF3MI3 (ORCPT ); Wed, 30 Jun 2021 08:08:29 -0400 Received: from mail.blacknight.com (pemlinmail03.blacknight.ie [81.17.254.16]) by outbound-smtp14.blacknight.com (Postfix) with ESMTPS id 6A5E51C43BC for ; Wed, 30 Jun 2021 13:05:59 +0100 (IST) Received: (qmail 24999 invoked from network); 30 Jun 2021 12:05:59 -0000 Received: from unknown (HELO techsingularity.net) (mgorman@techsingularity.net@[84.203.17.255]) by 81.17.254.9 with ESMTPSA (AES256-SHA encrypted, authenticated); 30 Jun 2021 12:05:59 -0000 Date: Wed, 30 Jun 2021 13:05:58 +0100 From: Mel Gorman To: Jesper Dangaard Brouer Cc: Chuck Lever , linux-nfs@vger.kernel.org, linux-mm@kvack.org, brouer@redhat.com Subject: Re: [PATCH v3] mm/page_alloc: Further fix __alloc_pages_bulk() return value Message-ID: <20210630120557.GK3840@techsingularity.net> References: <162497449506.16614.7781489905877008435.stgit@klimt.1015granger.net> <20210630065855.GH3840@techsingularity.net> <543ef038-d50c-f035-bd9d-ae3140ca0e33@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <543ef038-d50c-f035-bd9d-ae3140ca0e33@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org On Wed, Jun 30, 2021 at 01:22:24PM +0200, Jesper Dangaard Brouer wrote: > > On 30/06/2021 08.58, Mel Gorman wrote: > > On Tue, Jun 29, 2021 at 06:01:12PM +0200, Jesper Dangaard Brouer wrote: > > > On 29/06/2021 15.48, Chuck Lever wrote: > > > > > > > The call site in __page_pool_alloc_pages_slow() also seems to be > > > > confused on this matter. It should be attended to by someone who > > > > is familiar with that code. > > > I don't think we need a fix for __page_pool_alloc_pages_slow(), as the array > > > is guaranteed to be empty. > > > > > > But a fix would look like this: > > > > > > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > > > index c137ce308c27..1b04538a3da3 100644 > > > --- a/net/core/page_pool.c > > > +++ b/net/core/page_pool.c > > > @@ -245,22 +245,23 @@ static struct page > > > *__page_pool_alloc_pages_slow(struct page_pool *pool, > > > ??????? if (unlikely(pp_order)) > > > ??????????????? return __page_pool_alloc_page_order(pool, gfp); > > > > > > ??????? /* Unnecessary as alloc cache is empty, but guarantees zero count */ > > > -?????? if (unlikely(pool->alloc.count > 0)) > > > +?????? if (unlikely(pool->alloc.count > 0))?? // ^^^^^^^^^^^^^^^^^^^^^^ > > > ??????????????? return pool->alloc.cache[--pool->alloc.count]; > > > > > > ??????? /* Mark empty alloc.cache slots "empty" for alloc_pages_bulk_array > > > */ > > > ??????? memset(&pool->alloc.cache, 0, sizeof(void *) * bulk); > > > > > > +?????? /* bulk API ret value also count existing pages, but array is empty > > > */ > > > ??????? nr_pages = alloc_pages_bulk_array(gfp, bulk, pool->alloc.cache); > > > ??????? if (unlikely(!nr_pages)) > > > ??????????????? return NULL; > > > > > > ??????? /* Pages have been filled into alloc.cache array, but count is zero > > > and > > > ???????? * page element have not been (possibly) DMA mapped. > > > ???????? */ > > > -?????? for (i = 0; i < nr_pages; i++) { > > > +?????? for (i = pool->alloc.count; i < nr_pages; i++) { > > That last part would break as the loop is updating pool->alloc_count. > > The last part "i = pool->alloc.count" probably is a bad idea. It is because it confuses the context, either alloc.count is guaranteed zero or it's not. Initialised as zero, it's fairly clear that it's a) always zero and b) the way i and alloc.count works mean that the array element pointing to a freed page is either ignored or overwritten by a valid page pointer in a later loop iteration. -- Mel Gorman SUSE Labs