[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: "Jan Beulich" <JBeulich@xxxxxxxx>
  • From: Mihai DonÈu <mdontu@xxxxxxxxxxxxxxx>
  • Date: Mon, 8 Dec 2014 01:58:01 +0200
  • Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Comment: DomainKeys? See http://domainkeys.sourceforge.net/
  • Delivery-date: Sun, 07 Dec 2014 23:58:44 +0000
  • Domainkey-signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=bitdefender.com; b=ZOZhDtSPbg2aXL8EVY4umU6Ajd5mfQzRrvNzR8z4CGdyH9Cp4+qKBwT8518et+yqLMVCPA0LNVswhg7bjZnFzfLpJUR8XYLW17qIgleaV/cdTMLQRT6JUbbLgxvCNcylrMyjQhV3jGVafuUfwuFtU0cAx9LK8e4lYjJ/NVAKkGvdUHGxuFsF5l3D8SO5osjta4rLveeSafsL4IpIze7uS10cmoiKNySjxpVsiXIlqED91bI8z/PIHboBd8vZvLZAfbLzkTdOfqlLWrfpBFoV3BBvuGirIBicufI9aDgoIRnJKV5RLXva5pk0TuuSLoucXszP8jbLuEVG14OX2QKQPg==; h=Received:Received:Received:Received:Received:Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References:Organization:MIME-Version:Content-Type:Content-Transfer-Encoding:X-BitDefender-Scanner:X-BitDefender-Spam:X-BitDefender-SpamStamp:X-BitDefender-CF-Stamp;
  • List-id: Xen developer discussion <xen-devel.lists.xen.org>

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.