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 <mdo...@bitdefender.com>
>
> 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@lists.xen.org
http://lists.xen.org/xen-devel
