On Wed, 14 Nov 2012 13:57:06 -0500
Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> wrote:
> From: Dan Magenheimer <dan.magenheimer@xxxxxxxxxx>
>
> With the goal of allowing tmem backends (zcache, ramster, Xen tmem) to be
> built/loaded as modules rather than built-in and enabled by a boot parameter,
> this patch provides "lazy initialization", allowing backends to register to
> frontswap even after swapon was run. Before a backend registers all calls
> to init are recorded and the creation of tmem_pools delayed until a backend
> registers or until a frontswap put is attempted.
>
>
> ...
>
> --- a/mm/frontswap.c
> +++ b/mm/frontswap.c
> @@ -80,6 +80,18 @@ static inline void inc_frontswap_succ_stores(void) { }
> static inline void inc_frontswap_failed_stores(void) { }
> static inline void inc_frontswap_invalidates(void) { }
> #endif
> +
> +/*
> + * When no backend is registered all calls to init are registered and
What is "init"? Spell it out fully, please.
> + * remembered but fail to create tmem_pools. When a backend registers with
> + * frontswap the previous calls to init are executed to create tmem_pools
> + * and set the respective poolids.
Again, seems really hacky. Why can't we just change callers so they
call things in the correct order?
> + * While no backend is registered all "puts", "gets" and "flushes" are
> + * ignored or fail.
> + */
> +static DECLARE_BITMAP(need_init, MAX_SWAPFILES);
> +static bool backend_registered __read_mostly;
> +
> /*
> * Register operations for frontswap, returning previous thus allowing
> * detection of multiple backends and possible nesting.
> @@ -87,9 +99,19 @@ static inline void inc_frontswap_invalidates(void) { }
> struct frontswap_ops frontswap_register_ops(struct frontswap_ops *ops)
> {
> struct frontswap_ops old = frontswap_ops;
> + int i;
>
> frontswap_ops = *ops;
> frontswap_enabled = true;
> +
> + for (i = 0; i < MAX_SWAPFILES; i++) {
> + if (test_and_clear_bit(i, need_init))
ooh, that wasn't racy ;)
> + (*frontswap_ops.init)(i);
> + }
> + /* We MUST have backend_registered called _after_ the frontswap_init's
> + * have been called. Otherwise __frontswap_store might fail. */
Comment makes no sense - backend_registered is not a function.
Also, let's lay the comments out conventionally please:
/*
* We MUST have backend_registered called _after_ the frontswap_init's
* have been called. Otherwise __frontswap_store might fail.
*/
> + barrier();
> + backend_registered = true;
> return old;
> }
> EXPORT_SYMBOL(frontswap_register_ops);
>
> ...
>
> @@ -226,12 +266,15 @@ void __frontswap_invalidate_area(unsigned type)
> {
> struct swap_info_struct *sis = swap_info[type];
>
> - BUG_ON(sis == NULL);
> - if (sis->frontswap_map == NULL)
> - return;
> - frontswap_ops.invalidate_area(type);
> - atomic_set(&sis->frontswap_pages, 0);
> - memset(sis->frontswap_map, 0, sis->max / sizeof(long));
> + if (backend_registered) {
> + BUG_ON(sis == NULL);
> + if (sis->frontswap_map == NULL)
> + return;
> + (*frontswap_ops.invalidate_area)(type);
> + atomic_set(&sis->frontswap_pages, 0);
> + memset(sis->frontswap_map, 0, sis->max / sizeof(long));
> + }
> + clear_bit(type, need_init);
> }
> EXPORT_SYMBOL(__frontswap_invalidate_area);
>
> @@ -364,6 +407,9 @@ static int __init init_frontswap(void)
> debugfs_create_u64("invalidates", S_IRUGO,
> root, &frontswap_invalidates);
> #endif
> + bitmap_zero(need_init, MAX_SWAPFILES);
unneeded?
> + frontswap_enabled = 1;
> return 0;
> }
>
> ...
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
