Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Dan Magenheimer]

From: Dan Magenheimer <dan.magenheimer@oracle.com>
Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

This first patch of four in the frontswap series makes available core
swap data structures (swap_lock, swap_list and swap_info) that are
needed by frontswap.c but we don't need to expose them to the dozens
of files that include swap.h so we create a new swapfile.h just to
extern-ify these.

Also add frontswap-related elements to swap_info_struct.  Frontswap_map
points to vzalloc'ed one-bit-per-swap-page metadata that indicates
whether the swap page is in frontswap or in the device and frontswap_pages
counts how many pages are in frontswap.  We don't tie these to
CONFIG_FRONTSWAP to avoid unnecessary clutter around various frontswap
hooks.

[v6: rebase to 3.0-rc1]
[v5: no change from v4]
[v4: rebase to 2.6.39]
Signed-off-by: Dan Magenheimer <dan.magenheimer@oracle.com>
Reviewed-by: Konrad Wilk <konrad.wilk@oracle.com>
Acked-by: Seth Jennings <sjenning@linux.vnet.ibm.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Nitin Gupta <ngupta@vflare.org>
Cc: Matthew Wilcox <matthew@wil.cx>
Cc: Chris Mason <chris.mason@oracle.com>
Cc: Jan Beulich <JBeulich@novell.com>
Cc: Rik Riel <riel@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>

--- linux/include/linux/swapfile.h	1969-12-31 17:00:00.000000000 -0700
+++ frontswap/include/linux/swapfile.h	2011-08-08 08:59:03.951694506 -0600
@@ -0,0 +1,13 @@
+#ifndef _LINUX_SWAPFILE_H
+#define _LINUX_SWAPFILE_H
+
+/*
+ * these were static in swapfile.c but frontswap.c needs them and we don't
+ * want to expose them to the dozens of source files that include swap.h
+ */
+extern spinlock_t swap_lock;
+extern struct swap_list_t swap_list;
+extern struct swap_info_struct *swap_info[];
+extern int try_to_unuse(unsigned int, bool, unsigned long);
+
+#endif /* _LINUX_SWAPFILE_H */
--- linux/include/linux/swap.h	2011-08-08 08:19:25.880690134 -0600
+++ frontswap/include/linux/swap.h	2011-08-08 08:59:03.952691415 -0600
@@ -194,6 +194,8 @@ struct swap_info_struct {
 	struct block_device *bdev;	/* swap device or bdev of swap file */
 	struct file *swap_file;		/* seldom referenced */
 	unsigned int old_block_size;	/* seldom referenced */
+	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
+	unsigned int frontswap_pages;	/* frontswap pages in-use counter */
 };
 
 struct swap_list_t {

Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Dan Magenheimer]

From: Dan Magenheimer <dan.magenheimer@oracle.com>
Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

This first patch of four in the frontswap series makes available core
swap data structures (swap_lock, swap_list and swap_info) that are
needed by frontswap.c but we don't need to expose them to the dozens
of files that include swap.h so we create a new swapfile.h just to
extern-ify these.

Also add frontswap-related elements to swap_info_struct.  Frontswap_map
points to vzalloc'ed one-bit-per-swap-page metadata that indicates
whether the swap page is in frontswap or in the device and frontswap_pages
counts how many pages are in frontswap.  We don't tie these to
CONFIG_FRONTSWAP to avoid unnecessary clutter around various frontswap
hooks.

[v6: rebase to 3.0-rc1]
[v5: no change from v4]
[v4: rebase to 2.6.39]
Signed-off-by: Dan Magenheimer <dan.magenheimer@oracle.com>
Reviewed-by: Konrad Wilk <konrad.wilk@oracle.com>
Acked-by: Seth Jennings <sjenning@linux.vnet.ibm.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Nitin Gupta <ngupta@vflare.org>
Cc: Matthew Wilcox <matthew@wil.cx>
Cc: Chris Mason <chris.mason@oracle.com>
Cc: Jan Beulich <JBeulich@novell.com>
Cc: Rik Riel <riel@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>

