Re: [PATCH next/mmotm] slub: partly fix freeze in __slab_free

[Hugh Dickins]

On Tue, 12 Jul 2011, Christoph Lameter wrote:
> I wish there would be some easier way to overlay fields.

Yes, struct page has become ... a mess.

(I'd use stronger language, but I'm being a little careful at the
moment, not to say something that Andrew might then throw back at
me in our radix-tree discussion ;)

I'd be happy for the 32bit and 64bit cases to be separated entirely,
if that would help seeing their layouts clearly (but I don't think 
that's actually the problem).

It is the (standard e.g. x86_64) 64bit layout to which you are adding
8 bytes, isn't it?  Not in the patch below, but in your cmpxchg_double
slub series.  Which aligns it nicely for everyone, but does use
significantly more mem_map memory - I wonder how many people
have really taken that on board.

> The counters
> field exists to allow an easier reference to the bitfield structure in
> order to pass it to cmpxchg_double_slab.
> 
> In 64 bit the counters field needs to overlay the bitfields and _count
> since we can only pass 64 bits integers to cmpxchg_double_slab.
> 
> On 32 bit counters field only covers the bitfields.
> 
> The "new" page struct in __slab_free is only partially populated and the
> overlays are used for conversion between structs and words in order to be
> able to pass the struct contents by value.
> 
> Signed-off-by: Christoph Lameter <cl@xxxxxxxxx>

As I reported in Fengguang's more_io thread, I couldn't get this patch
to apply, unsurprisingly to mmotm, more surprisingly to Pekka's for-next
(which adds in the _count build fix).  Did you try to tidy up the
indentations a little in between?

At the bottom I've put the patch against mmotm which I started testing
instead on ppc64 a few hours ago.  It again seized up in __slab_free()
after two and a half hours.  So either I screwed up my transliteration
of your patch, or your patch is still wrong.  Please check mine out.

Is yours actually tested on PowerPC or ARM, say?  I wonder if you're
making endianness or bitfield ordering assumptions; but I've made no
attempt to work it out.  Anything I can send you?  Disassembly of
__slab_free()?

I didn't actually try the patch on x86 at all.

> 
> ---
>  include/linux/mm_types.h |   22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> Index: linux-2.6/include/linux/mm_types.h
> ===================================================================
> --- linux-2.6.orig/include/linux/mm_types.h	2011-07-12 10:14:08.057706223 -0500
> +++ linux-2.6/include/linux/mm_types.h	2011-07-12 10:14:14.117706184 -0500
> @@ -56,20 +56,26 @@ struct page {
>  		};
> 
>  		union {
> -			atomic_t _mapcount;	/* Count of ptes mapped in mms,
> +			/* Used for cmpxchg_double in slub */
> +			unsigned long counters;
> +
> +			struct {
> +
> +				union {
> +					atomic_t _mapcount;	/* Count of ptes mapped in mms,
>  							 * to show when page is mapped
>  							 * & limit reverse map searches.
>  							 */
> 
> -			/* Used for cmpxchg_double in slub */
> -			unsigned long counters;
> -			struct {
> -				unsigned inuse:16;
> -				unsigned objects:15;
> -				unsigned frozen:1;
> +					struct {
> +						unsigned inuse:16;
> +						unsigned objects:15;
> +						unsigned frozen:1;
> +					};
> +				};
> +				atomic_t _count;		/* Usage count, see below. */
>  			};
>  		};
> -		atomic_t _count;		/* Usage count, see below. */
>  	};
> 
>  	/* Third double word block */

This is what I tested with: is it what you intended against mmotm?

--- mmotm/include/linux/mm_types.h	2011-07-09 11:25:18.000000000 -0700
+++ linux/include/linux/mm_types.h	2011-07-12 22:04:01.000000000 -0700
@@ -49,30 +49,31 @@ struct page {
 					 * see PAGE_MAPPING_ANON below.
 					 */
 	/* Second double word */
-	union {
-		struct {
+	struct {
+		union {
 			pgoff_t index;		/* Our offset within mapping. */
-			atomic_t _mapcount;	/* Count of ptes mapped in mms,
+			void *freelist;		/* slub first free object */
+		};
+
+		union {
+			/* Used for cmpxchg_double in slub */
+			unsigned long counters;
+
+			struct {
+
+				union {
+					atomic_t _mapcount;	/* Count of ptes mapped in mms,
 							 * to show when page is mapped
 							 * & limit reverse map searches.
 							 */
-			atomic_t _count;		/* Usage count, see below. */
-		};
 
-		struct {			/* SLUB cmpxchg_double area */
-			void *freelist;
-			union {
-				unsigned long counters;
-				struct {
-					unsigned inuse:16;
-					unsigned objects:15;
-					unsigned frozen:1;
-					/*
-					 * Kernel may make use of this field even when slub
-					 * uses the rest of the double word!
-					 */
-					atomic_t _count;
+					struct {
+						unsigned inuse:16;
+						unsigned objects:15;
+						unsigned frozen:1;
+					};
 				};
+				atomic_t _count;		/* Usage count, see below. */
 			};
 		};
 	};
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/