From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yw1-f176.google.com (mail-yw1-f176.google.com [209.85.128.176]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0A0B9F9C0 for ; Sun, 23 Nov 2025 23:04:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.176 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763939058; cv=none; b=Gg1dACSfWirmMvWpn60MBR9A1Zjh6NBei2cV224d+z2NVwMBzqtJ8zbB3+dZfyJW0uHcmOPviCxDHQ8rfXeqrGSHDwWT2gTxLSXY9zS9h/2znFBSzUb5aX/+aVZfNnacLhObzGiTAOoYQBNf7DrfFqNtG/s/c9c4hRXoNOog8SA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763939058; c=relaxed/simple; bh=mKYWmNL4ToYQlw2oyJCvdrEDyTDZjColY5lUbVFaS/E=; h=Date:From:To:cc:Subject:In-Reply-To:Message-ID:References: MIME-Version:Content-Type; b=HcyWX7/1KwsIvzazlflv4QY6Tb4J7isc57fEtweNgGZfadEmGIc5HMMWfLiIlwseR1iqGdIa7Bq41uJrdnf2QkBoPdyyvl/8CZ0+pNTdCxV+g9Q9bbRJ7WQNLVHZ3YTfirYXHslpItlEJDOERnF0x6ZkiuAZYJ36//hv3ZzEXGg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=WbjAf7F9; arc=none smtp.client-ip=209.85.128.176 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="WbjAf7F9" Received: by mail-yw1-f176.google.com with SMTP id 00721157ae682-78aa49cde3dso3815527b3.1 for ; Sun, 23 Nov 2025 15:04:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1763939056; x=1764543856; darn=vger.kernel.org; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:from:to:cc:subject:date:message-id:reply-to; bh=4To7b+FdJCfm7x6NN8dJ7gobBokX+TP0wT9HA/9D15M=; b=WbjAf7F9nriWJMaxYm8Wz3hbNL/ArGMj/hy1aLatc3NAxtKEEfcW84did9DtB8RPJV IVEeWmgvpzauGgYi4tczU8fRhDY26r+LbBH5xMUINekES+Z7n1yByQhlslxC/anCzX9w cKvkA3Jau7o+P0l5CHHuof5kDIcy7G77nuo09rJ4YKBqTIRSgRoERKh3KCiSzXiazSt8 pfqau321NQZNWYAZ4DKbEyvBviPGFagYDt1KNSX+lv268ggAVzzGQO2nbgyhzeX7MHnr /SG3KDdLZrUWsOttMSNK2ENGAYQUQoUnC4A22kIcmoRarEXZxowfBFx2XMaJy9lW2T/C SJzg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763939056; x=1764543856; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=4To7b+FdJCfm7x6NN8dJ7gobBokX+TP0wT9HA/9D15M=; b=FpZMVfsqGRqqvN2NYQnZf+W60HYPS5h3/1MD7kcphQtQwd5vVFWEWKnMR9+pbFrcOT mJs6/8/1Fr3Zycm2LEOdkoQGzpjRaGGymeGszr69NmPLrxjDd8HosqeWqSMzPzNkEjVj Df+7OynRHIlmLpTWlSTgeR/r+F7f3yUmsLkvl6UFNng/IjFiRT5m4DWMY8HRaagWo5LT aEvhT7xfCZ8WdDVyNdqtzTEv9mmNvWhlLx0XaNTnn8H1d6sIQDWphGy3DeFRnupfOKZK yUKSJw1MTVqkD/V1muHTiDiFueBCADYm1LLww2IL8vnRgmCOZv+EJmhWskpW6Qo2q4O5 /9gQ== X-Forwarded-Encrypted: i=1; AJvYcCU1Z0Z9xl7M+8v7GV0SEYvGRw495AAXqaySEpJuYUgvTu0LoHsZ1ItpAIAF7+qZgK8BGy+KAn9AZLcSgl8=@vger.kernel.org X-Gm-Message-State: AOJu0Yx7jr3N6fq/T5T1/vNdKAiCJ6wXYFKiOeIRP2mT4PPCsf2+5IrT LgcRtLOssGFa1gfHKr1eYLIsXRBfaeIqbWOjBDeNmQpwZTugIy0BNweStUkGGSaNaw== X-Gm-Gg: ASbGncsM3lsxiQw65krbh+LjuDo6NuvC6CGVAWjx20nYHTd7vVIxLwdXO6LcaoQOEf8 eIVO8pSSdJ0a9S5JtjWUj1xAG1KK9UhRuN2I/X3v0b6Lt3t/P3tjAtlLTt/pt2oy5BZLPRrTEh2 jk4jGEe95ZjxUbdgybsDli5VUTJmSZOZlrp+cGfu1jV+JX2ed7B/ThaZAt67RztLCa/0U9ADS1w qtsUtvno5Zpc5DLFzPqg5DHoe9EAVkSi+4nS3l+cFkj3a2axIzDDLx4jhpZhrl0b+YpEXgkM2yS 448o3B7SezOSitQixnk6bB6w5z0r18uFR+2KX8Nb0PyjVaDOZ7Aw2uwrG0HA6E86a9blA8eQQmY VY3N/6ZyQFUENxTVRHlMsExEeCzyiUC48xRXr8g2Z/dCMHQe5YPzMngE5BKja1cJCtiGyd+PtAK nN2d2ErKWnlJTkdohT4FkxLxzl7Aj9CTsm73bgYCvHg/5YzRT0WbLT4aBZUsl2Tmslzm5uthYfo Elu5SHkHw== X-Google-Smtp-Source: AGHT+IF756jrV2oLibW5tf5NDM6N2r7s/mEMmh86sg0+8QTIPkOqCx8s0LJAvVrt7NhG09kGSX/0rg== X-Received: by 2002:a53:d048:0:10b0:63f:add1:e6da with SMTP id 956f58d0204a3-64302ac84a7mr6154857d50.57.1763939055759; Sun, 23 Nov 2025 15:04:15 -0800 (PST) Received: from darker.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id 00721157ae682-78a7987f5efsm38723527b3.9.2025.11.23.15.04.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 23 Nov 2025 15:04:14 -0800 (PST) Date: Sun, 23 Nov 2025 15:04:03 -0800 (PST) From: Hugh Dickins To: Vlastimil Babka cc: Hugh Dickins , Christoph Hellwig , Andrew Morton , Christoph Lameter , David Rientjes , Roman Gushchin , Harry Yoo , Suren Baghdasaryan , Michal Hocko , Brendan Jackman , Zi Yan , Eric Biggers , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 06/11] mempool: factor out a mempool_alloc_from_pool helper In-Reply-To: <23fac529-7b4d-4095-8c64-0d4a9d08c9b1@suse.cz> Message-ID: <8d043756-b59c-f604-f859-3f4803bbcd23@google.com> References: <20251113084022.1255121-1-hch@lst.de> <20251113084022.1255121-7-hch@lst.de> <7b1265bc-835e-4c7d-af75-f237c46bc3a7@suse.cz> <566ce586-4d53-f2d8-50b6-1f884f44d2c9@google.com> <23fac529-7b4d-4095-8c64-0d4a9d08c9b1@suse.cz> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII On Sun, 23 Nov 2025, Vlastimil Babka wrote: > On 11/23/25 18:49, Hugh Dickins wrote: > > On Sun, 23 Nov 2025, Vlastimil Babka wrote: > >> On 11/23/25 04:42, Hugh Dickins wrote: > >> > On Thu, 13 Nov 2025, Christoph Hellwig wrote: > >> > > >> > > >> > No, that is wrong, it breaks the mempool promise: linux-next oopses > >> > in swap_writepage_bdev_async(), which relies on bio_alloc(,,,GFP_NOIO) > >> > to return a good bio. > >> > > >> > The refactoring makes it hard to see, but the old version always used > >> > to go back to repeat_alloc at the end, if __GFP_DIRECT_RECLAIM, > >> > whereas here it only does so the first time, when gfp_temp != gfp_mask. > >> > > >> > After bisecting to here, I changed that "gfp_temp != gfp_mask" to > >> > "(gfp & __GFP_DIRECT_RECLAIM)", and it worked again. But other patches > >> > have come in on top, so below is a patch to the final mm/mempool.c... > >> > >> Thanks a lot Hugh and sorry for the trouble. > >> > >> Looking closer I noticed we're also not doing as the comment says about > >> passing the limited flags to mempool_alloc_from_pool() on the first attempt. > >> > >> I would also rather keep distinguishing the "retry with full flags" and > >> "retry because we can sleep" for now, in case there are callers that can't > >> sleep, but can benefit from memalloc context. It's hypothetical and I haven't > >> made an audit, but we can clean that up deliberately later and not as part > >> of a refactor patch. > >> > >> So I'd amend this patch with: > >> > >> diff --git a/mm/mempool.c b/mm/mempool.c > >> index c28087a3b8a9..224a4dead239 100644 > >> --- a/mm/mempool.c > >> +++ b/mm/mempool.c > >> @@ -478,10 +478,15 @@ void *mempool_alloc_noprof(mempool_t *pool, gfp_t gfp_mask) > >> * sleep in mempool_alloc_from_pool. Retry the allocation > >> * with all flags set in that case. > >> */ > >> - element = mempool_alloc_from_pool(pool, gfp_mask); > >> - if (!element && gfp_temp != gfp_mask) { > >> - gfp_temp = gfp_mask; > >> - goto repeat_alloc; > >> + element = mempool_alloc_from_pool(pool, gfp_temp); > > > > Haha, no. > > > > I had got excited when I too thought that should be gfp_temp not gfp_mask, > > but (a) it didn't fix the bug and (b) I then came to see that gfp_mask > > there is correct. > > > > It's looking ahead to what will be tried next: mempool_alloc_from_pool() > > is trying to alloc from mempool, and then, if it will be allowed to wait, > > waiting a suitable length of time, before letting the caller try again. > > If you substitute gfp_temp there, then it just does the same pool->alloc, > > alloc from mempool sequence twice in a row with no delay between (because > > gfp_temp does not at first allow waiting). > > But it's not exactly the same sequence, because in the second pass the > pool->alloc() has the original gfp flags restored (by gfp_temp = gfp_mask) > so it can now e.g. reclaim there. It's preferred to try that first before > waiting on a mempool refill. AFAIU the idea is to try succeeding quickly if > objects to allocate are either cheaply available to alloc() or in the pool, > and if that fails, go for the more expensive allocations or waiting for refill. > > AFAICS both the code before Christoph's changes, and after the changes with > my fixup do this: > > 1. pool->alloc(limited gfp) > 2. allocate from pool, but don't wait if there's nothing > 3. pool->alloc(full gfp) > 4. allocate from pool, wait if there's nothing > 5. goto 3 > > Am I missing something? You are completely right: it was me who was missing that the second pool->alloc is with the full gfp, so not an identical repeat of the the first; and so it is correct not to wait after the first attempt, so you are right to call with gfp_temp instead of gfp_mask there. (And this also addresses my slight concern, of whether it was appropriate to be doing a pool->alloc after waiting for a mempool free: your way, there is no such wait, at least not that most important first->second time: the wait would instead be inside the pool->alloc probably, reclaiming.) Yes, your fixup is better in all ways than mine, please go ahead. Thanks, Hugh > > > I agree it's confusing, and calls into question whether that was a good > > refactoring. Maybe there's a form of words for the comment above which > > I'd say it was intended to be good, apart from the bugs. > > > will make it clearer. Perhaps mempool_alloc_from_pool() is better split > > into two functions. Maybe gfp_temp could be named better. Etc etc: I > > preferred not to mess around further with how Christoph did it, not now. > > > > (I also wondered if it's right to pool->alloc before alloc from mempool > > after the wait was for a mempool element to be freed: but that's how it > > was before, and I expect it's been proved in the past that a strict > > pool->alloc before alloc from mempool is the best strategy.) > > I'd think we better do it that way, otherwise se might be recovering more > slowly from a temporary memory shortage that cause a number of tasks to wait > in the mempool, which would then have to wait for mempool refills even > though new objects might be available to allocate thanks to the shortage > resolved. > > >> + if (!element) { > >> + if (gfp_temp != gfp_mask) { > >> + gfp_temp = gfp_mask; > >> + goto repeat_alloc; > >> + } > >> + if (gfp_mask & __GFP_DIRECT_RECLAIM) { > >> + goto repeat_alloc; > >> + } > > > > I still prefer what I posted. > > > > Hugh > > > >> } > >> } > >> > >> > >> With the followup commit fixed up during rebase, the diff of the whole > >> branch before/after is: > >> > >> diff --git a/mm/mempool.c b/mm/mempool.c > >> index 5953fe801395..bb596cac57ff 100644 > >> --- a/mm/mempool.c > >> +++ b/mm/mempool.c > >> @@ -555,10 +555,14 @@ void *mempool_alloc_noprof(struct mempool *pool, gfp_t gfp_mask) > >> * sleep in mempool_alloc_from_pool. Retry the allocation > >> * with all flags set in that case. > >> */ > >> - if (!mempool_alloc_from_pool(pool, &element, 1, 0, gfp_mask) && > >> - gfp_temp != gfp_mask) { > >> - gfp_temp = gfp_mask; > >> - goto repeat_alloc; > >> + if (!mempool_alloc_from_pool(pool, &element, 1, 0, gfp_temp)) { > >> + if (gfp_temp != gfp_mask) { > >> + gfp_temp = gfp_mask; > >> + goto repeat_alloc; > >> + } > >> + if (gfp_mask & __GFP_DIRECT_RECLAIM) { > >> + goto repeat_alloc; > >> + } > >> } > >> } > >> > >