--- linux/include/linux/swapfile.h	1969-12-31 17:00:00.000000000 -0700
+++ frontswap/include/linux/swapfile.h	2011-08-08 08:59:03.951694506 -0600
@@ -0,0 +1,13 @@
+#ifndef _LINUX_SWAPFILE_H
+#define _LINUX_SWAPFILE_H
+
+/*
+ * these were static in swapfile.c but frontswap.c needs them and we don't
+ * want to expose them to the dozens of source files that include swap.h
+ */
+extern spinlock_t swap_lock;
+extern struct swap_list_t swap_list;
+extern struct swap_info_struct *swap_info[];
+extern int try_to_unuse(unsigned int, bool, unsigned long);
+
+#endif /* _LINUX_SWAPFILE_H */
--- linux/include/linux/swap.h	2011-08-08 08:19:25.880690134 -0600
+++ frontswap/include/linux/swap.h	2011-08-08 08:59:03.952691415 -0600
@@ -194,6 +194,8 @@ struct swap_info_struct {
 	struct block_device *bdev;	/* swap device or bdev of swap file */
 	struct file *swap_file;		/* seldom referenced */
 	unsigned int old_block_size;	/* seldom referenced */
+	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
+	unsigned int frontswap_pages;	/* frontswap pages in-use counter */
 };
 
 struct swap_list_t {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Jan Beulich]

>>> On 08.08.11 at 22:45, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> From: Dan Magenheimer <dan.magenheimer@oracle.com>
> Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes
> 
> This first patch of four in the frontswap series makes available core
> swap data structures (swap_lock, swap_list and swap_info) that are
> needed by frontswap.c but we don't need to expose them to the dozens
> of files that include swap.h so we create a new swapfile.h just to
> extern-ify these.
> 
> Also add frontswap-related elements to swap_info_struct.  Frontswap_map
> points to vzalloc'ed one-bit-per-swap-page metadata that indicates
> whether the swap page is in frontswap or in the device and frontswap_pages
> counts how many pages are in frontswap.  We don't tie these to
> CONFIG_FRONTSWAP to avoid unnecessary clutter around various frontswap
> hooks.
> 
> [v6: rebase to 3.0-rc1]
> [v5: no change from v4]
> [v4: rebase to 2.6.39]
> Signed-off-by: Dan Magenheimer <dan.magenheimer@oracle.com>
> Reviewed-by: Konrad Wilk <konrad.wilk@oracle.com>
> Acked-by: Seth Jennings <sjenning@linux.vnet.ibm.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Nitin Gupta <ngupta@vflare.org>
> Cc: Matthew Wilcox <matthew@wil.cx>
> Cc: Chris Mason <chris.mason@oracle.com>
> Cc: Jan Beulich <JBeulich@novell.com>
> Cc: Rik Riel <riel@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> 
> --- linux/include/linux/swapfile.h	1969-12-31 17:00:00.000000000 -0700
> +++ frontswap/include/linux/swapfile.h	2011-08-08 08:59:03.951694506 -0600
> @@ -0,0 +1,13 @@
> +#ifndef _LINUX_SWAPFILE_H
> +#define _LINUX_SWAPFILE_H
> +
> +/*
> + * these were static in swapfile.c but frontswap.c needs them and we don't
> + * want to expose them to the dozens of source files that include swap.h
> + */
> +extern spinlock_t swap_lock;
> +extern struct swap_list_t swap_list;
> +extern struct swap_info_struct *swap_info[];
> +extern int try_to_unuse(unsigned int, bool, unsigned long);
> +
> +#endif /* _LINUX_SWAPFILE_H */
> --- linux/include/linux/swap.h	2011-08-08 08:19:25.880690134 -0600
> +++ frontswap/include/linux/swap.h	2011-08-08 08:59:03.952691415 -0600
> @@ -194,6 +194,8 @@ struct swap_info_struct {
>  	struct block_device *bdev;	/* swap device or bdev of swap file */
>  	struct file *swap_file;		/* seldom referenced */
>  	unsigned int old_block_size;	/* seldom referenced */

#ifdef CONFIG_FRONTSWAP

> +	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
> +	unsigned int frontswap_pages;	/* frontswap pages in-use counter */


#endif

(to eliminate any overhead with that config option unset)

Jan

>  };
>  
>  struct swap_list_t {

Re: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Jan Beulich]

