Re: [PATCH v2 24/33] mm/slob: Convert SLOB to use struct slab
From: Vlastimil Babka <hidden>
Date: 2021-12-10 18:10:21
Also in:
linux-patches
On 12/10/21 16:29, Hyeonggon Yoo wrote:
On Fri, Dec 10, 2021 at 12:44:32PM +0100, Vlastimil Babka wrote:quoted
On 12/10/21 11:44, Hyeonggon Yoo wrote:quoted
On Wed, Dec 01, 2021 at 07:15:01PM +0100, Vlastimil Babka wrote:quoted
From: "Matthew Wilcox (Oracle)" <willy@infradead.org> Use struct slab throughout the slob allocator. [ vbabka@suse.cz: don't introduce wrappers for PageSlobFree in mm/slab.h just for the single callers being wrappers in mm/slob.c ] Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Signed-off-by: Vlastimil Babka <redacted> --- mm/slob.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)diff --git a/mm/slob.c b/mm/slob.c index d2d15e7f191c..d3512bcc3141 100644 --- a/mm/slob.c +++ b/mm/slob.c...quoted
/* Enough room on this page? */@@ -358,8 +358,8 @@ static void *slob_alloc(size_t size, gfp_t gfp, int align, int node, b = slob_new_pages(gfp & ~__GFP_ZERO, 0, node); if (!b) return NULL; - sp = virt_to_page(b); - __SetPageSlab(sp); + sp = virt_to_slab(b); + __SetPageSlab(slab_page(sp));Hello Vlastimil. I've tested this patch on my machine and it causes NULL pointer dereference. that's because virt_to_slab returns NULL if folio_test_slab is false. and __SetPageSlab is called with sp = NULL.Oops, thanks for catching this.quoted
diff below fixed bug.Changed it to use folio and will push an updated branch slab-struct_slab-v3r2 Thanks!Tested again on slab-struct_slab-v3r2 and everyting works fine! And the code overall looks good to me. Reviewed-by: Hyeonggon Yoo <redacted> Tested-by: Hyeonggon Yoo <redacted>
Thanks!
And below is not about this patch, but what about something like this? Because it's not necessary that page->_mapcount and slab->units have same offset, so we can remove (somewhat confusing) page_mapcount_reset function call.
Yeah this could be done as new patch on top of 21/33. I wouldn't squash it there so that it remains just a split without functional change.
quoted hunk ↗ jump to hunk
I want to remove calling page_mapcount_reset also in SLAB, but I have no idea. (maybe we can do that if we remove SLAB colouring but that's going too far)diff --git a/mm/slab.h b/mm/slab.h index 90d7fceba470..dd0480149d38 100644 --- a/mm/slab.h +++ b/mm/slab.h@@ -50,8 +50,8 @@ struct slab { struct list_head slab_list; void * __unused_1; void *freelist; /* first free block */ - void * __unused_2; - int units; + long units; + unsigned int __unused_2; #else #error "Unexpected slab allocator configured"diff --git a/mm/slob.c b/mm/slob.c index 39b651b3e6e7..7b2d2c7d69cc 100644 --- a/mm/slob.c +++ b/mm/slob.c@@ -404,7 +404,6 @@ static void slob_free(void *block, int size) clear_slob_page_free(sp); spin_unlock_irqrestore(&slob_lock, flags); __ClearPageSlab(slab_page(sp)); - page_mapcount_reset(slab_page(sp)); slob_free_pages(b, 0); return; }quoted
quoted
diff --git a/mm/slob.c b/mm/slob.c index d3512bcc3141..cf669f03440f 100644 --- a/mm/slob.c +++ b/mm/slob.c@@ -358,8 +358,8 @@ static void *slob_alloc(size_t size, gfp_t gfp, int align, int node, b = slob_new_pages(gfp & ~__GFP_ZERO, 0, node); if (!b) return NULL; + __SetPageSlab(virt_to_page(b)); sp = virt_to_slab(b); - __SetPageSlab(slab_page(sp)); spin_lock_irqsave(&slob_lock, flags); sp->units = SLOB_UNITS(PAGE_SIZE); Thanks, Hyeonggon.quoted
spin_lock_irqsave(&slob_lock, flags); sp->units = SLOB_UNITS(PAGE_SIZE);@@ -381,7 +381,7 @@ static void *slob_alloc(size_t size, gfp_t gfp, int align, int node, */ static void slob_free(void *block, int size) { - struct page *sp; + struct slab *sp; slob_t *prev, *next, *b = (slob_t *)block; slobidx_t units; unsigned long flags;@@ -391,7 +391,7 @@ static void slob_free(void *block, int size) return; BUG_ON(!size); - sp = virt_to_page(block); + sp = virt_to_slab(block); units = SLOB_UNITS(size); spin_lock_irqsave(&slob_lock, flags);@@ -401,8 +401,8 @@ static void slob_free(void *block, int size) if (slob_page_free(sp)) clear_slob_page_free(sp); spin_unlock_irqrestore(&slob_lock, flags); - __ClearPageSlab(sp); - page_mapcount_reset(sp); + __ClearPageSlab(slab_page(sp)); + page_mapcount_reset(slab_page(sp)); slob_free_pages(b, 0); return; }-- 2.33.1