[PATCH v4] xmalloc: add support for checking the pool integrity

[PATCH v4] xmalloc: add support for checking the pool integrity

[Mihai Donțu]

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@bitdefender.com>

---
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);
+            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;
+}
+
+#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)
+#endif
+
 #endif /* __XMALLOC_H__ */
-- 
2.2.0


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

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

[Andrew Cooper]

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@bitdefender.com>
>
> ---
> 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",

Is it liable to get confusing with a hex number and a decimal number
combined with just a colon?

Is b even useful?  You have the pool name, indicies and size which seem
to be the salient information.

> +                   pool->name, b, b->size, fl, sl, __fl, __sl);
> +            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;

What is this static doing?  Surely corruption corruption in one pool has
no effect on corruption in a separate pool (other than the usual side
effects of general memory corruption, which tend to be bad).

It looks as if it wants to be an extra field in an xmem_pool.

> +
> +    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);

The __warn()s here will currently do a full register dump, as well as
stack dump and stack trace.  It occurs to me that only the stack trace
itself is useful at this point.

__bug and __warn() are currently unused (I am struggling to work out
exactly when they were orphaned; they do not form part of regular
BUG()/WARN() operations, which are handled in do_invalid_op()).  They
are also not fantastically useful as they will always identify
themselves in the printed information, not their caller.  I suspect they
can just be dropped completely.

I suspect you also would be better, and certainly more brief, with
"run_in_exception_handler(show_stack)" instead, which will just print a
stack trace, but nothing more.

> +                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;
> +}
> +
> +#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);

Why should a NULL pool be tolerated here?  This is debug code only, so
we can require and trust that we are called appropriately.

> +}
> +#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. 
However, I think you need to conditionally change __xmem_pool_check()
and move #define xmem_pool_check outside the conditional, to cover
future consumers who might choose to call __xmem_pool_check() directly.


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 easy-to-alter
compile time knob so the default can be overridden in a single location?

Perhaps something like:

#ifndef NDEBUG
#define XMEM_POOL_CHECKS
#endif

#ifdef XMEM_POOL_CHECKS
...
#else
...
#endif

~Andrew


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

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

[Julien Grall]

Hi,

On 16/12/2014 20:28, Andrew Cooper wrote:
> I suspect you also would be better, and certainly more brief, with
> "run_in_exception_handler(show_stack)" instead, which will just print a
> stack trace, but nothing more.

FIY, run_in_exception_handler doesn't exists on ARM.

Regards,

-- 
Julien Grall

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

[Andrew Cooper]

On 16/12/2014 23:06, Julien Grall wrote:
> Hi,
>
> On 16/12/2014 20:28, Andrew Cooper wrote:
>> I suspect you also would be better, and certainly more brief, with
>> "run_in_exception_handler(show_stack)" instead, which will just print a
>> stack trace, but nothing more.
>
> FIY, run_in_exception_handler doesn't exists on ARM.
>
> Regards,
>

In which case it even more lucky that __bug() and __warn() are orphaned
functions, as they don't work correctly on arm.  Even more reason to
remove them.

It doesn't look like run_in_exception_handler() would be hard to add to
arm at all.  It already has broadly similar bug/warn/assert infrastructure.

~Andrew

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

[Julien Grall]

On 16/12/2014 23:26, Andrew Cooper wrote:
> On 16/12/2014 23:06, Julien Grall wrote:
>> Hi,
>>
>> On 16/12/2014 20:28, Andrew Cooper wrote:
>>> I suspect you also would be better, and certainly more brief, with
>>> "run_in_exception_handler(show_stack)" instead, which will just print a
>>> stack trace, but nothing more.
>>
>> FIY, run_in_exception_handler doesn't exists on ARM.
>>
>> Regards,
>>
>
> In which case it even more lucky that __bug() and __warn() are orphaned
> functions, as they don't work correctly on arm.  Even more reason to
> remove them.
>
> It doesn't look like run_in_exception_handler() would be hard to add to
> arm at all.  It already has broadly similar bug/warn/assert infrastructure.

It's more difficult than you think. For ARM we had to use a different 
infrastructure than x86 to setup the BUG_FRAME. This is because %c is 
not correctly support on most ARM compiler today.

So I don't think it worth to spend time on it just for one call.

How about introducing dump_stack() which would call 
run_in_exception_handler() on x86 and something different (maybe a new 
BUG_FRAME type) on ARM?

Regards,

-- 
Julien Grall

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

[Jan Beulich]

>>> On 16.12.14 at 21:28, <andrew.cooper3@citrix.com> wrote:
> On 16/12/14 19:33, Mihai Donțu wrote:
>> +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;
> 
> What is this static doing?  Surely corruption corruption in one pool has
> no effect on corruption in a separate pool (other than the usual side
> effects of general memory corruption, which tend to be bad).
> 
> It looks as if it wants to be an extra field in an xmem_pool.

Question is whether logging more than the first corruption ever is
really all that useful.


