* Re: [RFC][PATCH -tip] kmemcheck: add hooks for the page allocator
2008-11-25 17:02 [RFC][PATCH -tip] kmemcheck: add hooks for the page allocator Vegard Nossum
@ 2008-11-25 20:24 ` Dave Hansen
0 siblings, 0 replies; 2+ messages in thread
From: Dave Hansen @ 2008-11-25 20:24 UTC (permalink / raw)
To: Vegard Nossum
Cc: Ingo Molnar, Pekka Enberg, Andrew Morton, linux-kernel, linux-mm
On Tue, 2008-11-25 at 18:02 +0100, Vegard Nossum wrote:
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -23,6 +23,7 @@
> #include <linux/bootmem.h>
> #include <linux/compiler.h>
> #include <linux/kernel.h>
> +#include <linux/kmemcheck.h>
> #include <linux/module.h>
> #include <linux/suspend.h>
> #include <linux/pagevec.h>
> @@ -511,6 +512,9 @@ static void __free_pages_ok(struct page *page, unsigned int order)
> int i;
> int reserved = 0;
>
> + if (kmemcheck_page_is_tracked(page))
> + kmemcheck_free_shadow(page, order);
> +
> for (i = 0 ; i < (1 << order) ; ++i)
> reserved += free_pages_check(page + i);
> if (reserved)
> @@ -974,6 +978,9 @@ static void free_hot_cold_page(struct page *page, int cold)
> struct per_cpu_pages *pcp;
> unsigned long flags;
>
> + if (kmemcheck_page_is_tracked(page))
> + kmemcheck_free_shadow(page, 0);
I think it would be best to just integrate the
kmemcheck_page_is_tracked() inside of kmemcheck_free_shadow(). It will
be shorter and less error-prone.
> if (PageAnon(page))
> page->mapping = NULL;
> if (free_pages_check(page))
> @@ -1637,7 +1644,28 @@ nopage:
> dump_stack();
> show_mem();
> }
> + return page;
> got_pg:
> + if (kmemcheck_enabled
> + && !(gfp_mask & (__GFP_HIGHMEM | __GFP_IO | __GFP_NOTRACK)))
> + {
> + int nr_pages = 1 << order;
> +
> + /*
> + * NOTE: We choose to track GFP_ZERO pages too; in fact, they
> + * can become uninitialized by copying uninitialized memory
> + * into them.
> + */
> +
> + /* XXX: Can use zone->node for node? */
> + kmemcheck_alloc_shadow(page, order, gfp_mask, -1);
> +
> + if (gfp_mask & __GFP_ZERO)
> + kmemcheck_mark_initialized_pages(page, nr_pages);
> + else
> + kmemcheck_mark_uninitialized_pages(page, nr_pages);
> + }
> +
> return page;
> }
That's too much gunk to add to a core function like
free_hot_cold_page(). Can you please break this out into a function?
Does 'kmemcheck_enabled' get compiled down to a constant if !
CONFIG_KMEMCHECK? If not, it should.
> EXPORT_SYMBOL(__alloc_pages_internal);
> diff --git a/mm/slab.c b/mm/slab.c
> index 37deade..286c6a6 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -1600,7 +1600,7 @@ static void *kmem_getpages(struct kmem_cache *cachep, gfp_t flags, int nodeid)
> if (cachep->flags & SLAB_RECLAIM_ACCOUNT)
> flags |= __GFP_RECLAIMABLE;
>
> - page = alloc_pages_node(nodeid, flags, cachep->gfporder);
> + page = alloc_pages_node(nodeid, flags & ~__GFP_NOTRACK, cachep->gfporder);
> if (!page)
> return NULL;
>
> @@ -1614,8 +1614,14 @@ static void *kmem_getpages(struct kmem_cache *cachep, gfp_t flags, int nodeid)
> for (i = 0; i < nr_pages; i++)
> __SetPageSlab(page + i);
>
> - if (kmemcheck_enabled && !(cachep->flags & SLAB_NOTRACK))
> - kmemcheck_alloc_shadow(cachep, flags, nodeid, page, cachep->gfporder);
> + if (kmemcheck_enabled && !(cachep->flags & SLAB_NOTRACK)) {
> + kmemcheck_alloc_shadow(page, cachep->gfporder, flags, nodeid);
> +
> + if (cachep->ctor)
> + kmemcheck_mark_uninitialized_pages(page, nr_pages);
> + else
> + kmemcheck_mark_unallocated_pages(page, nr_pages);
> + }
>
> return page_address(page);
> }
> @@ -1630,7 +1636,7 @@ static void kmem_freepages(struct kmem_cache *cachep, void *addr)
> const unsigned long nr_freed = i;
>
> if (kmemcheck_page_is_tracked(page))
> - kmemcheck_free_shadow(cachep, page, cachep->gfporder);
> + kmemcheck_free_shadow(page, cachep->gfporder);
>
> if (cachep->flags & SLAB_RECLAIM_ACCOUNT)
> sub_zone_page_state(page_zone(page),
> diff --git a/mm/slub.c b/mm/slub.c
> index adcb5e3..eb9855f 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1064,6 +1064,8 @@ static inline struct page *alloc_slab_page(gfp_t flags, int node,
> {
> int order = oo_order(oo);
>
> + flags |= __GFP_NOTRACK;
> +
> if (node == -1)
> return alloc_pages(flags, order);
> else
> @@ -1095,7 +1097,18 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
> if (kmemcheck_enabled
> && !(s->flags & (SLAB_NOTRACK | DEBUG_DEFAULT_FLAGS)))
> {
> - kmemcheck_alloc_shadow(s, flags, node, page, compound_order(page));
> + int pages = 1 << oo_order(oo);
> +
> + kmemcheck_alloc_shadow(page, oo_order(oo), flags, node);
> +
> + /*
> + * Objects from caches that have a constructor don't get
> + * cleared when they're allocated, so we need to do it here.
> + */
> + if (s->ctor)
> + kmemcheck_mark_uninitialized_pages(page, pages);
> + else
> + kmemcheck_mark_unallocated_pages(page, pages);
> }
>
> page->objects = oo_objects(oo);
> @@ -1172,7 +1185,7 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
> }
>
> if (kmemcheck_page_is_tracked(page))
> - kmemcheck_free_shadow(s, page, compound_order(page));
> + kmemcheck_free_shadow(page, compound_order(page));
>
> mod_zone_page_state(page_zone(page),
> (s->flags & SLAB_RECLAIM_ACCOUNT) ?
> @@ -2679,8 +2692,8 @@ EXPORT_SYMBOL(__kmalloc);
>
> static void *kmalloc_large_node(size_t size, gfp_t flags, int node)
> {
> - struct page *page = alloc_pages_node(node, flags | __GFP_COMP,
> - get_order(size));
> + struct page *page = alloc_pages_node(node,
> + flags | __GFP_COMP | __GFP_NOTRACK, get_order(size));
>
> if (page)
> return page_address(page);
Adding the new flag made that pretty ugly. Can you fix it up?
-- Dave
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 2+ messages in thread