>>> On 08.08.11 at 22:45, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> From: Dan Magenheimer <dan.magenheimer@oracle.com>
> Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes
> 
> This first patch of four in the frontswap series makes available core
> swap data structures (swap_lock, swap_list and swap_info) that are
> needed by frontswap.c but we don't need to expose them to the dozens
> of files that include swap.h so we create a new swapfile.h just to
> extern-ify these.
> 
> Also add frontswap-related elements to swap_info_struct.  Frontswap_map
> points to vzalloc'ed one-bit-per-swap-page metadata that indicates
> whether the swap page is in frontswap or in the device and frontswap_pages
> counts how many pages are in frontswap.  We don't tie these to
> CONFIG_FRONTSWAP to avoid unnecessary clutter around various frontswap
> hooks.
> 
> [v6: rebase to 3.0-rc1]
> [v5: no change from v4]
> [v4: rebase to 2.6.39]
> Signed-off-by: Dan Magenheimer <dan.magenheimer@oracle.com>
> Reviewed-by: Konrad Wilk <konrad.wilk@oracle.com>
> Acked-by: Seth Jennings <sjenning@linux.vnet.ibm.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Nitin Gupta <ngupta@vflare.org>
> Cc: Matthew Wilcox <matthew@wil.cx>
> Cc: Chris Mason <chris.mason@oracle.com>
> Cc: Jan Beulich <JBeulich@novell.com>
> Cc: Rik Riel <riel@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> 
> --- linux/include/linux/swapfile.h	1969-12-31 17:00:00.000000000 -0700
> +++ frontswap/include/linux/swapfile.h	2011-08-08 08:59:03.951694506 -0600
> @@ -0,0 +1,13 @@
> +#ifndef _LINUX_SWAPFILE_H
> +#define _LINUX_SWAPFILE_H
> +
> +/*
> + * these were static in swapfile.c but frontswap.c needs them and we don't
> + * want to expose them to the dozens of source files that include swap.h
> + */
> +extern spinlock_t swap_lock;
> +extern struct swap_list_t swap_list;
> +extern struct swap_info_struct *swap_info[];
> +extern int try_to_unuse(unsigned int, bool, unsigned long);
> +
> +#endif /* _LINUX_SWAPFILE_H */
> --- linux/include/linux/swap.h	2011-08-08 08:19:25.880690134 -0600
> +++ frontswap/include/linux/swap.h	2011-08-08 08:59:03.952691415 -0600
> @@ -194,6 +194,8 @@ struct swap_info_struct {
>  	struct block_device *bdev;	/* swap device or bdev of swap file */
>  	struct file *swap_file;		/* seldom referenced */
>  	unsigned int old_block_size;	/* seldom referenced */

#ifdef CONFIG_FRONTSWAP

> +	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
> +	unsigned int frontswap_pages;	/* frontswap pages in-use counter */


#endif

(to eliminate any overhead with that config option unset)

Jan

>  };
>  
>  struct swap_list_t {



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Dan Magenheimer]

> > --- linux/include/linux/swap.h	2011-08-08 08:19:25.880690134 -0600
> > +++ frontswap/include/linux/swap.h	2011-08-08 08:59:03.952691415 -0600
> > @@ -194,6 +194,8 @@ struct swap_info_struct {
> >  	struct block_device *bdev;	/* swap device or bdev of swap file */
> >  	struct file *swap_file;		/* seldom referenced */
> >  	unsigned int old_block_size;	/* seldom referenced */
> 
> #ifdef CONFIG_FRONTSWAP
> 
> > +	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
> > +	unsigned int frontswap_pages;	/* frontswap pages in-use counter */
> 
> 
> #endif
> 
> (to eliminate any overhead with that config option unset)
> 
> Jan

Hi Jan --

Thanks for the review!

As noted in the commit comment, if these structure elements are
not put inside an #ifdef CONFIG_FRONTSWAP, it becomes
unnecessary to clutter the core swap code with several ifdefs.
The cost is one pointer and one unsigned int per allocated
swap device (often no more than one swap device per system),
so the code clarity seemed more important than the tiny
additional runtime space cost.

Do you disagree?

Thanks,
Dan

RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Dan Magenheimer]

