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

[PATCH v2] 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 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 | 110 +++++++++++++++++++++++++++++++++++++++++++++-
 xen/include/xen/xmalloc.h |   7 +++
 2 files changed, 115 insertions(+), 2 deletions(-)

diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
index a5769c9..8681185 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -120,9 +120,111 @@ 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 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);
+            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)
+{
+    int i;
+    static bool_t once = 1;
+
+    if ( !once )
+        goto out;
+    for ( i = 0; i < REAL_FLI; i++ )
+    {
+        int fl = (pool->fl_bitmap & (1 << i)) ? i : -1;
+
+        if ( fl >= 0 )
+        {
+            int j;
+
+            if ( !pool->sl_bitmap[fl] )
+            {
+                printk(XENLOG_ERR "xmem_pool: the TLSF bitmap is corrupted (non-empty FL with empty SL)\n");
+                __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: the TLSF bitmap is corrupted (matrix[%d][%d] is NULL)\n",
+                        fl, sl);
+                    __warn(file, line);
+                    once = 0;
+                    break;
+                }
+                if ( !xmem_pool_check_size(pool->matrix[fl][sl], fl, sl) )
+                {
+                    printk(XENLOG_ERR "xmem_pool: the TLSF chunk matrix is corrupted\n");
+                    __warn(file, line);
+                    once = 0;
+                    break;
+                }
+            }
+            if ( !once )
+                break;
+        }
+    }
+out:
+    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 ? 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 +483,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 +546,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 +570,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 +607,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 +620,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 v2] xmalloc: add support for checking the pool integrity

[Mihai Donțu]

On Mon, 8 Dec 2014 04:30:48 +0200 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 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 | 110 +++++++++++++++++++++++++++++++++++++++++++++-
>  xen/include/xen/xmalloc.h |   7 +++
>  2 files changed, 115 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> index a5769c9..8681185 100644
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -120,9 +120,111 @@ 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 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);
> +            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)
> +{
> +    int i;
> +    static bool_t once = 1;
> +
> +    if ( !once )
> +        goto out;
> +    for ( i = 0; i < REAL_FLI; i++ )
> +    {
> +        int fl = (pool->fl_bitmap & (1 << i)) ? i : -1;
> +
> +        if ( fl >= 0 )
> +        {
> +            int j;
> +
> +            if ( !pool->sl_bitmap[fl] )
> +            {
> +                printk(XENLOG_ERR "xmem_pool: the TLSF bitmap is corrupted (non-empty FL with empty SL)\n");
> +                __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: the TLSF bitmap is corrupted (matrix[%d][%d] is NULL)\n",
> +                        fl, sl);
> +                    __warn(file, line);
> +                    once = 0;
> +                    break;
> +                }
> +                if ( !xmem_pool_check_size(pool->matrix[fl][sl], fl, sl) )
> +                {
> +                    printk(XENLOG_ERR "xmem_pool: the TLSF chunk matrix is corrupted\n");
> +                    __warn(file, line);
> +                    once = 0;
> +                    break;
> +                }
> +            }
> +            if ( !once )
> +                break;
> +        }
> +    }
> +out:
> +    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 ? 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 +483,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 +546,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 +570,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 +607,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 +620,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__ */

This patch depends on:
http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00809.html

-- 
Mihai DONȚU

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

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

[Jan Beulich]

>>> On 08.12.14 at 03:30, <mdontu@bitdefender.com> wrote:
> +#ifndef NDEBUG
> +static bool_t 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",

Quoting my reply to v1: "Long line. Only the format message alone
is allowed to exceed 80 characters."

Also with there potentially being multiple pools, shouldn't all of the
log messages the patch issues be extended to allow identifying the
offending one?

