Re: [Xen-devel] [PATCH v4] xmalloc: add support for checking the pool integrity

[Mihai DonÈu <mdontu@xxxxxxxxxxxxxxx>]

On Wednesday 07 January 2015 19:20:38 Andrew Cooper wrote:
> On 16/12/14 19:33, Mihai DonÈu wrote:
> > Implemented xmem_pool_check(), xmem_pool_check_locked() and
> > xmem_pool_check_unlocked() to verity the integrity of the TLSF matrix.
> >
> > Signed-off-by: Mihai DonÈu <mdontu@xxxxxxxxxxxxxxx>
> 
> This review supersedes (and is adjusted accordingly for) the two
> discussion threads which happened off my first review.
> 
> >
> > ---
> > Changes since v3:
> >  - try harder to respect the 80 column limit
> >  - use 'unsigned int' instead of 'int' where possible
> >  - made the logged messages even shorter
> >  - dropped the use of goto (didn't really make sense)
> >
> > Changes since v2:
> >  - print the name of the corrupted pool
> >  - adjusted the messages to better fit within 80 columns
> >  - minor style change (a ? a : b -> a ?: b)
> >
> > Changes since v1:
> >  - fixed the codingstyle
> >  - swaped _locked/_unlocked naming
> >  - reworked __xmem_pool_check_locked() a bit
> >  - used bool_t where appropriate
> >  - made xmem_pool_check() take a pool argument which can be NULL
> > ---
> >  xen/common/xmalloc_tlsf.c | 121 
> > +++++++++++++++++++++++++++++++++++++++++++++-
> >  xen/include/xen/xmalloc.h |   7 +++
> >  2 files changed, 126 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> > index a5769c9..eca4f1c 100644
> > --- a/xen/common/xmalloc_tlsf.c
> > +++ b/xen/common/xmalloc_tlsf.c
> > @@ -120,9 +120,122 @@ struct xmem_pool {
> >      char name[MAX_POOL_NAME_LEN];
> >  };
> >  
> > +static struct xmem_pool *xenpool;
> > +
> > +static inline void MAPPING_INSERT(unsigned long r, int *fl, int *sl);
> > +
> >  /*
> >   * Helping functions
> >   */
> > +#ifndef NDEBUG
> > +static bool_t xmem_pool_check_size(const struct xmem_pool *pool,
> > +                                   int fl, int sl)
> > +{
> > +    const struct bhdr *b = pool->matrix[fl][sl];
> > +
> > +    while ( b )
> > +    {
> > +        int __fl;
> > +        int __sl;
> > +
> > +        MAPPING_INSERT(b->size, &__fl, &__sl);
> > +        if ( __fl != fl || __sl != sl )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "xmem_pool: %s: misplaced block %p:%u ({%d,%d} -> 
> > {%d,%d})\n",
> > +                   pool->name, b, b->size, fl, sl, __fl, __sl);
> 
> Is it liable to get confusing with a hex number and a decimal number
> combined with just a colon?
> 
> Is b even a useful pointer to print? You have the pool name, indicies
> and size which seem to be the salient information.
> 

I think you are right. I went ahead and printed all information
available just because I had access to it thinking someone other than
myself might find it valuable.

> > +            return 0;
> > +        }
> > +        b = b->ptr.free_ptr.next;
> > +    }
> > +    return 1;
> > +}
> > +
> > +/*
> > + * This function must be called from a context where pool->lock is
> > + * already acquired.
> > + *
> > + * Returns true if the pool has been corrupted, false otherwise
> > + */
> > +#define xmem_pool_check_locked(pool) \
> > +    __xmem_pool_check_locked(__FILE__, __LINE__, pool)
> > +static bool_t __xmem_pool_check_locked(const char *file, int line,
> > +                                       const struct xmem_pool *pool)
> > +{
> > +    unsigned int i;
> > +    static bool_t once = 1;
> > +
> > +    if ( !once )
> > +        return 1;
> > +    for ( i = 0; i < REAL_FLI; i++ )
> > +    {
> > +        int fl = (pool->fl_bitmap & (1 << i)) ? i : -1;
> > +
> > +        if ( fl >= 0 )
> > +        {
> > +            unsigned int j;
> > +
> > +            if ( !pool->sl_bitmap[fl] )
> > +            {
> > +                printk(XENLOG_ERR
> > +                       "xmem_pool: %s: TLSF bitmap corrupt (!empty FL, 
> > empty SL)\n",
> > +                       pool->name);
> > +                __warn(file, line);
> > +                once = 0;
> > +                break;
> > +            }
> > +            for ( j = 0; j < MAX_SLI; j++ )
> > +            {
> > +                int sl = (pool->sl_bitmap[fl] & (1 << j)) ? j : -1;
> > +
> > +                if ( sl < 0 )
> > +                    continue;
> > +                if ( !pool->matrix[fl][sl] )
> > +                {
> > +                    printk(XENLOG_ERR
> > +                           "xmem_pool: %s: TLSF bitmap corrupt 
> > (!matrix[%d][%d])\n",
> > +                           pool->name, fl, sl);
> > +                    __warn(file, line);
> > +                    once = 0;
> > +                    break;
> > +                }
> > +                if ( !xmem_pool_check_size(pool, fl, sl) )
> > +                {
> > +                    printk(XENLOG_ERR "xmem_pool: %s: TLSF chunk matrix 
> > corrupt\n",
> > +                           pool->name);
> > +                    __warn(file, line);
> > +                    once = 0;
> > +                    break;
> > +                }
> > +            }
> > +            if ( !once )
> > +                break;
> > +        }
> > +    }
> > +    return !once;
> > +}
> 
> I want to remove __bug() and __warn() because they are currently unused
> in the code, and are a cludge on ARM (lack of working
> run_in_exception_handler()).  You can use BUG()/WARN() instead, which
> will work as expected ARM.
> 
> However, once memory corruption has been detected, all bets are off
> regarding correct functioning of the host.  I would suggest printing as
> much error information as possible and using BUG() to bring Xen down. 
> This allows you to remove 'once' (dubious being static, yet applying to
> arbitrary xmem pools), and to change the function to be void.
> 

