> From: Jan Beulich [mailto:JBeulich@xxxxxxxxxx] > Subject: RE: Subject: [PATCH V6 1/4] mm: frontswap: swap data structure changes > > >>> On 09.08.11 at 17:03, Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> 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@xxxxxxxxx. 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