All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm, slab, slub: clear the slab_cache field when freeing page
@ 2020-12-10 16:00 Vlastimil Babka
  2020-12-10 23:46   ` David Rientjes
  2020-12-11  1:17   ` Joonsoo Kim
  0 siblings, 2 replies; 5+ messages in thread
From: Vlastimil Babka @ 2020-12-10 16:00 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, Christoph Lameter, Pekka Enberg,
	David Rientjes, Joonsoo Kim, Matthew Wilcox, Vlastimil Babka

The page allocator expects that page->mapping is NULL for a page being freed.
SLAB and SLUB use the slab_cache field which is in union with mapping, but
before freeing the page, the field is referenced with the "mapping" name when
set to NULL.

It's IMHO more correct (albeit functionally the same) to use the slab_cache
name as that's the field we use in SL*B, and document why we clear it in a
comment (we don't clear fields such as s_mem or freelist, as page allocator
doesn't care about those). While using the 'mapping' name would automagically
keep the code correct if the unions in struct page changed, such changes should
be done consciously and needed changes evaluated - the comment should help with
that.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slab.c | 3 ++-
 mm/slub.c | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 72b6743bdccf..b667f03095f1 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1399,7 +1399,8 @@ static void kmem_freepages(struct kmem_cache *cachep, struct page *page)
 	__ClearPageSlabPfmemalloc(page);
 	__ClearPageSlab(page);
 	page_mapcount_reset(page);
-	page->mapping = NULL;
+	/* In union with page->mapping where page allocator expects NULL */
+	page->slab_cache = NULL;
 
 	if (current->reclaim_state)
 		current->reclaim_state->reclaimed_slab += 1 << order;
diff --git a/mm/slub.c b/mm/slub.c
index d3406ef65863..81c22f4c7e63 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1836,8 +1836,8 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
 
 	__ClearPageSlabPfmemalloc(page);
 	__ClearPageSlab(page);
-
-	page->mapping = NULL;
+	/* In union with page->mapping where page allocator expects NULL */
+	page->slab_cache = NULL;
 	if (current->reclaim_state)
 		current->reclaim_state->reclaimed_slab += pages;
 	unaccount_slab_page(page, order, s);
-- 
2.29.2


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

* Re: [PATCH] mm, slab, slub: clear the slab_cache field when freeing page
  2020-12-10 16:00 [PATCH] mm, slab, slub: clear the slab_cache field when freeing page Vlastimil Babka
@ 2020-12-10 23:46   ` David Rientjes
  2020-12-11  1:17   ` Joonsoo Kim
  1 sibling, 0 replies; 5+ messages in thread
From: David Rientjes @ 2020-12-10 23:46 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, Christoph Lameter,
	Pekka Enberg, Joonsoo Kim, Matthew Wilcox

On Thu, 10 Dec 2020, Vlastimil Babka wrote:

> The page allocator expects that page->mapping is NULL for a page being freed.
> SLAB and SLUB use the slab_cache field which is in union with mapping, but
> before freeing the page, the field is referenced with the "mapping" name when
> set to NULL.
> 
> It's IMHO more correct (albeit functionally the same) to use the slab_cache
> name as that's the field we use in SL*B, and document why we clear it in a
> comment (we don't clear fields such as s_mem or freelist, as page allocator
> doesn't care about those). While using the 'mapping' name would automagically
> keep the code correct if the unions in struct page changed, such changes should
> be done consciously and needed changes evaluated - the comment should help with
> that.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: David Rientjes <rientjes@google.com>

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

* Re: [PATCH] mm, slab, slub: clear the slab_cache field when freeing page
@ 2020-12-10 23:46   ` David Rientjes
  0 siblings, 0 replies; 5+ messages in thread
From: David Rientjes @ 2020-12-10 23:46 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, linux-mm, linux-kernel, Christoph Lameter,
	Pekka Enberg, Joonsoo Kim, Matthew Wilcox

On Thu, 10 Dec 2020, Vlastimil Babka wrote:

> The page allocator expects that page->mapping is NULL for a page being freed.
> SLAB and SLUB use the slab_cache field which is in union with mapping, but
> before freeing the page, the field is referenced with the "mapping" name when
> set to NULL.
> 
> It's IMHO more correct (albeit functionally the same) to use the slab_cache
> name as that's the field we use in SL*B, and document why we clear it in a
> comment (we don't clear fields such as s_mem or freelist, as page allocator
> doesn't care about those). While using the 'mapping' name would automagically
> keep the code correct if the unions in struct page changed, such changes should
> be done consciously and needed changes evaluated - the comment should help with
> that.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: David Rientjes <rientjes@google.com>


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

* Re: [PATCH] mm, slab, slub: clear the slab_cache field when freeing page
  2020-12-10 16:00 [PATCH] mm, slab, slub: clear the slab_cache field when freeing page Vlastimil Babka
@ 2020-12-11  1:17   ` Joonsoo Kim
  2020-12-11  1:17   ` Joonsoo Kim
  1 sibling, 0 replies; 5+ messages in thread
From: Joonsoo Kim @ 2020-12-11  1:17 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Linux Memory Management List, LKML,
	Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Matthew Wilcox

2020년 12월 11일 (금) 오전 1:00, Vlastimil Babka <vbabka@suse.cz>님이 작성:
>
> The page allocator expects that page->mapping is NULL for a page being freed.
> SLAB and SLUB use the slab_cache field which is in union with mapping, but
> before freeing the page, the field is referenced with the "mapping" name when
> set to NULL.
>
> It's IMHO more correct (albeit functionally the same) to use the slab_cache
> name as that's the field we use in SL*B, and document why we clear it in a
> comment (we don't clear fields such as s_mem or freelist, as page allocator
> doesn't care about those). While using the 'mapping' name would automagically
> keep the code correct if the unions in struct page changed, such changes should
> be done consciously and needed changes evaluated - the comment should help with
> that.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

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

* Re: [PATCH] mm, slab, slub: clear the slab_cache field when freeing page
@ 2020-12-11  1:17   ` Joonsoo Kim
  0 siblings, 0 replies; 5+ messages in thread
From: Joonsoo Kim @ 2020-12-11  1:17 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Linux Memory Management List, LKML,
	Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Matthew Wilcox

2020년 12월 11일 (금) 오전 1:00, Vlastimil Babka <vbabka@suse.cz>님이 작성:
>
> The page allocator expects that page->mapping is NULL for a page being freed.
> SLAB and SLUB use the slab_cache field which is in union with mapping, but
> before freeing the page, the field is referenced with the "mapping" name when
> set to NULL.
>
> It's IMHO more correct (albeit functionally the same) to use the slab_cache
> name as that's the field we use in SL*B, and document why we clear it in a
> comment (we don't clear fields such as s_mem or freelist, as page allocator
> doesn't care about those). While using the 'mapping' name would automagically
> keep the code correct if the unions in struct page changed, such changes should
> be done consciously and needed changes evaluated - the comment should help with
> that.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>


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

end of thread, other threads:[~2020-12-11  1:19 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-10 16:00 [PATCH] mm, slab, slub: clear the slab_cache field when freeing page Vlastimil Babka
2020-12-10 23:46 ` David Rientjes
2020-12-10 23:46   ` David Rientjes
2020-12-11  1:17 ` Joonsoo Kim
2020-12-11  1:17   ` Joonsoo Kim

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.