mm/slub.c | 57 ++++++++++++++++++++------------------------------------- 1 file changed, 20 insertions(+), 37 deletions(-)
The function allocates and then refills and empty sheaf. It's only
called from __pcs_replace_empty_main(), which can also in some cases
refill an empty sheaf. We can therefore consolidate this code.
Remove alloc_full_sheaf() and refactor __pcs_replace_empty_main() so it
will call alloc_empty_sheaf() when necessary, and then use the
pre-existing refill_sheaf(). The result should be simpler to follow and
less duplicated code.
Also adjust the comment about returning sheaves to barn, the part about
where the empty sheaf we'd be returning comes from is incorrect.
No functional change intended.
Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
---
Just something I noticed when applying Qing's hotfix, thus a followup.
But since it's a cleanup, it would be for 7.1.
---
mm/slub.c | 57 ++++++++++++++++++++-------------------------------------
1 file changed, 20 insertions(+), 37 deletions(-)
diff --git a/mm/slub.c b/mm/slub.c
index 2b2d33cc735c..a8347b79e46f 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2822,24 +2822,6 @@ static int refill_sheaf(struct kmem_cache *s, struct slab_sheaf *sheaf,
return 0;
}
-static void sheaf_flush_unused(struct kmem_cache *s, struct slab_sheaf *sheaf);
-
-static struct slab_sheaf *alloc_full_sheaf(struct kmem_cache *s, gfp_t gfp)
-{
- struct slab_sheaf *sheaf = alloc_empty_sheaf(s, gfp);
-
- if (!sheaf)
- return NULL;
-
- if (refill_sheaf(s, sheaf, gfp | __GFP_NOMEMALLOC | __GFP_NOWARN)) {
- sheaf_flush_unused(s, sheaf);
- free_empty_sheaf(s, sheaf);
- return NULL;
- }
-
- return sheaf;
-}
-
/*
* Maximum number of objects freed during a single flush of main pcs sheaf.
* Translates directly to an on-stack array size.
@@ -4611,34 +4593,35 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
if (!allow_spin)
return NULL;
- if (empty) {
- if (!refill_sheaf(s, empty, gfp | __GFP_NOMEMALLOC | __GFP_NOWARN)) {
- full = empty;
- } else {
- /*
- * we must be very low on memory so don't bother
- * with the barn
- */
- sheaf_flush_unused(s, empty);
- free_empty_sheaf(s, empty);
- }
- } else {
- full = alloc_full_sheaf(s, gfp);
+ if (!empty) {
+ empty = alloc_empty_sheaf(s, gfp);
+ if (!empty)
+ return NULL;
}
- if (!full)
+ if (refill_sheaf(s, empty, gfp | __GFP_NOMEMALLOC | __GFP_NOWARN)) {
+ /*
+ * we must be very low on memory so don't bother
+ * with the barn
+ */
+ sheaf_flush_unused(s, empty);
+ free_empty_sheaf(s, empty);
+
return NULL;
+ }
+
+ full = empty;
+ empty = NULL;
if (!local_trylock(&s->cpu_sheaves->lock))
goto barn_put;
pcs = this_cpu_ptr(s->cpu_sheaves);
/*
- * If we are returning empty sheaf, we either got it from the
- * barn or had to allocate one. If we are returning a full
- * sheaf, it's due to racing or being migrated to a different
- * cpu. Breaching the barn's sheaf limits should be thus rare
- * enough so just ignore them to simplify the recovery.
+ * If we put any empty or full sheaf to the barn below, it's due to
+ * racing or being migrated to a different cpu. Breaching the barn's
+ * sheaf limits should be thus rare enough so just ignore them to
+ * simplify the recovery.
*/
if (pcs->main->size == 0) {
---
base-commit: 464b1c115852fe025635ae2065e00caced184d92
change-id: 20260311-b4-slab-remove-alloc_full_sheaf-b3404881d45f
Best regards,
--
Vlastimil Babka (SUSE) <vbabka@kernel.org>On Wed, Mar 11, 2026 at 07:22:33PM +0100, Vlastimil Babka (SUSE) wrote: > The function allocates and then refills and empty sheaf. It's only nit: ^an empty sheaf? > called from __pcs_replace_empty_main(), which can also in some cases > refill an empty sheaf. We can therefore consolidate this code. > > Remove alloc_full_sheaf() and refactor __pcs_replace_empty_main() so it > will call alloc_empty_sheaf() when necessary, and then use the > pre-existing refill_sheaf(). The result should be simpler to follow and > less duplicated code. > > Also adjust the comment about returning sheaves to barn, the part about > where the empty sheaf we'd be returning comes from is incorrect. > > No functional change intended. > > Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org> > --- Nice cleanup! Looks good to me, Reviewed-by: Harry Yoo <harry.yoo@oracle.com> -- Cheers, Harry / Hyeonggon
On Thu, Mar 12, 2026 at 01:36:11PM +0900, Harry Yoo wrote: > On Wed, Mar 11, 2026 at 07:22:33PM +0100, Vlastimil Babka (SUSE) wrote: > > The function allocates and then refills and empty sheaf. It's only > > nit: ^an empty sheaf? > > > called from __pcs_replace_empty_main(), which can also in some cases > > refill an empty sheaf. We can therefore consolidate this code. > > > > Remove alloc_full_sheaf() and refactor __pcs_replace_empty_main() so it > > will call alloc_empty_sheaf() when necessary, and then use the > > pre-existing refill_sheaf(). The result should be simpler to follow and > > less duplicated code. > > > > Also adjust the comment about returning sheaves to barn, the part about > > where the empty sheaf we'd be returning comes from is incorrect. > > > > No functional change intended. > > > > Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org> > > --- > > Nice cleanup! Indeed! Reviewed-by: Hao Li <hao.li@linux.dev> -- Thanks, Hao
On Thu, 12 Mar 2026 at 02:22, "Vlastimil Babka (SUSE)" <vbabka@kernel.org> wrote:
> - if (!full)
> + if (refill_sheaf(s, empty, gfp | __GFP_NOMEMALLOC | __GFP_NOWARN)) {
> + /*
> + * we must be very low on memory so don't bother
> + * with the barn
> + */
> + sheaf_flush_unused(s, empty);
> + free_empty_sheaf(s, empty);
> +
> return NULL;
> + }
> +
> + full = empty;
> + empty = NULL;
'empty = NULL' is meaningless since 'empty' is not used after here.
>
> if (!local_trylock(&s->cpu_sheaves->lock))
> goto barn_put;
> pcs = this_cpu_ptr(s->cpu_sheaves);
>
> /*
> - * If we are returning empty sheaf, we either got it from the
> - * barn or had to allocate one. If we are returning a full
> - * sheaf, it's due to racing or being migrated to a different
> - * cpu. Breaching the barn's sheaf limits should be thus rare
> - * enough so just ignore them to simplify the recovery.
> + * If we put any empty or full sheaf to the barn below, it's due to
> + * racing or being migrated to a different cpu. Breaching the barn's
> + * sheaf limits should be thus rare enough so just ignore them to
> + * simplify the recovery.
> */
>
> if (pcs->main->size == 0) {
LGTM.
Reviewed-by: Qing Wang <wangqing7171@gmail.com>
--
Cheers,
Qing
On 3/12/26 04:31, Qing Wang wrote:
> On Thu, 12 Mar 2026 at 02:22, "Vlastimil Babka (SUSE)" <vbabka@kernel.org> wrote:
>> - if (!full)
>> + if (refill_sheaf(s, empty, gfp | __GFP_NOMEMALLOC | __GFP_NOWARN)) {
>> + /*
>> + * we must be very low on memory so don't bother
>> + * with the barn
>> + */
>> + sheaf_flush_unused(s, empty);
>> + free_empty_sheaf(s, empty);
>> +
>> return NULL;
>> + }
>> +
>> + full = empty;
>> + empty = NULL;
>
> 'empty = NULL' is meaningless since 'empty' is not used after here.
Exactly, it's only for humans reading the code to make it explicit. And if
someone tries to make a change that will start using empty, it will oops
immediately instead of something more subtle.
>>
>> if (!local_trylock(&s->cpu_sheaves->lock))
>> goto barn_put;
>> pcs = this_cpu_ptr(s->cpu_sheaves);
>>
>> /*
>> - * If we are returning empty sheaf, we either got it from the
>> - * barn or had to allocate one. If we are returning a full
>> - * sheaf, it's due to racing or being migrated to a different
>> - * cpu. Breaching the barn's sheaf limits should be thus rare
>> - * enough so just ignore them to simplify the recovery.
>> + * If we put any empty or full sheaf to the barn below, it's due to
>> + * racing or being migrated to a different cpu. Breaching the barn's
>> + * sheaf limits should be thus rare enough so just ignore them to
>> + * simplify the recovery.
>> */
>>
>> if (pcs->main->size == 0) {
>
> LGTM.
>
> Reviewed-by: Qing Wang <wangqing7171@gmail.com>
Thanks!
> --
> Cheers,
> Qing
On Thu, 12 Mar 2026 at 17:10, vbabka@kernel.org wrote: > Exactly, it's only for humans reading the code to make it explicit. And if > someone tries to make a change that will start using empty, it will oops > immediately instead of something more subtle. Indeed, thanks for your explaination. -- Cheers, Qing
© 2016 - 2026 Red Hat, Inc.