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

[Mihai DonÈu <mdontu@xxxxxxxxxxxxxxx>]

On Friday 05 December 2014 12:09:02 Jan Beulich wrote:
> >>> 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.
> 

Thank you for your review Jan. I'll follow up with an update soon, as
well as a patch for __warn() (and __bug() while I'm at it).

-- 
Mihai DONÈU

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