> > --- linux/include/linux/swap.h	2011-08-08 08:19:25.880690134 -0600
> > +++ frontswap/include/linux/swap.h	2011-08-08 08:59:03.952691415 -0600
> > @@ -194,6 +194,8 @@ struct swap_info_struct {
> >  	struct block_device *bdev;	/* swap device or bdev of swap file */
> >  	struct file *swap_file;		/* seldom referenced */
> >  	unsigned int old_block_size;	/* seldom referenced */
> 
> #ifdef CONFIG_FRONTSWAP
> 
> > +	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
> > +	unsigned int frontswap_pages;	/* frontswap pages in-use counter */
> 
> 
> #endif
> 
> (to eliminate any overhead with that config option unset)
> 
> Jan

Hi Jan --

Thanks for the review!

As noted in the commit comment, if these structure elements are
not put inside an #ifdef CONFIG_FRONTSWAP, it becomes
unnecessary to clutter the core swap code with several ifdefs.
The cost is one pointer and one unsigned int per allocated
swap device (often no more than one swap device per system),
so the code clarity seemed more important than the tiny
additional runtime space cost.

Do you disagree?

Thanks,
Dan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Jan Beulich]

>>> On 09.08.11 at 17:03, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
>> > --- linux/include/linux/swap.h	2011-08-08 08:19:25.880690134 -0600
>> > +++ frontswap/include/linux/swap.h	2011-08-08 08:59:03.952691415 -0600
>> > @@ -194,6 +194,8 @@ struct swap_info_struct {
>> >  	struct block_device *bdev;	/* swap device or bdev of swap file */
>> >  	struct file *swap_file;		/* seldom referenced */
>> >  	unsigned int old_block_size;	/* seldom referenced */
>> 
>> #ifdef CONFIG_FRONTSWAP
>> 
>> > +	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
>> > +	unsigned int frontswap_pages;	/* frontswap pages in-use counter */
>> 
>> 
>> #endif
>> 
>> (to eliminate any overhead with that config option unset)
>> 
>> Jan
> 
> Hi Jan --
> 
> Thanks for the review!
> 
> As noted in the commit comment, if these structure elements are
> not put inside an #ifdef CONFIG_FRONTSWAP, it becomes
> unnecessary to clutter the core swap code with several ifdefs.
> The cost is one pointer and one unsigned int per allocated
> swap device (often no more than one swap device per system),
> so the code clarity seemed more important than the tiny
> additional runtime space cost.
> 
> Do you disagree?

Not necessarily - I just know that in other similar occasions (partly
internally to our company) I was asked to make sure turned off
features would not leave *any* run time foot print whatsoever.

Jan

RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Jan Beulich]

>>> On 09.08.11 at 17:03, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
>> > --- linux/include/linux/swap.h	2011-08-08 08:19:25.880690134 -0600
>> > +++ frontswap/include/linux/swap.h	2011-08-08 08:59:03.952691415 -0600
>> > @@ -194,6 +194,8 @@ struct swap_info_struct {
>> >  	struct block_device *bdev;	/* swap device or bdev of swap file */
>> >  	struct file *swap_file;		/* seldom referenced */
>> >  	unsigned int old_block_size;	/* seldom referenced */
>> 
>> #ifdef CONFIG_FRONTSWAP
>> 
>> > +	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
>> > +	unsigned int frontswap_pages;	/* frontswap pages in-use counter */
>> 
>> 
>> #endif
>> 
>> (to eliminate any overhead with that config option unset)
>> 
>> Jan
> 
> Hi Jan --
> 
> Thanks for the review!
> 
> As noted in the commit comment, if these structure elements are
> not put inside an #ifdef CONFIG_FRONTSWAP, it becomes
> unnecessary to clutter the core swap code with several ifdefs.
> The cost is one pointer and one unsigned int per allocated
> swap device (often no more than one swap device per system),
> so the code clarity seemed more important than the tiny
> additional runtime space cost.
> 
> Do you disagree?

Not necessarily - I just know that in other similar occasions (partly
internally to our company) I was asked to make sure turned off
features would not leave *any* run time foot print whatsoever.

Jan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Dan Magenheimer]

