From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yw1-f169.google.com (mail-yw1-f169.google.com [209.85.128.169]) (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 1080272627 for ; Sun, 23 Nov 2025 17:49:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.169 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763920162; cv=none; b=Na6/TKfYoNl1TjYBvgjkDEqE4m5s231VLzLrl/lQhoqitEtOfjc02iFLw62hBYJ+pxnukxpk0IXlSG5VPikA5q7y3aBSJobPN0WqhW3Z/cBKa9hJAF9NDMca9jHx3KLZBQL8MJDIoMq27SLU1cmUegjAdb5B5bpa+LlSojKfxGg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763920162; c=relaxed/simple; bh=qD//lM4OJT6jBR9EsS28wcNVm4SzTQ8kC3+lqrjbNrQ=; h=Date:From:To:cc:Subject:In-Reply-To:Message-ID:References: MIME-Version:Content-Type; b=dPSAJ89sBSu8IMNbnD8Y7kkELVsYvG4fbo9vNl1AGU4FbhrCfQWmxummxMjidnEbLpw2ylQgw2p9AjBVefLh4QSkQdIEFLQVJjmPu1zh+CRZUnuJTfGsVQTHh/9tGDdQkcaW7DAiFD1mFaXpjQrKwcYoM9CwpOcTL08r03SraIk= 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=F1CkNo6K; arc=none smtp.client-ip=209.85.128.169 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="F1CkNo6K" Received: by mail-yw1-f169.google.com with SMTP id 00721157ae682-787da30c50fso34378087b3.3 for ; Sun, 23 Nov 2025 09:49:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1763920160; x=1764524960; 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=5lLgXKdZ+zTDTzgdon+lya5F7bzs0gv5/kPgOiRFpQ8=; b=F1CkNo6K5N95frNFvpmstDjIEm3bJUymMynJefFds6aH7ES/mUjvZ6i95UozuctLzu C1GEmgYz/mB73GgHAkmNQwjfLLMbBf25KhNT5xDrMuZvmGxHEaaX9K3KIYzutVhRrBoY Gisbc6/SH1KFh2ZRI9wfyJrKGHxTYmKayuKrfhfaTXsLVKoeP7KxXuXetXu60bTim8PA 13z95t6YAjqxgGIQAaB/lHD4KX/sUZ2grA6+6HzamsgizM55nwWD089Q63tZLr5wk7hH gxo2cmvRGfIrUzwAiOITr2/V2ztXLxIBHX9OuZ22M/+g6IhTsG48iMUB3X9EwpcQvymV PBvQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763920160; x=1764524960; 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=5lLgXKdZ+zTDTzgdon+lya5F7bzs0gv5/kPgOiRFpQ8=; b=MtLrZMjlI6a29IfzvuIBSKyY2piJzo+l1tiBmKhUZxb+CoJ2N74FhfkTmo7Q7p49Rb Tdsd4fVQqImI05rKYNTgX+3XM2KfNWtJDMG5iSVKU64X6MOuakC3UsrIkspUt22Akb+1 jUjHnf1lwhqXWkhN2jNO9NARHY2R6NsFhUdcyYkS71JCkRhlzZZpgbQNaCA0BPy77Wtf e7MfVODAU4MpfXLCY/coJZ9P4DwZY4MHEOohaI3whS4tglHYH9n2QfI0Q8kgzoTy3Sd8 MdRZF9CytnFjUtmltznLR5GfeHKAV9bZcPIOKE7ChX8WFrvnVjT0eWKqbFx6MHM23lSC MpWw== X-Forwarded-Encrypted: i=1; AJvYcCV5n+rJwcPc1dKLzxuDOATRK4JdB3LyNPwz8zikAzRoLAX+QZAf9o1vP0l0GQXEC+E39mhOEjGlmXrYVtw=@vger.kernel.org X-Gm-Message-State: AOJu0Yw0oO7Qf62RqwHYvNnKeYIp3kCwShW7E5uyq0v0AQjp8C7t35nu +LlSfE2cwsFLM5ousmscdwk5Cmbxh5lNjojg49t3LtSaSyQBxkci7yRLxDKtCYzg3g== X-Gm-Gg: ASbGncvq8yxAVlBsl3n0cGtZROZfWm5jrfub/zLEqYZfnXGm+uZlCRuPOlhLtPI1D6o Q3NMc5U2fgFvGxmIdW1wGjgblpS7QFTWecsfPZMnYtvHWStVyxHngHJVtcXtYQ+sIsv+ctP3myg Y4Qx7b4r5ANLQyvGnL6AWTAnMyRkKIfn7yUGBzb/1mh2eEjCKZQOlILNc/RrWEgpZYzrw65pWVs n2716hwar7u/fyNrU45xRLO2HECWl2zXskSntKsPtUTcS0py0Lg7uXGsPii0HaFiapEVs71idIo Vi9mjjgGGNoLAp3CwuNxY7ejM75lB3lVbaLbbWoVlAkKx+u2qRmp9K2xxTyzn88EEqXBP0tA4RX J6UuJKdfjZz4lMr7RQ/irQg7xVCEGFR3aZ7YFqWPdGhI8BmMJgvEgTNa3QzukpuUz5vhSahbQTq sTfGtQ+wrBjrI1Tz2L8rhpeZiBvNu4znqSolv5YdVQmQVNho+x9NRoXrqxQB3yDTuW7M+H5dI= X-Google-Smtp-Source: AGHT+IGbDSkHYRbM7lgco7/onahBlAGpTDd64qpGSNVB0DFL/YhCO2ClP1ZFOcTnwsTgOv51dJxnIw== X-Received: by 2002:a05:690c:34ca:b0:786:4459:cb84 with SMTP id 00721157ae682-78a8b4c5749mr154845607b3.29.1763920159658; Sun, 23 Nov 2025 09:49:19 -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 956f58d0204a3-642f71ae739sm4009148d50.25.2025.11.23.09.49.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 23 Nov 2025 09:49:18 -0800 (PST) Date: Sun, 23 Nov 2025 09:49:08 -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: <7b1265bc-835e-4c7d-af75-f237c46bc3a7@suse.cz> Message-ID: <566ce586-4d53-f2d8-50b6-1f884f44d2c9@google.com> References: <20251113084022.1255121-1-hch@lst.de> <20251113084022.1255121-7-hch@lst.de> <7b1265bc-835e-4c7d-af75-f237c46bc3a7@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 04:42, Hugh Dickins wrote: > > On Thu, 13 Nov 2025, Christoph Hellwig wrote: > > > >> Add a helper for the mempool_alloc slowpath to better separate it from the > >> fast path, and also use it to implement mempool_alloc_preallocated which > >> shares the same logic. > >> > >> Signed-off-by: Christoph Hellwig > >> --- > > ... > >> @@ -413,8 +457,6 @@ void *mempool_alloc_noprof(mempool_t *pool, gfp_t gfp_mask) > >> { > >> gfp_t gfp_temp = mempool_adjust_gfp(&gfp_mask); > >> void *element; > >> - unsigned long flags; > >> - wait_queue_entry_t wait; > >> > >> VM_WARN_ON_ONCE(gfp_mask & __GFP_ZERO); > >> might_alloc(gfp_mask); > >> @@ -428,53 +470,22 @@ void *mempool_alloc_noprof(mempool_t *pool, gfp_t gfp_mask) > >> element = pool->alloc(gfp_temp, pool->pool_data); > >> } > >> > >> - if (likely(element)) > >> - return element; > >> - > >> - spin_lock_irqsave(&pool->lock, flags); > >> - if (likely(pool->curr_nr)) { > >> - element = remove_element(pool); > >> - spin_unlock_irqrestore(&pool->lock, flags); > >> - /* paired with rmb in mempool_free(), read comment there */ > >> - smp_wmb(); > >> + if (unlikely(!element)) { > >> /* > >> - * Update the allocation stack trace as this is more useful > >> - * for debugging. > >> + * Try to allocate an element from the pool. > >> + * > >> + * The first pass won't have __GFP_DIRECT_RECLAIM and won't > >> + * sleep in mempool_alloc_from_pool. Retry the allocation > >> + * with all flags set in that case. > >> */ > >> - kmemleak_update_trace(element); > >> - return element; > >> - } > >> - > >> - /* > >> - * We use gfp mask w/o direct reclaim or IO for the first round. If > >> - * alloc failed with that and @pool was empty, retry immediately. > >> - */ > >> - if (gfp_temp != gfp_mask) { > >> - spin_unlock_irqrestore(&pool->lock, flags); > >> - gfp_temp = gfp_mask; > >> - goto repeat_alloc; > >> - } > >> - > >> - /* We must not sleep if !__GFP_DIRECT_RECLAIM */ > >> - if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) { > >> - spin_unlock_irqrestore(&pool->lock, flags); > >> - return NULL; > >> + element = mempool_alloc_from_pool(pool, gfp_mask); > >> + if (!element && gfp_temp != gfp_mask) { > > > > 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). 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 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.) > + 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; > + } > } > } >