lkml.org 
[lkml]   [2008]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [v2.6.26] what's brewing in x86.git for v2.6.26
    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..



    \
     
     \ /
      Last update: 2008-04-17 21:47    [W:4.114 / U:0.532 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site