> From: Jan Beulich [mailto:JBeulich@novell.com]
> Subject: RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes
> 
> >>> On 09.08.11 at 17:03, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> >> > --- linux/include/linux/swap.h	2011-08-08 08:19:25.880690134 -0600
> >> > +++ frontswap/include/linux/swap.h	2011-08-08 08:59:03.952691415 -0600
> >> > @@ -194,6 +194,8 @@ struct swap_info_struct {
> >> >  	struct block_device *bdev;	/* swap device or bdev of swap file */
> >> >  	struct file *swap_file;		/* seldom referenced */
> >> >  	unsigned int old_block_size;	/* seldom referenced */
> >>
> >> #ifdef CONFIG_FRONTSWAP
> >>
> >> > +	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
> >> > +	unsigned int frontswap_pages;	/* frontswap pages in-use counter */
> >>
> >> #endif
> >>
> >> (to eliminate any overhead with that config option unset)
> >>
> >> Jan
> >
> > Hi Jan --
> >
> > Thanks for the review!
> >
> > As noted in the commit comment, if these structure elements are
> > not put inside an #ifdef CONFIG_FRONTSWAP, it becomes
> > unnecessary to clutter the core swap code with several ifdefs.
> > The cost is one pointer and one unsigned int per allocated
> > swap device (often no more than one swap device per system),
> > so the code clarity seemed more important than the tiny
> > additional runtime space cost.
> >
> > Do you disagree?
> 
> Not necessarily - I just know that in other similar occasions (partly
> internally to our company) I was asked to make sure turned off
> features would not leave *any* run time foot print whatsoever.
> 
> Jan

Well I tried adding the ifdef to the structure as you suggested
and it requires three instances of "#ifdef CONFIG_FRONTSWAP"
in mm/swapfile.c.  BUT unless I get into massive code duplication
it still leaves a runtime footprint as extra parameters are passed
to enable_swap_info(), try_to_unuse(), and find_next_to_unuse();
so the intent to achieve zero runtime footprint is illusory.
I expect "absolutely zero runtime footprint" is a goal that very
very few significant new features achieve.

That said, frontswap and cleancache are designed so that they
SHOULD be config=y by default for most distros.  Unless a
module (e.g. zcache or tmem) registers the callbacks, the
overhead is very nearly zero... but if they are config=n,
then a module cannot use them at all.  This would be unfortunate
because the potential performance gain is not insignificant.
I would have preferred them to be merged with default of config=y,
but Linus disabused me of that notion :-}

Anyway, unless you feel very strongly about this, I'm
inclined to not add the ifdef to the struct for the
reasons previously stated.

Dan

RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Dan Magenheimer]

> From: Jan Beulich [mailto:JBeulich@novell.com]
> Subject: RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes
> 
> >>> On 09.08.11 at 17:03, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> >> > --- linux/include/linux/swap.h	2011-08-08 08:19:25.880690134 -0600
> >> > +++ frontswap/include/linux/swap.h	2011-08-08 08:59:03.952691415 -0600
> >> > @@ -194,6 +194,8 @@ struct swap_info_struct {
> >> >  	struct block_device *bdev;	/* swap device or bdev of swap file */
> >> >  	struct file *swap_file;		/* seldom referenced */
> >> >  	unsigned int old_block_size;	/* seldom referenced */
> >>
> >> #ifdef CONFIG_FRONTSWAP
> >>
> >> > +	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
> >> > +	unsigned int frontswap_pages;	/* frontswap pages in-use counter */
> >>
> >> #endif
> >>
> >> (to eliminate any overhead with that config option unset)
> >>
> >> Jan
> >
> > Hi Jan --
> >
> > Thanks for the review!
> >
> > As noted in the commit comment, if these structure elements are
> > not put inside an #ifdef CONFIG_FRONTSWAP, it becomes
> > unnecessary to clutter the core swap code with several ifdefs.
> > The cost is one pointer and one unsigned int per allocated
> > swap device (often no more than one swap device per system),
> > so the code clarity seemed more important than the tiny
> > additional runtime space cost.
> >
> > Do you disagree?
> 
> Not necessarily - I just know that in other similar occasions (partly
> internally to our company) I was asked to make sure turned off
> features would not leave *any* run time foot print whatsoever.
> 
> Jan

Well I tried adding the ifdef to the structure as you suggested
and it requires three instances of "#ifdef CONFIG_FRONTSWAP"
in mm/swapfile.c.  BUT unless I get into massive code duplication
it still leaves a runtime footprint as extra parameters are passed
to enable_swap_info(), try_to_unuse(), and find_next_to_unuse();
so the intent to achieve zero runtime footprint is illusory.
I expect "absolutely zero runtime footprint" is a goal that very
very few significant new features achieve.

That said, frontswap and cleancache are designed so that they
SHOULD be config=y by default for most distros.  Unless a
module (e.g. zcache or tmem) registers the callbacks, the
overhead is very nearly zero... but if they are config=n,
then a module cannot use them at all.  This would be unfortunate
because the potential performance gain is not insignificant.
I would have preferred them to be merged with default of config=y,
but Linus disabused me of that notion :-}

