[PATCH] Replace zone padding with a definition in cache.h

[PATCH] Replace zone padding with a definition in cache.h

[Christoph Lameter]

This patch removes the zone padding hack and establishes definitions
in include/linux/cache.h to define the padding within struct zone.

Signed-off-by: Christoph Lameter <christoph@lameter.com>
Signed-off-by: Shai Fultheim <Shai@Scalex86.org>

Index: linux-2.6.11/include/linux/cache.h
===================================================================
--- linux-2.6.11.orig/include/linux/cache.h	2005-03-08 18:40:15.000000000 -0800
+++ linux-2.6.11/include/linux/cache.h	2005-03-14 10:33:45.247701040 -0800
@@ -48,4 +48,12 @@
 #endif
 #endif

+#ifndef ____cacheline_pad_in_smp
+#if defined(CONFIG_SMP)
+#define ____cacheline_pad_in_smp struct { char  x; } ____cacheline_maxaligned_in_smp
+#else
+#define ____cacheline_pad_in_smp
+#endif
+#endif
+
 #endif /* __LINUX_CACHE_H */
Index: linux-2.6.11/include/linux/mmzone.h
===================================================================
--- linux-2.6.11.orig/include/linux/mmzone.h	2005-03-14 10:33:01.037422024 -0800
+++ linux-2.6.11/include/linux/mmzone.h	2005-03-14 10:33:45.248700888 -0800
@@ -28,21 +28,6 @@ struct free_area {

 struct pglist_data;

-/*
- * zone->lock and zone->lru_lock are two of the hottest locks in the kernel.
- * So add a wild amount of padding here to ensure that they fall into separate
- * cachelines.  There are very few zone structures in the machine, so space
- * consumption is not a concern here.
- */
-#if defined(CONFIG_SMP)
-struct zone_padding {
-	char x[0];
-} ____cacheline_maxaligned_in_smp;
-#define ZONE_PADDING(name)	struct zone_padding name;
-#else
-#define ZONE_PADDING(name)
-#endif
-
 struct per_cpu_pages {
 	int count;		/* number of pages in the list */
 	int low;		/* low watermark, refill needed */
@@ -131,7 +116,14 @@ struct zone {
 	struct free_area	free_area[MAX_ORDER];


-	ZONE_PADDING(_pad1_)
+	/*
+	 * zone->lock and zone->lru_lock are two of the hottest locks in the kernel.
+	 * So add a wild amount of padding here to ensure that they fall into separate
+	 * cachelines.  There are very few zone structures in the machine, so space
+	 * consumption is not a concern here.
+	 */
+
+	____cacheline_pad_in_smp;

 	/* Fields commonly accessed by the page reclaim scanner */
 	spinlock_t		lru_lock;
@@ -164,7 +156,7 @@ struct zone {
 	int prev_priority;


-	ZONE_PADDING(_pad2_)
+	____cacheline_pad_in_smp;
 	/* Rarely used or read-mostly fields */

 	/*

Re: [PATCH] Replace zone padding with a definition in cache.h

[Andrew Morton]

Christoph Lameter <christoph@lameter.com> wrote:
>
>  +#ifndef ____cacheline_pad_in_smp
>  +#if defined(CONFIG_SMP)
>  +#define ____cacheline_pad_in_smp struct { char  x; } ____cacheline_maxaligned_in_smp
>  +#else
>  +#define ____cacheline_pad_in_smp
>  +#endif
>  +#endif

That's going to spit a warning with older gcc's.  "warning: unnamed
struct/union that defines no instances".

Re: [PATCH] Replace zone padding with a definition in cache.h

[Nick Piggin]

On Tue, 2005-03-15 at 20:12 -0800, Christoph Lameter wrote:
> This patch removes the zone padding hack and establishes definitions
> in include/linux/cache.h to define the padding within struct zone.
> 
> Signed-off-by: Christoph Lameter <christoph@lameter.com>
> Signed-off-by: Shai Fultheim <Shai@Scalex86.org>
> 
> Index: linux-2.6.11/include/linux/cache.h
> ===================================================================
> --- linux-2.6.11.orig/include/linux/cache.h	2005-03-08 18:40:15.000000000 -0800
> +++ linux-2.6.11/include/linux/cache.h	2005-03-14 10:33:45.247701040 -0800
> @@ -48,4 +48,12 @@
>  #endif
>  #endif
> 
> +#ifndef ____cacheline_pad_in_smp
> +#if defined(CONFIG_SMP)
> +#define ____cacheline_pad_in_smp struct { char  x; } ____cacheline_maxaligned_in_smp
                                             ^^^^^^^

Doesn't this add a redundant cacheline if the padding is
previously perfect? Because of the extra byte you're adding?

IIRC, the char x[0]; trick does the job correctly.

Re: [PATCH] Replace zone padding with a definition in cache.h

[Christoph Lameter]

On Tue, 15 Mar 2005, Andrew Morton wrote:

> Christoph Lameter <christoph@lameter.com> wrote:
> >
> >  +#ifndef ____cacheline_pad_in_smp
> >  +#if defined(CONFIG_SMP)
> >  +#define ____cacheline_pad_in_smp struct { char  x; } ____cacheline_maxaligned_in_smp
> >  +#else
> >  +#define ____cacheline_pad_in_smp
> >  +#endif
> >  +#endif
>
> That's going to spit a warning with older gcc's.  "warning: unnamed
> struct/union that defines no instances".
>
Is it really that important? If the struct is named then there may be
conflicts if its used repeatedly.

Re: [PATCH] Replace zone padding with a definition in cache.h

[Christoph Lameter]

On Wed, 16 Mar 2005, Nick Piggin wrote:

> >
> > +#ifndef ____cacheline_pad_in_smp
> > +#if defined(CONFIG_SMP)
> > +#define ____cacheline_pad_in_smp struct { char  x; } ____cacheline_maxaligned_in_smp
>                                              ^^^^^^^
>
> Doesn't this add a redundant cacheline if the padding is
> previously perfect? Because of the extra byte you're adding?
>
> IIRC, the char x[0]; trick does the job correctly.

Good idea.

This patch removes the zone padding hack and establishes definitions
in include/linux/cache.h to define the padding within struct zone.

Signed-off-by: Christoph Lameter <christoph@lameter.com>
Signed-off-by: Shai Fultheim <Shai@Scalex86.org>

Index: linux-2.6.11/include/linux/cache.h
===================================================================
--- linux-2.6.11.orig/include/linux/cache.h	2005-03-08 18:40:15.000000000 -0800
+++ linux-2.6.11/include/linux/cache.h	2005-03-14 10:33:45.247701040 -0800
@@ -48,4 +48,12 @@
 #endif
 #endif

+#ifndef ____cacheline_pad_in_smp
+#if defined(CONFIG_SMP)
+#define ____cacheline_pad_in_smp struct { char  x[0]; } ____cacheline_maxaligned_in_smp
+#else
+#define ____cacheline_pad_in_smp
+#endif
+#endif
+
 #endif /* __LINUX_CACHE_H */
Index: linux-2.6.11/include/linux/mmzone.h
===================================================================
--- linux-2.6.11.orig/include/linux/mmzone.h	2005-03-14 10:33:01.037422024 -0800
+++ linux-2.6.11/include/linux/mmzone.h	2005-03-14 10:33:45.248700888 -0800
@@ -28,21 +28,6 @@ struct free_area {

 struct pglist_data;

-/*
- * zone->lock and zone->lru_lock are two of the hottest locks in the kernel.
- * So add a wild amount of padding here to ensure that they fall into separate
- * cachelines.  There are very few zone structures in the machine, so space
- * consumption is not a concern here.
- */
-#if defined(CONFIG_SMP)
-struct zone_padding {
-	char x[0];
-} ____cacheline_maxaligned_in_smp;
-#define ZONE_PADDING(name)	struct zone_padding name;
-#else
-#define ZONE_PADDING(name)
-#endif
-
 struct per_cpu_pages {
 	int count;		/* number of pages in the list */
 	int low;		/* low watermark, refill needed */
@@ -131,7 +116,14 @@ struct zone {
 	struct free_area	free_area[MAX_ORDER];


-	ZONE_PADDING(_pad1_)
+	/*
+	 * zone->lock and zone->lru_lock are two of the hottest locks in the kernel.
+	 * So add a wild amount of padding here to ensure that they fall into separate
+	 * cachelines.  There are very few zone structures in the machine, so space
+	 * consumption is not a concern here.
+	 */
+
+	____cacheline_pad_in_smp;

 	/* Fields commonly accessed by the page reclaim scanner */
 	spinlock_t		lru_lock;
@@ -164,7 +156,7 @@ struct zone {
 	int prev_priority;


-	ZONE_PADDING(_pad2_)
+	____cacheline_pad_in_smp;
 	/* Rarely used or read-mostly fields */

 	/*

Re: [PATCH] Replace zone padding with a definition in cache.h

[Andrew Morton]

Christoph Lameter <christoph@lameter.com> wrote:
>
> On Tue, 15 Mar 2005, Andrew Morton wrote:
> 
> > Christoph Lameter <christoph@lameter.com> wrote:
> > >
> > >  +#ifndef ____cacheline_pad_in_smp
> > >  +#if defined(CONFIG_SMP)
> > >  +#define ____cacheline_pad_in_smp struct { char  x; } ____cacheline_maxaligned_in_smp
> > >  +#else
> > >  +#define ____cacheline_pad_in_smp
> > >  +#endif
> > >  +#endif
> >
> > That's going to spit a warning with older gcc's.  "warning: unnamed
> > struct/union that defines no instances".
> >
> Is it really that important?

Well, it makes gcc-2.95.x unusable, and a number of people like to use it.

It has not proven too burdensome to support.  And we know that if it works
on 2.95.x, it will work on 3.1, 3.2, 3.3, etc.

> If the struct is named then there may be
> conflicts if its used repeatedly.

Hence the "hack" which you just deleted ;)

Re: [PATCH] Replace zone padding with a definition in cache.h

[Christoph Lameter]

On Tue, 15 Mar 2005, Andrew Morton wrote:

> > If the struct is named then there may be
> > conflicts if its used repeatedly.
>
> Hence the "hack" which you just deleted ;)

Ok, Master, I see the light....