>> +bool_t __xmem_pool_check(const char *file, int line, struct xmem_pool *pool)
>> +{
>> +    return __xmem_pool_check_unlocked(file, line, pool ?: xenpool);
> 
> Why should a NULL pool be tolerated here?  This is debug code only, so
> we can require and trust that we are called appropriately.

xenpool is not and should not be visible to code outside this file.

Jan

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

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

[Andrew Cooper]

On 17/12/14 09:59, Jan Beulich wrote:
>>>> On 16.12.14 at 21:28, <andrew.cooper3@citrix.com> wrote:
>> On 16/12/14 19:33, Mihai Donțu wrote:
>>> +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;
>> What is this static doing?  Surely corruption corruption in one pool has
>> no effect on corruption in a separate pool (other than the usual side
>> effects of general memory corruption, which tend to be bad).
>>
>> It looks as if it wants to be an extra field in an xmem_pool.
> Question is whether logging more than the first corruption ever is
> really all that useful.

True - as soon as one bit of memory corruption is detected, all other
bets are off.

Might it perhaps be prudent to then panic() on a positive detection of
memory corruption?

>
>
>>> +bool_t __xmem_pool_check(const char *file, int line, struct xmem_pool *pool)
>>> +{
>>> +    return __xmem_pool_check_unlocked(file, line, pool ?: xenpool);
>> Why should a NULL pool be tolerated here?  This is debug code only, so
>> we can require and trust that we are called appropriately.
> xenpool is not and should not be visible to code outside this file.

Quite, but the only plausible external callers will be checking pools of
their own.  This patch adds internal callers for checking the xenpool.

~Andrew

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

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

[Andrew Cooper]

On 16/12/14 23:37, Julien Grall wrote:
>
>
> On 16/12/2014 23:26, Andrew Cooper wrote:
>> On 16/12/2014 23:06, Julien Grall wrote:
>>> Hi,
>>>
>>> On 16/12/2014 20:28, Andrew Cooper wrote:
>>>> I suspect you also would be better, and certainly more brief, with
>>>> "run_in_exception_handler(show_stack)" instead, which will just
>>>> print a
>>>> stack trace, but nothing more.
>>>
>>> FIY, run_in_exception_handler doesn't exists on ARM.
>>>
>>> Regards,
>>>
>>
>> In which case it even more lucky that __bug() and __warn() are orphaned
>> functions, as they don't work correctly on arm.  Even more reason to
>> remove them.
>>
>> It doesn't look like run_in_exception_handler() would be hard to add to
>> arm at all.  It already has broadly similar bug/warn/assert
>> infrastructure.
>
> It's more difficult than you think. For ARM we had to use a different
> infrastructure than x86 to setup the BUG_FRAME. This is because %c is
> not correctly support on most ARM compiler today.
>
> So I don't think it worth to spend time on it just for one call.
>
> How about introducing dump_stack() which would call
> run_in_exception_handler() on x86 and something different (maybe a new
> BUG_FRAME type) on ARM?

Introducing a new bugframe is precicely what I meant by "this doesn't
look hard".  x86 currently has one more bugframe than arm, being
BUGFRAME_run_fn.

At this point, run_in_exception_handler() effectively becomes common.

~Andrew

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

[Jan Beulich]

>>> On 17.12.14 at 11:08, <andrew.cooper3@citrix.com> wrote:
> On 17/12/14 09:59, Jan Beulich wrote:
>>>>> On 16.12.14 at 21:28, <andrew.cooper3@citrix.com> wrote:
>>> On 16/12/14 19:33, Mihai Donțu wrote:
>>>> +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;
>>> What is this static doing?  Surely corruption corruption in one pool has
>>> no effect on corruption in a separate pool (other than the usual side
>>> effects of general memory corruption, which tend to be bad).
>>>
>>> It looks as if it wants to be an extra field in an xmem_pool.
>> Question is whether logging more than the first corruption ever is
>> really all that useful.
> 
> True - as soon as one bit of memory corruption is detected, all other
> bets are off.
> 
> Might it perhaps be prudent to then panic() on a positive detection of
> memory corruption?

Indeed.

>>>> +bool_t __xmem_pool_check(const char *file, int line, struct xmem_pool 
> *pool)
>>>> +{
>>>> +    return __xmem_pool_check_unlocked(file, line, pool ?: xenpool);
>>> Why should a NULL pool be tolerated here?  This is debug code only, so
>>> we can require and trust that we are called appropriately.
>> xenpool is not and should not be visible to code outside this file.
> 
> Quite, but the only plausible external callers will be checking pools of
> their own.  This patch adds internal callers for checking the xenpool.

If you look up earlier conversation on this patch and its origin, I
think you'll find that it is specifically intended for calls to this to be
addable at arbitrary places in the code.

Jan

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

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

[Julien Grall]

On 17/12/2014 10:11, Andrew Cooper wrote:
> On 16/12/14 23:37, Julien Grall wrote:
> Introducing a new bugframe is precicely what I meant by "this doesn't
> look hard".  x86 currently has one more bugframe than arm, being
> BUGFRAME_run_fn.

And how do you pass the pointer of the function? As I said, ARM lacks of 
%c because of compiler issue.

I'm out of idea on how to do simply on ARM. Let me know if you have a 
good solution.

What I meant with introduce a new bug frame adding BUGFRAM_dump_stack.

Regards,

-- 
Julien Grall

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

[Andrew Cooper]

On 17/12/14 10:24, Julien Grall wrote:
>
>
> On 17/12/2014 10:11, Andrew Cooper wrote:
>> On 16/12/14 23:37, Julien Grall wrote:
>> Introducing a new bugframe is precicely what I meant by "this doesn't
>> look hard".  x86 currently has one more bugframe than arm, being
>> BUGFRAME_run_fn.
>
> And how do you pass the pointer of the function? As I said, ARM lacks
> of %c because of compiler issue.
>
> I'm out of idea on how to do simply on ARM. Let me know if you have a
> good solution.
>
> What I meant with introduce a new bug frame adding BUGFRAM_dump_stack.
>
> Regards,
>

I don't know about good, but stringly typing the function pointer and
stashing it in the same way as the assert message would work.

(I can see now why you suggested BUGFRAME_dump_stack, but it would be
nice to find a more generic alternative if possible.)

~Andrew

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

[Jan Beulich]

>>> On 17.12.14 at 11:24, <julien.grall@linaro.org> wrote:
> On 17/12/2014 10:11, Andrew Cooper wrote:
>> On 16/12/14 23:37, Julien Grall wrote:
>> Introducing a new bugframe is precicely what I meant by "this doesn't
>> look hard".  x86 currently has one more bugframe than arm, being
>> BUGFRAME_run_fn.
> 
> And how do you pass the pointer of the function? As I said, ARM lacks of 
> %c because of compiler issue.

First of all can you explain what compiler issue there is? Both
{arm,aarch64}_print_operand() handle 'c' (but of course I can't
tell whether correctly).

And then can't you possibly find ways to deal with the # prefix
added when not using 'c'? For the section name (if you need the
section split in the first place) I would think a # should be fine,
and the uses in expressions can surely be worked around by
interposing an assembler macro (even if that macro may end up
looking quite ugly).

Jan

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

[Julien Grall]

Hi Jan,

On 17/12/14 10:40, Jan Beulich wrote:
>>>> On 17.12.14 at 11:24, <julien.grall@linaro.org> wrote:
>> On 17/12/2014 10:11, Andrew Cooper wrote:
>>> On 16/12/14 23:37, Julien Grall wrote:
>>> Introducing a new bugframe is precicely what I meant by "this doesn't
>>> look hard".  x86 currently has one more bugframe than arm, being
>>> BUGFRAME_run_fn.
>>
>> And how do you pass the pointer of the function? As I said, ARM lacks of 
>> %c because of compiler issue.
> 
> First of all can you explain what compiler issue there is? Both
> {arm,aarch64}_print_operand() handle 'c' (but of course I can't
> tell whether correctly).

This has been added recently on aarch64 (late 2013) and there was an
outstanding bug on some version of GCC for arm
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48637)

FYI, the BUG support has been borrowed from Linux.

> And then can't you possibly find ways to deal with the # prefix
> added when not using 'c'? For the section name (if you need the
> section split in the first place) I would think a # should be fine,
> and the uses in expressions can surely be worked around by
> interposing an assembler macro (even if that macro may end up
> looking quite ugly).

We already have a working solution without BUGFRAME_run_fn. See the
implementation in xen/include/asm-arm.h.

I don't plan to work on modifying this code. But I would be happy if
someone post a patch to support BUGFRAME_run_fn.

Regards,

-- 
Julien Grall

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

[Jan Beulich]

>>> On 16.12.14 at 20:33, <mdontu@bitdefender.com> 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@bitdefender.com>

Andrew, Julien,

having gone through the discussion following this patch submission
once again just now, I wonder where we are: If you're objecting to
any part of the patch as is, please call this out clearly so that Mihai
has a way to address your concerns. Otherwise please state
clearly that you don't object to the patch going in as is.

Thanks, Jan

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

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

[Andrew Cooper]

On 07/01/15 15:41, Jan Beulich wrote:
>>>> On 16.12.14 at 20:33, <mdontu@bitdefender.com> 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@bitdefender.com>
> Andrew, Julien,
>
> having gone through the discussion following this patch submission
> once again just now, I wonder where we are: If you're objecting to
> any part of the patch as is, please call this out clearly so that Mihai
> has a way to address your concerns. Otherwise please state
> clearly that you don't object to the patch going in as is.
>
> Thanks, Jan

Despite some of the back-and-forth, there are still issues I would like
to see addressed.

I will send another review, adjusting for the results of the back-and-forth.

~Andrew


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

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

[Andrew Cooper]

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@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.

> +            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.

> +
> +#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?

Perhaps something like:

#ifndef NDEBUG
#define XMEM_POOL_CHECKS
#endif

#ifdef XMEM_POOL_CHECKS
...
#else
...
#endif

~Andrew


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

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

[Mihai Donțu]

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@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