I used the 'once' variable to keep the code from spamming dmesg. I
figured once one sees the message, he/she knows things are about to go
downhill.

I will use BUG().

> > +
> > +#define xmem_pool_check_unlocked(pool) \
> > +    __xmem_pool_check_unlocked(__FILE__, __LINE__, pool)
> > +static bool_t __xmem_pool_check_unlocked(const char *file, int line,
> > +                                         struct xmem_pool *pool)
> > +{
> > +    bool_t oops;
> > +
> > +    spin_lock(&pool->lock);
> > +    oops = __xmem_pool_check_locked(file, line, pool);
> > +    spin_unlock(&pool->lock);
> > +    return oops;
> > +}
> > +
> > +bool_t __xmem_pool_check(const char *file, int line, struct xmem_pool 
> > *pool)
> > +{
> > +    return __xmem_pool_check_unlocked(file, line, pool ?: xenpool);
> > +}
> > +#else
> > +#define xmem_pool_check_locked(pool) ((void)(pool))
> > +#define xmem_pool_check_unlocked(pool) ((void)(pool))
> > +#endif
> >  
> >  /**
> >   * Returns indexes (fl, sl) of the list used to serve request of size r
> > @@ -381,6 +494,8 @@ void *xmem_pool_alloc(unsigned long size, struct 
> > xmem_pool *pool)
> >      int fl, sl;
> >      unsigned long tmp_size;
> >  
> > +    xmem_pool_check_unlocked(pool);
> > +
> >      if ( pool->init_region == NULL )
> >      {
> >          if ( (region = pool->get_mem(pool->init_size)) == NULL )
> > @@ -442,11 +557,13 @@ void *xmem_pool_alloc(unsigned long size, struct 
> > xmem_pool *pool)
> >  
> >      pool->used_size += (b->size & BLOCK_SIZE_MASK) + BHDR_OVERHEAD;
> >  
> > +    xmem_pool_check_locked(pool);
> >      spin_unlock(&pool->lock);
> >      return (void *)b->ptr.buffer;
> >  
> >      /* Failed alloc */
> >   out_locked:
> > +    xmem_pool_check_locked(pool);
> >      spin_unlock(&pool->lock);
> >  
> >   out:
> > @@ -464,6 +581,7 @@ void xmem_pool_free(void *ptr, struct xmem_pool *pool)
> >      b = (struct bhdr *)((char *) ptr - BHDR_OVERHEAD);
> >  
> >      spin_lock(&pool->lock);
> > +    xmem_pool_check_locked(pool);
> >      b->size |= FREE_BLOCK;
> >      pool->used_size -= (b->size & BLOCK_SIZE_MASK) + BHDR_OVERHEAD;
> >      b->ptr.free_ptr = (struct free_ptr) { NULL, NULL};
> > @@ -500,6 +618,7 @@ void xmem_pool_free(void *ptr, struct xmem_pool *pool)
> >      tmp_b->size |= PREV_FREE;
> >      tmp_b->prev_hdr = b;
> >   out:
> > +    xmem_pool_check_locked(pool);
> >      spin_unlock(&pool->lock);
> >  }
> >  
> > @@ -512,8 +631,6 @@ int xmem_pool_maxalloc(struct xmem_pool *pool)
> >   * Glue for xmalloc().
> >   */
> >  
> > -static struct xmem_pool *xenpool;
> > -
> >  static void *xmalloc_pool_get(unsigned long size)
> >  {
> >      ASSERT(size == PAGE_SIZE);
> > diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
> > index 24a99ac..ad48930 100644
> > --- a/xen/include/xen/xmalloc.h
> > +++ b/xen/include/xen/xmalloc.h
> > @@ -123,4 +123,11 @@ unsigned long xmem_pool_get_used_size(struct xmem_pool 
> > *pool);
> >   */
> >  unsigned long xmem_pool_get_total_size(struct xmem_pool *pool);
> >  
> > +#ifndef NDEBUG
> > +#define xmem_pool_check(pool) __xmem_pool_check(__FILE__, __LINE__, pool)
> > +bool_t __xmem_pool_check(const char *file, int line, struct xmem_pool 
> > *pool);
> > +#else
> > +#define xmem_pool_check(pool) ((void)0)
> 
> This needs to be ((void)(pool)) to evaluate the potential side effects.
> 
> 
> What are the overheads of pool consistency checking?  It looks to be
> moderately high, given a full inspection of the matrix on each
> allocation and free.  Would it be possible to have one easier-to-alter
> compile time knob so the default can be overridden in a single location?

In my tests, I have not noticed a slowdown, or maybe Haswells these
days are that good.

> Perhaps something like:
> 
> #ifndef NDEBUG
> #define XMEM_POOL_CHECKS
> #endif
> 
> #ifdef XMEM_POOL_CHECKS
> ...
> #else
> ...
> #endif

Sure thing. My interest is in only having an upstream method to detect
these, other than a patch somewhere on my disk.

Thank you,

-- 
Mihai DONÈU

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel