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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.12.14 at 18:01, <mdontu@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -120,9 +120,120 @@ 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 int xmem_pool_check_size(const struct bhdr *b, int fl, int sl)
> +{
> +    while ( b )
> +    {
> +        int __fl;
> +        int __sl;
> +
> +        MAPPING_INSERT(b->size, &__fl, &__sl);
> +        if ( __fl != fl || __sl != sl )
> +        {
> +            printk(XENLOG_ERR "xmem_pool: for block %p size = %u, { fl = %d, 
> sl = %d } should be { fl = %d, sl = %d }\n", b, b->size, fl, sl, __fl, __sl);

Long line. Only the format message alone is allowed to exceed 80
characters.

> +            return 0;
> +        }
> +        b = b->ptr.free_ptr.next;
> +    }
> +    return 1;
> +}
> +
> +/*
> + * This function must be called from a context where pool->lock is
> + * already acquired
> + */
> +#define xmem_pool_check_unlocked(__pool) 
> __xmem_pool_check_unlocked(__FILE__, __LINE__, __pool)

No need for the double underscores on the macro parameter.

> +static int __xmem_pool_check_unlocked(const char *file, int line, const 
> struct xmem_pool *pool)
> +{
> +    int i;
> +    int woops = 0;
> +    static int once = 1;

bool_t

> +
> +    for ( i = 0; i < REAL_FLI; i++ )
> +    {
> +        int fl = ( pool->fl_bitmap & (1 << i) ) ? i : -1;

Bogus spaces inside parentheses.

> +
> +        if ( fl >= 0 )
> +        {
> +            int j;
> +            int bitmap_empty = 1;
> +            int matrix_empty = 1;

For any of the int-s here and above - can they really all become
negative? If not, they ought to be unsigned int or bool_t.

> +
> +            for ( j = 0; j < MAX_SLI; j++ )
> +            {
> +                int sl = ( pool->sl_bitmap[fl] & (1 << j) ) ? j : -1;
> +
> +                if ( sl < 0 )
> +                    continue;
> +
> +                if ( once && !pool->matrix[fl][sl] )
> +                {
> +                    /* The bitmap is corrupted */
> +                    printk(XENLOG_ERR "xmem_pool:%s:%d the TLSF bitmap is 
> corrupted\n", file, line);
> +                    __warn((char *)file, line);

Please constify the first parameter of __warn() instead of adding
fragile casts. I also don't see why file and line need printing twice.

> +static int __xmem_pool_check_locked(const char *file, int line, struct 
> xmem_pool *pool)
> +{
> +    int err;
> +
> +    spin_lock(&pool->lock);
> +    err = __xmem_pool_check_unlocked(file, line, pool);

Inversed naming: The caller here should be _unlocked, and the
callee _locked.

> +#define xmem_pool_check_locked(__pool) do { if ( 0 && (__pool) ); } while (0)
> +#define xmem_pool_check_unlocked(__pool) do { if ( 0 && (__pool) ); } while 
> (0)

((void)(pool)) or at least drop the "0 &&" - after all you _want_ the
macro argument to be evaluated (in order to carry out side effects).

> --- 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() __xmem_pool_check(__FILE__, __LINE__)
> +int __xmem_pool_check(const char *file, int line);
> +#else
> +#define xmem_pool_check() do { if ( 0 ); } while (0)

((void)0)

or

do {} while (0)

Also perhaps __xmem_pool_check() should have a pool parameter,
with NULL meaning the default one.

Jan


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