> +bool_t __xmem_pool_check(const char *file, int line, struct xmem_pool *pool)
> +{
> +    return __xmem_pool_check_unlocked(file, line, pool ? pool : xenpool);

For brevity, the shorter "pool ?: xenpool" is generally preferable. The
only place using this is not allowed are the public headers.

Jan

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

[Mihai Donțu]

On Monday 08 December 2014 10:18:01 Jan Beulich wrote:
> >>> On 08.12.14 at 03:30, <mdontu@bitdefender.com> wrote:
> > +#ifndef NDEBUG
> > +static bool_t 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",
> 
> Quoting my reply to v1: "Long line. Only the format message alone
> is allowed to exceed 80 characters."
> 

Just so I don't send another faulty patch, you would see that printk()
being:

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

?

> Also with there potentially being multiple pools, shouldn't all of the
> log messages the patch issues be extended to allow identifying the
> offending one?
> 

I think I can insert the pool name in that message too. Something like:

  printk(XENLOG_ERR
         "xmem_pool: %s: for block [...]\n",
         pool->name, b, b->size [...]);

would do? A quick preview:

[2014-12-04 15:41:23] (XEN) [ 1374.507125] xmem_pool: xmalloc: for block ffff8304004fb9b0 size = 0, { fl = 3, sl = 9 } should be { fl = 0, sl = 0 }
[2014-12-04 15:41:23] (XEN) [ 1374.507127] xmem_pool: xmalloc: the TLSF chunk matrix is corrupted

> > +bool_t __xmem_pool_check(const char *file, int line, struct xmem_pool *pool)
> > +{
> > +    return __xmem_pool_check_unlocked(file, line, pool ? pool : xenpool);
> 
> For brevity, the shorter "pool ?: xenpool" is generally preferable. The
> only place using this is not allowed are the public headers.
> 

Will do.

Thank you,

-- 
Mihai DONȚU

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

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

[Ian Campbell]

On Mon, 2014-12-08 at 18:00 +0200, Mihai Donțu wrote:
> On Monday 08 December 2014 10:18:01 Jan Beulich wrote:
> > >>> On 08.12.14 at 03:30, <mdontu@bitdefender.com> wrote:
> > > +#ifndef NDEBUG
> > > +static bool_t 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",
> > 
> > Quoting my reply to v1: "Long line. Only the format message alone
> > is allowed to exceed 80 characters."
> > 
> 
> Just so I don't send another faulty patch, you would see that printk()
> being:
> 
>   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);
> 
> ?

The log message here is going to be substantially more than 80
characters (the format string by itself already is). Could you find a
more compact representation of the useful info?

Ian.


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

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

[Mihai Donțu]

On Monday 08 December 2014 16:04:55 Ian Campbell wrote:
> On Mon, 2014-12-08 at 18:00 +0200, Mihai Donțu wrote:
> > On Monday 08 December 2014 10:18:01 Jan Beulich wrote:
> > > >>> On 08.12.14 at 03:30, <mdontu@bitdefender.com> wrote:
> > > > +#ifndef NDEBUG
> > > > +static bool_t 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",
> > > 
> > > Quoting my reply to v1: "Long line. Only the format message alone
> > > is allowed to exceed 80 characters."
> > > 
> > 
> > Just so I don't send another faulty patch, you would see that printk()
> > being:
> > 
> >   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);
> > 
> > ?
> 
> The log message here is going to be substantially more than 80
> characters (the format string by itself already is). Could you find a
> more compact representation of the useful info?
> 

Ah! I see. I apologize for being so slow. :-) How about:

printk(XENLOG_ERR "xmem_pool: %s: misplaced block %p:%u ({%d,%d} -> {%d,%d})\n",
       pool->name, b, b->size, fl, sl, __fl, __sl); 

Looks a bit cryptic, but the TLSF itself is pretty complex and for
brave souls wishing to debug it, the message format will be the last
thing on their minds. :-)

Preview:

[2014-12-04 15:41:23] (XEN) [ 1374.507125] xmem_pool: xmalloc: misplaced block ffff8304004fb9b0:0 ({3,9} -> {0,0})
[2014-12-04 15:41:23] (XEN) [ 1374.507127] xmem_pool: xmalloc: the TLSF chunk matrix is corrupted

Thanks,

-- 
Mihai DONȚU

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

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

[Jan Beulich]

>>> On 08.12.14 at 17:00, <mdontu@bitdefender.com> wrote:
> On Monday 08 December 2014 10:18:01 Jan Beulich wrote:
>> >>> On 08.12.14 at 03:30, <mdontu@bitdefender.com> wrote:
>> > +#ifndef NDEBUG
>> > +static bool_t 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",
>> 
>> Quoting my reply to v1: "Long line. Only the format message alone
>> is allowed to exceed 80 characters."
>> 
> 
> Just so I don't send another faulty patch, you would see that printk()
> being:
> 
>   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);
> 
> ?

Yes. And I'd also recommend removing the spaces around the =
characters in the format string. Considering the message being a
debugging one, perhaps the two pairs could be printed without
any extras, i.e. just {x,y}.

>> Also with there potentially being multiple pools, shouldn't all of the
>> log messages the patch issues be extended to allow identifying the
>> offending one?
>> 
> 
> I think I can insert the pool name in that message too. Something like:
> 
>   printk(XENLOG_ERR
>          "xmem_pool: %s: for block [...]\n",
>          pool->name, b, b->size [...]);
> 
> would do? A quick preview:

Yes. I very think that's what the name's for after all.

Jan