Re: [v2.6.26] what's brewing in x86.git for v2.6.26

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 17 Apr 2008 20:47:06 +0200
"Vegard Nossum" <vegard.nossum@gmail.com> wrote:

> On Thu, Apr 17, 2008 at 11:36 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Thu, 17 Apr 2008 11:30:25 +0200 Ingo Molnar <mingo@elte.hu> wrote:
> >  > you mean kmemcheck? Yes, that's planned. We've been working 4 months
> >  > non-stop on kmemcheck to make it mergeable and usable, it's at version 7
> >  > right now, and it caught a handful of real bugs already (such as
> >  > 63a7138671c - unfortunately not credited in the log to kmemcheck). But
> >  > because it touches SLUB (because it has to - and they are acked by
> >  > Pekka) i never had the chance to move it into the for-akpm branch.
> >
> >  Does it really really really need to consume one of our few remaining page
> >  flags?  We'll be in a mess when we run out.
> 
> Actually it doesn't. I attach a patch which gets rid of the page flag,
> and we rely instead on the PTE flag for page-trackedness.
> 
> The reason we didn't do this at once is that the making of kmemcheck
> has been pretty much my first introduction to SLUB, x86, page flags,
> etc., and the actual semantics of the various introduced flags have
> varied since the first version of kmemcheck. At this point, the struct
> page flags weren't actually needed anymore, but they were convenient.
> 
> My apologies for not inlining the patch -- I don't have a mail client
> that won't mess up whitespace. It can also be downloaded at:
> http://folk.uio.no/vegardno/linux/0001-kmemcheck-remove-use-of-tracked-page-flag.patch
> 
> The patch has received minimal amount of testing, but I've
> double-checked the logic. It boots fine on my laptop, boot log at:
> http://folk.uio.no/vegardno/linux/kmemcheck-20080417.txt
> 
> Ingo, will you take this for some additional testing?
> 

If you're OK with doing it this way then it would be preferable.

> diff --git a/arch/x86/kernel/kmemcheck.c b/arch/x86/kernel/kmemcheck.c
> index 16dce10..d82f35d 100644
> --- a/arch/x86/kernel/kmemcheck.c
> +++ b/arch/x86/kernel/kmemcheck.c
> @@ -233,12 +233,27 @@ param_kmemcheck(char *str)
>  	if (!str)
>  		return -EINVAL;
>  
> -	sscanf("%d", str, &kmemcheck_enabled);
> +	sscanf(str, "%d", &kmemcheck_enabled);
>  	return 0;
>  }

whoops.  Note to Ingo: unrelated bugfix in there.

>  early_param("kmemcheck", param_kmemcheck);

kmemcheck= is documented in at least three places, which is nice, but it
isn't mentioned in the place where we document kernel-parameters:
Documentation/kernel-parameters.txt.  A brief section there which directs
the user to the extended docs would be fine.

early_param() is unusual - we normally use __setup().  I assume there's a
reason for using early_param(), but that reason cannot be discerned from
reading the code.  A /*comment*/ is the way to fix that.

> +static pte_t *
> +address_get_pte(unsigned int address)

This is not the preferred way of laying out function declarations but I've
basically given up on this one.

> +{
> +	pte_t *pte;
> +	int level;
> +
> +	pte = lookup_address(address, &level);
> +	if (!pte)
> +		return NULL;
> +	if (!pte_hidden(*pte))
> +		return NULL;
> +
> +	return pte;
> +}
> +
>  /*
>   * Return the shadow address for the given address. Returns NULL if the
>   * address is not tracked.
> @@ -249,88 +264,53 @@ early_param("kmemcheck", param_kmemcheck);
>  static void *
>  address_get_shadow(unsigned long address)
>  {
> +	pte_t *pte;
>  	struct page *page;
>  	struct page *head;
>  
>  	if (!virt_addr_valid(address))
>  		return NULL;
>  
> +	pte = address_get_pte(address);
> +	if (!pte)
> +		return NULL;
> +
>  	/* The accessed page */
>  	page = virt_to_page(address);
> -	if (!PageCompound(page))
> -		return NULL;
> +	BUG_ON(!PageCompound(page));
>  
>  	/* The head page */
>  	head = compound_head(page);
> -	if (!PageTracked(head))
> -		return NULL;
> +	BUG_ON(compound_order(head) == 0);
>  
>  	return (void *) address + (PAGE_SIZE << (compound_order(head) - 1));
>  }

	(void *)address

is more common, but I'm close to giving up on that too.

>  static int
> -show_addr(uint32_t addr)
> +show_addr(uint32_t address)

u32 is preferred, but ditto.

>  {
>  	pte_t *pte;
> -	int level;
> -
> -	if (!address_get_shadow(addr))
> -		return 0;
> -
> -	pte = lookup_address(addr, &level);
> -	BUG_ON(!pte);
> -
> -	if (level != PG_LEVEL_4K)
> -		return 0;
> -
> -	set_pte(pte, __pte(pte_val(*pte) | _PAGE_PRESENT));
> -	__flush_tlb_one(addr);
> -	return 1;
> -}
>
> ...
>
> --- a/include/linux/kmemcheck.h
> +++ b/include/linux/kmemcheck.h
> @@ -9,6 +9,8 @@ void kmemcheck_init(void);
>  void kmemcheck_show_pages(struct page *p, unsigned int n);
>  void kmemcheck_hide_pages(struct page *p, unsigned int n);
>  
> +bool kmemcheck_page_is_tracked(struct page *p);
> +
>  void kmemcheck_mark_unallocated(void *address, unsigned int n);
>  void kmemcheck_mark_uninitialized(void *address, unsigned int n);
>  void kmemcheck_mark_initialized(void *address, unsigned int n);
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 63f5fd8..3696889 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -89,7 +89,6 @@
>  #define PG_mappedtodisk		16	/* Has blocks allocated on-disk */
>  #define PG_reclaim		17	/* To be reclaimed asap */
>  #define PG_buddy		19	/* Page is free, on buddy lists */
> -#define PG_tracked		20	/* Tracked by kmemcheck */
>  
>  /* PG_readahead is only used for file reads; PG_reclaim is only for writes */
>  #define PG_readahead		PG_reclaim /* Reminder to do async read-ahead */
> @@ -297,10 +296,6 @@ static inline void __ClearPageTail(struct page *page)
>  #define SetPageUncached(page)	set_bit(PG_uncached, &(page)->flags)
>  #define ClearPageUncached(page)	clear_bit(PG_uncached, &(page)->flags)
>  
> -#define PageTracked(page)	test_bit(PG_tracked, &(page)->flags)
> -#define SetPageTracked(page)	set_bit(PG_tracked, &(page)->flags)
> -#define ClearPageTracked(page)	clear_bit(PG_tracked, &(page)->flags)
> -

That's about 15 less rejects I have to fix ;)
 
>  struct page;	/* forward declaration */
>  
> diff --git a/mm/slub.c b/mm/slub.c
> index 9b58979..7a544e6 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1125,7 +1125,7 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
>  		ClearSlabDebug(page);
>  	}
>  
> -	if (PageTracked(page) && !(s->flags & SLAB_NOTRACK)) {
> +	if (kmemcheck_page_is_tracked(page) && !(s->flags & SLAB_NOTRACK)) {
>  		kmemcheck_free_slab(s, page, pages);
>  		return;
>  	}

Perhaps we should get all this code onto the list(s) for re-review.  It's
been a while..