Anyway, unless you feel very strongly about this, I'm
inclined to not add the ifdef to the struct for the
reasons previously stated.

Dan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Jan Beulich]

>>> On 09.08.11 at 19:43, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> Anyway, unless you feel very strongly about this, I'm
> inclined to not add the ifdef to the struct for the
> reasons previously stated.

No, I don't feel really strongly about this - it you can get it accepted with
the minor overhead, that's fine to me. It's just that for integration into
our kernels (i.e. until these get accepted upstream) I chose to do those
adjustments to avoid possible complaints.

Jan

RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Jan Beulich]

>>> On 09.08.11 at 19:43, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> Anyway, unless you feel very strongly about this, I'm
> inclined to not add the ifdef to the struct for the
> reasons previously stated.

No, I don't feel really strongly about this - it you can get it accepted with
the minor overhead, that's fine to me. It's just that for integration into
our kernels (i.e. until these get accepted upstream) I chose to do those
adjustments to avoid possible complaints.

Jan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Dan Magenheimer]

> From: Jan Beulich [mailto:JBeulich@novell.com]
> Sent: Tuesday, August 09, 2011 10:18 AM
> To: Dan Magenheimer
> Cc: hannes@cmpxchg.org; jackdachef@gmail.com; hughd@google.com; jeremy@goop.org; npiggin@kernel.dk;
> linux-mm@kvack.org; akpm@linux-foundation.org; sjenning@linux.vnet.ibm.com; Chris Mason; Konrad Wilk;
> Kurt Hackel; riel@redhat.com; ngupta@vflare.org; linux-kernel@vger.kernel.org; matthew@wil.cx
> Subject: RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes
> 
> >>> On 09.08.11 at 17:03, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> >> > --- linux/include/linux/swap.h	2011-08-08 08:19:25.880690134 -0600
> >> > +++ frontswap/include/linux/swap.h	2011-08-08 08:59:03.952691415 -0600
> >> > @@ -194,6 +194,8 @@ struct swap_info_struct {
> >> >  	struct block_device *bdev;	/* swap device or bdev of swap file */
> >> >  	struct file *swap_file;		/* seldom referenced */
> >> >  	unsigned int old_block_size;	/* seldom referenced */
> >>
> >> #ifdef CONFIG_FRONTSWAP
> >>
> >> > +	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
> >> > +	unsigned int frontswap_pages;	/* frontswap pages in-use counter */
> >>
> >> #endif
> >>
> >> (to eliminate any overhead with that config option unset)
> >>
> >> Jan
> >
> > Hi Jan --
> >
> > Thanks for the review!
> >
> > As noted in the commit comment, if these structure elements are
> > not put inside an #ifdef CONFIG_FRONTSWAP, it becomes
> > unnecessary to clutter the core swap code with several ifdefs.
> > The cost is one pointer and one unsigned int per allocated
> > swap device (often no more than one swap device per system),
> > so the code clarity seemed more important than the tiny
> > additional runtime space cost.
> >
> > Do you disagree?
> 
> Not necessarily - I just know that in other similar occasions (partly
> internally to our company) I was asked to make sure turned off
> features would not leave *any* run time foot print whatsoever.
> 
> Jan

Hi Jan --

With two extra static inlines in frontswap.h (frontswap_map_get()
and frontswap_map_set(), I've managed to both avoid the extra swap struct
members for frontswap_map and frontswap_pages when CONFIG_FRONTSWAP is
disabled AND avoid the #ifdef CONFIG_FRONTSWAP clutter in swapfile.h.

I'll post a V7 soon... let me know what you think!

Thanks,
Dan

RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Dan Magenheimer]

> From: Jan Beulich [mailto:JBeulich@novell.com]
> Sent: Tuesday, August 09, 2011 10:18 AM
> To: Dan Magenheimer
> Cc: hannes@cmpxchg.org; jackdachef@gmail.com; hughd@google.com; jeremy@goop.org; npiggin@kernel.dk;
> linux-mm@kvack.org; akpm@linux-foundation.org; sjenning@linux.vnet.ibm.com; Chris Mason; Konrad Wilk;
> Kurt Hackel; riel@redhat.com; ngupta@vflare.org; linux-kernel@vger.kernel.org; matthew@wil.cx
> Subject: RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes
> 
> >>> On 09.08.11 at 17:03, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> >> > --- linux/include/linux/swap.h	2011-08-08 08:19:25.880690134 -0600
> >> > +++ frontswap/include/linux/swap.h	2011-08-08 08:59:03.952691415 -0600
> >> > @@ -194,6 +194,8 @@ struct swap_info_struct {
> >> >  	struct block_device *bdev;	/* swap device or bdev of swap file */
> >> >  	struct file *swap_file;		/* seldom referenced */
> >> >  	unsigned int old_block_size;	/* seldom referenced */
> >>
> >> #ifdef CONFIG_FRONTSWAP
> >>
> >> > +	unsigned long *frontswap_map;	/* frontswap in-use, one bit per page */
> >> > +	unsigned int frontswap_pages;	/* frontswap pages in-use counter */
> >>
> >> #endif
> >>
> >> (to eliminate any overhead with that config option unset)
> >>
> >> Jan
> >
> > Hi Jan --
> >
> > Thanks for the review!
> >
> > As noted in the commit comment, if these structure elements are
> > not put inside an #ifdef CONFIG_FRONTSWAP, it becomes
> > unnecessary to clutter the core swap code with several ifdefs.
> > The cost is one pointer and one unsigned int per allocated
> > swap device (often no more than one swap device per system),
> > so the code clarity seemed more important than the tiny
> > additional runtime space cost.
> >
> > Do you disagree?
> 
> Not necessarily - I just know that in other similar occasions (partly
> internally to our company) I was asked to make sure turned off
> features would not leave *any* run time foot print whatsoever.
> 
> Jan

Hi Jan --

With two extra static inlines in frontswap.h (frontswap_map_get()
and frontswap_map_set(), I've managed to both avoid the extra swap struct
members for frontswap_map and frontswap_pages when CONFIG_FRONTSWAP is
disabled AND avoid the #ifdef CONFIG_FRONTSWAP clutter in swapfile.h.

I'll post a V7 soon... let me know what you think!

Thanks,
Dan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Jan Beulich]

>>> On 22.08.11 at 19:08, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> With two extra static inlines in frontswap.h (frontswap_map_get()
> and frontswap_map_set(), I've managed to both avoid the extra swap struct
> members for frontswap_map and frontswap_pages when CONFIG_FRONTSWAP is
> disabled AND avoid the #ifdef CONFIG_FRONTSWAP clutter in swapfile.h.
> 
> I'll post a V7 soon... let me know what you think!

Sounds promising - looking forward to seeing it.

Jan

RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes

[Jan Beulich]

>>> On 22.08.11 at 19:08, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> With two extra static inlines in frontswap.h (frontswap_map_get()
> and frontswap_map_set(), I've managed to both avoid the extra swap struct
> members for frontswap_map and frontswap_pages when CONFIG_FRONTSWAP is
> disabled AND avoid the #ifdef CONFIG_FRONTSWAP clutter in swapfile.h.
> 
> I'll post a V7 soon... let me know what you think!

Sounds promising - looking forward to seeing it.

Jan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>