| Message ID | 20230310103210.22372-1-vbabka@suse.cz (mailing list archive) |
|---|---|
| Headers | show |
| Series | remove SLOB and allow kfree() with kmem_cache_alloc() | expand |
On Fri, 10 Mar 2023 11:32:02 +0100 Vlastimil Babka wrote: > Otherwise it's straightforward. Patch 2 is a cleanup in net area, that I > can either handle in slab tree or submit in net after SLOB is removed. Letter would be better, if you don't mind. skbuff.c is relatively hot, there's a good chance we'll create a conflict.
Hi Vlastimil, On Fri, Mar 10, 2023 at 11:32:02AM +0100, Vlastimil Babka wrote: > Also in git: > https://git.kernel.org/vbabka/h/slab-remove-slob-v1r1 > > The SLOB allocator was deprecated in 6.2 so I think we can start > exposing the complete removal in for-next and aim at 6.4 if there are no > complaints. > > Besides code cleanup, the main immediate benefit will be allowing > kfree() family of function to work on kmem_cache_alloc() objects (Patch > 7), which was incompatible with SLOB. > > This includes kfree_rcu() so I've updated the comment there to remove > the mention of potential future addition of kmem_cache_free_rcu() as > there should be no need for that now. > > Otherwise it's straightforward. Patch 2 is a cleanup in net area, that I > can either handle in slab tree or submit in net after SLOB is removed. > Another cleanup in tomoyo is already in the tomoyo tree as that didn't > need to wait until SLOB removal. > > Vlastimil Babka (7): > mm/slob: remove CONFIG_SLOB > net: skbuff: remove SLOB-specific ifdefs > mm, page_flags: remove PG_slob_free > mm, pagemap: remove SLOB and SLQB from comments and documentation > mm/slab: remove CONFIG_SLOB code from slab common code > mm/slob: remove slob.c > mm/slab: document kfree() as allowed for kmem_cache_alloc() objects > > Documentation/admin-guide/mm/pagemap.rst | 6 +- > Documentation/core-api/memory-allocation.rst | 15 +- > fs/proc/page.c | 5 +- > include/linux/page-flags.h | 4 - > include/linux/rcupdate.h | 6 +- > include/linux/slab.h | 39 - > init/Kconfig | 2 +- > kernel/configs/tiny.config | 1 - > mm/Kconfig | 22 - > mm/Makefile | 1 - > mm/slab.h | 61 -- > mm/slab_common.c | 7 +- > mm/slob.c | 757 ------------------- > net/core/skbuff.c | 16 - > tools/mm/page-types.c | 6 +- > 15 files changed, 23 insertions(+), 925 deletions(-) > delete mode 100644 mm/slob.c git grep -in slob still gives a couple of matches. I've dropped the irrelevant ones it it left me with these: CREDITS:14:D: SLOB slab allocator kernel/trace/ring_buffer.c:358: * Also stolen from mm/slob.c. Thanks to Mathieu Desnoyers for pointing mm/Kconfig:251: SLOB allocator and is not recommended for systems with more than mm/Makefile:25:KCOV_INSTRUMENT_slob.o := n Except the comment in kernel/trace/ring_buffer.c all are trivial. As for the comment in ring_buffer.c, it looks completely irrelevant at this point. @Steve? > -- > 2.39.2 >
On Sun, 12 Mar 2023 11:51:29 +0200 Mike Rapoport <mike.rapoport@gmail.com> wrote: > git grep -in slob still gives a couple of matches. I've dropped the > irrelevant ones it it left me with these: > > CREDITS:14:D: SLOB slab allocator > kernel/trace/ring_buffer.c:358: * Also stolen from mm/slob.c. Thanks to Mathieu Desnoyers for pointing > mm/Kconfig:251: SLOB allocator and is not recommended for systems with more than > mm/Makefile:25:KCOV_INSTRUMENT_slob.o := n > > Except the comment in kernel/trace/ring_buffer.c all are trivial. > > As for the comment in ring_buffer.c, it looks completely irrelevant at this > point. > > @Steve? You want me to remember something I wrote almost 15 years ago? I think I understand that comment as much as you do. Yeah, that was when I was still learning to write comments for my older self to understand, and I failed miserably! But git history comes to the rescue. The commit that added that comment was: ed56829cb3195 ("ring_buffer: reset buffer page when freeing") This was at a time when it was suggested to me to use the struct page directly in the ring buffer and where we could do fun "tricks" for "performance". (I was never really for this, but I wasn't going to argue). And the code in question then had: /* * Also stolen from mm/slob.c. Thanks to Mathieu Desnoyers for pointing * this issue out. */ static inline void free_buffer_page(struct buffer_page *bpage) { reset_page_mapcount(&bpage->page); bpage->page.mapping = NULL; __free_page(&bpage->page); } But looking at commit: e4c2ce82ca27 ("ring_buffer: allocate buffer page pointer") It was finally decided that method was not safe, and we should not be using struct page but just allocate an actual page (much safer!). I never got rid of the comment, which was more about that "reset_page_mapcount()", and should have been deleted back then. Just remove that comment. And you could even add: Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org> Fixes: e4c2ce82ca27 ("ring_buffer: allocate buffer page pointer") -- Steve
On 3/12/23 10:51, Mike Rapoport wrote: > Hi Vlastimil, > > On Fri, Mar 10, 2023 at 11:32:02AM +0100, Vlastimil Babka wrote: >> Also in git: >> https://git.kernel.org/vbabka/h/slab-remove-slob-v1r1 >> >> The SLOB allocator was deprecated in 6.2 so I think we can start >> exposing the complete removal in for-next and aim at 6.4 if there are no >> complaints. >> >> Besides code cleanup, the main immediate benefit will be allowing >> kfree() family of function to work on kmem_cache_alloc() objects (Patch >> 7), which was incompatible with SLOB. >> >> This includes kfree_rcu() so I've updated the comment there to remove >> the mention of potential future addition of kmem_cache_free_rcu() as >> there should be no need for that now. >> >> Otherwise it's straightforward. Patch 2 is a cleanup in net area, that I >> can either handle in slab tree or submit in net after SLOB is removed. >> Another cleanup in tomoyo is already in the tomoyo tree as that didn't >> need to wait until SLOB removal. >> >> Vlastimil Babka (7): >> mm/slob: remove CONFIG_SLOB >> net: skbuff: remove SLOB-specific ifdefs >> mm, page_flags: remove PG_slob_free >> mm, pagemap: remove SLOB and SLQB from comments and documentation >> mm/slab: remove CONFIG_SLOB code from slab common code >> mm/slob: remove slob.c >> mm/slab: document kfree() as allowed for kmem_cache_alloc() objects >> >> Documentation/admin-guide/mm/pagemap.rst | 6 +- >> Documentation/core-api/memory-allocation.rst | 15 +- >> fs/proc/page.c | 5 +- >> include/linux/page-flags.h | 4 - >> include/linux/rcupdate.h | 6 +- >> include/linux/slab.h | 39 - >> init/Kconfig | 2 +- >> kernel/configs/tiny.config | 1 - >> mm/Kconfig | 22 - >> mm/Makefile | 1 - >> mm/slab.h | 61 -- >> mm/slab_common.c | 7 +- >> mm/slob.c | 757 ------------------- >> net/core/skbuff.c | 16 - >> tools/mm/page-types.c | 6 +- >> 15 files changed, 23 insertions(+), 925 deletions(-) >> delete mode 100644 mm/slob.c > > git grep -in slob still gives a couple of matches. I've dropped the > irrelevant ones it it left me with these: > > CREDITS:14:D: SLOB slab allocator I think it wouldn't be fair to remove that one as it's a historical record of some sort? > kernel/trace/ring_buffer.c:358: * Also stolen from mm/slob.c. Thanks to Mathieu Desnoyers for pointing > mm/Kconfig:251: SLOB allocator and is not recommended for systems with more than Yeah that's a help text for SLUB_TINY which can still help those who migrate from SLOB. > mm/Makefile:25:KCOV_INSTRUMENT_slob.o := n That one I will remove, thanks! > Except the comment in kernel/trace/ring_buffer.c all are trivial. > > As for the comment in ring_buffer.c, it looks completely irrelevant at this > point. > > @Steve? > >> -- >> 2.39.2 >> >
On Mon, Mar 13, 2023 at 12:31:47PM -0400, Steven Rostedt wrote: > On Sun, 12 Mar 2023 11:51:29 +0200 > Mike Rapoport <mike.rapoport@gmail.com> wrote: > > > git grep -in slob still gives a couple of matches. I've dropped the > > irrelevant ones it it left me with these: > > > > CREDITS:14:D: SLOB slab allocator > > kernel/trace/ring_buffer.c:358: * Also stolen from mm/slob.c. Thanks to Mathieu Desnoyers for pointing > > mm/Kconfig:251: SLOB allocator and is not recommended for systems with more than > > mm/Makefile:25:KCOV_INSTRUMENT_slob.o := n > > > > Except the comment in kernel/trace/ring_buffer.c all are trivial. > > > > As for the comment in ring_buffer.c, it looks completely irrelevant at this > > point. > > > > @Steve? > > You want me to remember something I wrote almost 15 years ago? I just wanted to make sure you don't have a problem with removing this comment :) > I think I understand that comment as much as you do. Yeah, that was when > I was still learning to write comments for my older self to understand, > and I failed miserably! > > But git history comes to the rescue. The commit that added that comment was: > > ed56829cb3195 ("ring_buffer: reset buffer page when freeing") > > This was at a time when it was suggested to me to use the struct page > directly in the ring buffer and where we could do fun "tricks" for > "performance". (I was never really for this, but I wasn't going to argue). > > And the code in question then had: > > /* > * Also stolen from mm/slob.c. Thanks to Mathieu Desnoyers for pointing > * this issue out. > */ > static inline void free_buffer_page(struct buffer_page *bpage) > { > reset_page_mapcount(&bpage->page); > bpage->page.mapping = NULL; > __free_page(&bpage->page); > } > > > But looking at commit: e4c2ce82ca27 ("ring_buffer: allocate buffer page > pointer") > > It was finally decided that method was not safe, and we should not be using > struct page but just allocate an actual page (much safer!). > > I never got rid of the comment, which was more about that > "reset_page_mapcount()", and should have been deleted back then. Yeah, I did the same analysis, just was too lazy to post it. > Just remove that comment. And you could even add: > > Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org> > Fixes: e4c2ce82ca27 ("ring_buffer: allocate buffer page pointer") > > -- Steve
On Mon, Mar 13, 2023 at 05:36:44PM +0100, Vlastimil Babka wrote: > > git grep -in slob still gives a couple of matches. I've dropped the > > irrelevant ones it it left me with these: I see an #ifdef in security/tomoyo/common.h which I guess is not really relevant? And certainly not harmful in practice. Thought might be nice to eliminate the last reference to CONFIG_SLOB in the kernel :)
On 3/14/23 23:10, Lorenzo Stoakes wrote: > On Mon, Mar 13, 2023 at 05:36:44PM +0100, Vlastimil Babka wrote: >> > git grep -in slob still gives a couple of matches. I've dropped the >> > irrelevant ones it it left me with these: > > I see an #ifdef in security/tomoyo/common.h which I guess is not really > relevant? And certainly not harmful in practice. Thought might be nice to > eliminate the last reference to CONFIG_SLOB in the kernel :) Yeah I wrote in the cover letter the tomoyo change is already going through tomoyo tree. And based on Jakub's feedback the skbuff change will be also posted separately.
On 3/13/23 17:31, Steven Rostedt wrote: > Just remove that comment. And you could even add: > > Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org> > Fixes: e4c2ce82ca27 ("ring_buffer: allocate buffer page pointer") Thanks for the analysis. Want to take the following patch to your tree or should I make it part of the series? ----8<---- From 297a8c8fda98dc5499cfe0eac6ffabfb19d1b70f Mon Sep 17 00:00:00 2001 From: Vlastimil Babka <vbabka@suse.cz> Date: Wed, 15 Mar 2023 14:45:15 +0100 Subject: [PATCH] ring-buffer: remove obsolete comment for free_buffer_page() The comment refers to mm/slob.o which is being removed. It comes from commit ed56829cb319 ("ring_buffer: reset buffer page when freeing") and according to Steven the borrowed code was a page mapcount and mapping reset, which was later removed by commit e4c2ce82ca27 ("ring_buffer: allocate buffer page pointer"). Thus the comment is not accurate anyway, remove it. Reported-by: Mike Rapoport <mike.rapoport@gmail.com> Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org> Fixes: e4c2ce82ca27 ("ring_buffer: allocate buffer page pointer") Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- kernel/trace/ring_buffer.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c index af50d931b020..c6f47b6cfd5f 100644 --- a/kernel/trace/ring_buffer.c +++ b/kernel/trace/ring_buffer.c @@ -354,10 +354,6 @@ static void rb_init_page(struct buffer_data_page *bpage) local_set(&bpage->commit, 0); } -/* - * Also stolen from mm/slob.c. Thanks to Mathieu Desnoyers for pointing - * this issue out. - */ static void free_buffer_page(struct buffer_page *bpage) { free_page((unsigned long)bpage->page);
On Wed, 15 Mar 2023 14:53:14 +0100 Vlastimil Babka <vbabka@suse.cz> wrote: > On 3/13/23 17:31, Steven Rostedt wrote: > > Just remove that comment. And you could even add: > > > > Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > Fixes: e4c2ce82ca27 ("ring_buffer: allocate buffer page pointer") > > Thanks for the analysis. Want to take the following patch to your tree or > should I make it part of the series? I can take it if you send it as a proper patch and Cc linux-trace-kernel@vger.kernel.org. I'm guessing it's not required for stable. -- Steve
On 3/15/23 15:20, Steven Rostedt wrote: > On Wed, 15 Mar 2023 14:53:14 +0100 > Vlastimil Babka <vbabka@suse.cz> wrote: > >> On 3/13/23 17:31, Steven Rostedt wrote: >> > Just remove that comment. And you could even add: >> > >> > Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org> >> > Fixes: e4c2ce82ca27 ("ring_buffer: allocate buffer page pointer") >> >> Thanks for the analysis. Want to take the following patch to your tree or >> should I make it part of the series? > > I can take it if you send it as a proper patch and Cc > linux-trace-kernel@vger.kernel.org. OK, will do. > I'm guessing it's not required for stable. No, but maybe AUTOSEL will pick it anyway as it has Fixes: but that's their problem ;) > -- Steve