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=-10.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 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 28102C11F68 for ; Wed, 30 Jun 2021 12:06:05 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A6C8A61919 for ; Wed, 30 Jun 2021 12:06:04 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A6C8A61919 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=techsingularity.net Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 0D2F88D017A; Wed, 30 Jun 2021 08:06:04 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0AAC56B00A6; Wed, 30 Jun 2021 08:06:04 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EB4438D017A; Wed, 30 Jun 2021 08:06:03 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0225.hostedemail.com [216.40.44.225]) by kanga.kvack.org (Postfix) with ESMTP id C220C6B00A5 for ; Wed, 30 Jun 2021 08:06:03 -0400 (EDT) Received: from smtpin38.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 97284824C443 for ; Wed, 30 Jun 2021 12:06:03 +0000 (UTC) X-FDA: 78310261806.38.B00E488 Received: from outbound-smtp35.blacknight.com (outbound-smtp35.blacknight.com [46.22.139.218]) by imf18.hostedemail.com (Postfix) with ESMTP id 1E7B34002088 for ; Wed, 30 Jun 2021 12:06:01 +0000 (UTC) Received: from mail.blacknight.com (pemlinmail03.blacknight.ie [81.17.254.16]) by outbound-smtp35.blacknight.com (Postfix) with ESMTPS id 6B3301FE6 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 In-Reply-To: <543ef038-d50c-f035-bd9d-ae3140ca0e33@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) Authentication-Results: imf18.hostedemail.com; dkim=none; spf=pass (imf18.hostedemail.com: domain of mgorman@techsingularity.net designates 46.22.139.218 as permitted sender) smtp.mailfrom=mgorman@techsingularity.net; dmarc=none X-Stat-Signature: 6r9o5o7reykzto5uqmtuhokoduzwmno3 X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 1E7B34002088 X-HE-Tag: 1625054761-35426 Content-Transfer-Encoding: quoted-printable 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 Wed, Jun 30, 2021 at 01:22:24PM +0200, Jesper Dangaard Brouer wrote: >=20 > On 30/06/2021 08.58, Mel Gorman wrote: > > On Tue, Jun 29, 2021 at 06:01:12PM +0200, Jesper Dangaard Brouer wrot= e: > > > On 29/06/2021 15.48, Chuck Lever wrote: > > >=20 > > > > 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. > > >=20 > > > But a fix would look like this: > > >=20 > > > 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, > > > =A0=A0=A0=A0=A0=A0=A0 if (unlikely(pp_order)) > > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return __page_pool_a= lloc_page_order(pool, gfp); > > >=20 > > > =A0=A0=A0=A0=A0=A0=A0 /* Unnecessary as alloc cache is empty, but = guarantees zero count */ > > > -=A0=A0=A0=A0=A0=A0 if (unlikely(pool->alloc.count > 0)) > > > +=A0=A0=A0=A0=A0=A0 if (unlikely(pool->alloc.count > 0))=A0=A0 // ^= ^^^^^^^^^^^^^^^^^^^^^ > > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return pool->alloc.c= ache[--pool->alloc.count]; > > >=20 > > > =A0=A0=A0=A0=A0=A0=A0 /* Mark empty alloc.cache slots "empty" for = alloc_pages_bulk_array > > > */ > > > =A0=A0=A0=A0=A0=A0=A0 memset(&pool->alloc.cache, 0, sizeof(void *)= * bulk); > > >=20 > > > +=A0=A0=A0=A0=A0=A0 /* bulk API ret value also count existing pages= , but array is empty > > > */ > > > =A0=A0=A0=A0=A0=A0=A0 nr_pages =3D alloc_pages_bulk_array(gfp, bul= k, pool->alloc.cache); > > > =A0=A0=A0=A0=A0=A0=A0 if (unlikely(!nr_pages)) > > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return NULL; > > >=20 > > > =A0=A0=A0=A0=A0=A0=A0 /* Pages have been filled into alloc.cache a= rray, but count is zero > > > and > > > =A0=A0=A0=A0=A0=A0=A0=A0 * page element have not been (possibly) D= MA mapped. > > > =A0=A0=A0=A0=A0=A0=A0=A0 */ > > > -=A0=A0=A0=A0=A0=A0 for (i =3D 0; i < nr_pages; i++) { > > > +=A0=A0=A0=A0=A0=A0 for (i =3D pool->alloc.count; i < nr_pages; i++= ) { > > That last part would break as the loop is updating pool->alloc_count. > > The last part "i =3D 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. --=20 Mel Gorman SUSE Labs