All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] mm: slub: merge get_freelist into ___slab_alloc
@ 2021-09-18  7:15 Hyeonggon Yoo
  2021-09-18  7:41 ` Sorry for annoyance. it's buggy Hyeonggon Yoo
  0 siblings, 1 reply; 2+ messages in thread
From: Hyeonggon Yoo @ 2021-09-18  7:15 UTC (permalink / raw)
  To: 42.hyeyoo
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, linux-mm, linux-kernel

get_freelist was introduced commit 213eeb9fd9d6 ("slub: Extract
get_freelist from __slab_alloc") and its comment says this function
transfers freelist to the per cpu freelist or deactivate page.

But what it actually does is just deactivating page and returning
its freelist. And the code working with variable 'new' is confusing
because it does nothing.

This function have been unmaintained for a long time and is confusing
reader. So simplify it for better readability.

Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
 mm/slub.c | 46 +++++++++++-----------------------------------
 1 file changed, 11 insertions(+), 35 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 3d2025f7163b..26fe1eb50d88 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2815,40 +2815,6 @@ static inline bool pfmemalloc_match_unsafe(struct page *page, gfp_t gfpflags)
 	return true;
 }
 
-/*
- * Check the page->freelist of a page and either transfer the freelist to the
- * per cpu freelist or deactivate the page.
- *
- * The page is still frozen if the return value is not NULL.
- *
- * If this function returns NULL then the page has been unfrozen.
- */
-static inline void *get_freelist(struct kmem_cache *s, struct page *page)
-{
-	struct page new;
-	unsigned long counters;
-	void *freelist;
-
-	lockdep_assert_held(this_cpu_ptr(&s->cpu_slab->lock));
-
-	do {
-		freelist = page->freelist;
-		counters = page->counters;
-
-		new.counters = counters;
-		VM_BUG_ON(!new.frozen);
-
-		new.inuse = page->objects;
-		new.frozen = freelist != NULL;
-
-	} while (!__cmpxchg_double_slab(s, page,
-		freelist, counters,
-		NULL, new.counters,
-		"get_freelist"));
-
-	return freelist;
-}
-
 /*
  * Slow path. The lockless freelist is empty or we need to perform
  * debugging duties.
@@ -2874,6 +2840,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
 	void *freelist;
 	struct page *page;
 	unsigned long flags;
+	unsigned long counters;
 
 	stat(s, ALLOC_SLOWPATH);
 
@@ -2920,12 +2887,21 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
 		local_unlock_irqrestore(&s->cpu_slab->lock, flags);
 		goto reread_page;
 	}
+
 	freelist = c->freelist;
 	if (freelist)
 		goto load_freelist;
 
-	freelist = get_freelist(s, page);
+	/* deactivate page */
+	do {
+		freelist = page->freelist;
+		counters = page->counters;
+	} while (!__cmpxchg_double_slab(s, page,
+		freelist, counters,
+		NULL, counters,
+		"deactivate_page"));
 
+	/* there was no free objects in that page  */
 	if (!freelist) {
 		c->page = NULL;
 		local_unlock_irqrestore(&s->cpu_slab->lock, flags);
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Sorry for annoyance. it's buggy.
  2021-09-18  7:15 [RFC PATCH] mm: slub: merge get_freelist into ___slab_alloc Hyeonggon Yoo
@ 2021-09-18  7:41 ` Hyeonggon Yoo
  0 siblings, 0 replies; 2+ messages in thread
From: Hyeonggon Yoo @ 2021-09-18  7:41 UTC (permalink / raw)
  To: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, linux-mm, linux-kernel,
	42.hyeyoo

it was actually misunderstanding.

I misunderstood counters - it is union, not just unsigned long :(
So changing inuse and frozen affects count and it's not meaningless. 

Sorry for annoyance.

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-09-18  7:42 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-18  7:15 [RFC PATCH] mm: slub: merge get_freelist into ___slab_alloc Hyeonggon Yoo
2021-09-18  7:41 ` Sorry for annoyance. it's buggy Hyeonggon Yoo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.