[PATCH 1/2] [mmotm] Add notifiers for various swap events

[PATCH 1/2] [mmotm] Add notifiers for various swap events

[Nitin Gupta]

Events:
 - Swapon
 - Swapoff
 - When a swap slot is freed

This is required for ramzswap module which implements RAM based block
devices to be used as swap disks. These devices require a notification
on these events to function properly.

Currently, I'm not sure if any of these event notifiers have any other
users. However, adding ramzswap specific hooks instead of this generic
approach resulted in a bad/hacky code.

For SWAP_EVENT_SLOT_FREE, callbacks are made under swap_lock. Currently, this
is not a problem since ramzswap is the only user and the callback it registers
can be safely made under this lock. However, if this event finds more users,
we might have to work on reducing contention on this lock (per-swap lock?).

Signed-off-by: Nitin Gupta <ngupta@vflare.org>
---
 include/linux/swap.h |   12 +++++++++
 mm/swapfile.c        |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 0 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index a2602a8..5fd2ac7 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -150,6 +150,12 @@ enum {
 	SWP_SCANNING	= (1 << 8),	/* refcount in scan_swap_map */
 };
 
+enum swap_event {
+	SWAP_EVENT_SWAPON,
+	SWAP_EVENT_SWAPOFF,
+	SWAP_EVENT_SLOT_FREE,
+};
+
 #define SWAP_CLUSTER_MAX 32
 
 #define SWAP_MAP_MAX	0x3e	/* Max duplication count, in first swap_map */
@@ -180,6 +186,7 @@ struct swap_info_struct {
 	struct swap_extent *curr_swap_extent;
 	struct swap_extent first_swap_extent;
 	struct block_device *bdev;	/* swap device or bdev of swap file */
+	struct atomic_notifier_head slot_free_notify_list;
 	struct file *swap_file;		/* seldom referenced */
 	unsigned int old_block_size;	/* seldom referenced */
 };
@@ -329,8 +336,13 @@ extern sector_t map_swap_page(struct page *, struct block_device **);
 extern sector_t swapdev_block(int, pgoff_t);
 extern int reuse_swap_page(struct page *);
 extern int try_to_free_swap(struct page *);
+extern int register_swap_event_notifier(struct notifier_block *nb,
+			enum swap_event event, unsigned long val);
+extern int unregister_swap_event_notifier(struct notifier_block *nb,
+			enum swap_event event, unsigned long val);
 struct backing_dev_info;
 
+
 /* linux/mm/thrash.c */
 extern struct mm_struct *swap_token_mm;
 extern void grab_swap_token(struct mm_struct *);
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 6c0585b..301905d 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -57,6 +57,9 @@ static struct swap_list_t swap_list = {-1, -1};
 static struct swap_info_struct *swap_info[MAX_SWAPFILES];
 
 static DEFINE_MUTEX(swapon_mutex);
+static BLOCKING_NOTIFIER_HEAD(swapon_notify_list);
+static BLOCKING_NOTIFIER_HEAD(swapoff_notify_list);
+
 
 static inline unsigned char swap_count(unsigned char ent)
 {
@@ -583,6 +586,8 @@ static unsigned char swap_entry_free(struct swap_info_struct *p,
 			swap_list.next = p->type;
 		nr_swap_pages++;
 		p->inuse_pages--;
+		atomic_notifier_call_chain(&p->slot_free_notify_list,
+					offset, p->swap_file);
 	}
 
 	return usage;
@@ -1609,6 +1614,7 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
 	p->swap_map = NULL;
 	p->flags = 0;
 	spin_unlock(&swap_lock);
+	blocking_notifier_call_chain(&swapoff_notify_list, type, swap_file);
 	mutex_unlock(&swapon_mutex);
 	vfree(swap_map);
 	/* Destroy swap account informatin */
@@ -2022,7 +2028,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
 		swap_list.head = swap_list.next = type;
 	else
 		swap_info[prev]->next = type;
+	ATOMIC_INIT_NOTIFIER_HEAD(&p->slot_free_notify_list);
 	spin_unlock(&swap_lock);
+	blocking_notifier_call_chain(&swapon_notify_list, type, swap_file);
 	mutex_unlock(&swapon_mutex);
 	error = 0;
 	goto out;
@@ -2446,3 +2454,62 @@ static void free_swap_count_continuations(struct swap_info_struct *si)
 		}
 	}
 }
+
+int register_swap_event_notifier(struct notifier_block *nb,
+				enum swap_event event, unsigned long val)
+{
+	switch (event) {
+	case SWAP_EVENT_SWAPON:
+		return blocking_notifier_chain_register(
+					&swapon_notify_list, nb);
+	case SWAP_EVENT_SWAPOFF:
+		return blocking_notifier_chain_register(
+					&swapoff_notify_list, nb);
+	case SWAP_EVENT_SLOT_FREE:
+		{
+		struct swap_info_struct *sis;
+
+		if (val > nr_swapfiles)
+			goto out;
+		sis = swap_info[val];
+		return atomic_notifier_chain_register(
+				&sis->slot_free_notify_list, nb);
+		}
+	default:
+		pr_err("Invalid swap event: %d\n", event);
+	};
+
+out:
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(register_swap_event_notifier);
+
+int unregister_swap_event_notifier(struct notifier_block *nb,
+				enum swap_event event, unsigned long val)
+{
+	switch (event) {
+	case SWAP_EVENT_SWAPON:
+		return blocking_notifier_chain_unregister(
+					&swapon_notify_list, nb);
+	case SWAP_EVENT_SWAPOFF:
+		return blocking_notifier_chain_unregister(
+					&swapoff_notify_list, nb);
+	case SWAP_EVENT_SLOT_FREE:
+		{
+		struct swap_info_struct *sis;
+
+		if (val > nr_swapfiles)
+			goto out;
+		sis = swap_info[val];
+		return atomic_notifier_chain_unregister(
+				&sis->slot_free_notify_list, nb);
+		}
+	default:
+		pr_err("Invalid swap event: %d\n", event);
+	};
+
+out:
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(unregister_swap_event_notifier);
+
-- 
1.6.2.5

[PATCH 2/2] [mmotm] ramzswap: add handlers for various swap events

[Nitin Gupta]

In particular, add handler for SWAP_EVENT_SLOT_FREE event.
The handler frees memory associated with given swap slot,
eliminating any stale data in corresponding ramzswap device.

Signed-off-by: Nitin Gupta <ngupta@vflare.org>
---
 drivers/staging/ramzswap/ramzswap_drv.c   |   69 +++++++++++++++++++++++++++++
 drivers/staging/ramzswap/ramzswap_drv.h   |    1 +
 drivers/staging/ramzswap/ramzswap_ioctl.h |    1 +
 3 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/ramzswap/ramzswap_drv.c b/drivers/staging/ramzswap/ramzswap_drv.c
index b839f05..1808fdb 100644
--- a/drivers/staging/ramzswap/ramzswap_drv.c
+++ b/drivers/staging/ramzswap/ramzswap_drv.c
@@ -276,6 +276,7 @@ void ramzswap_ioctl_get_stats(struct ramzswap *rzs,
 	s->failed_reads = rs->failed_reads;
 	s->failed_writes = rs->failed_writes;
 	s->invalid_io = rs->invalid_io;
+	s->notify_free = rs->notify_free;
 	s->pages_zero = rs->pages_zero;
 
 	s->good_compress_pct = good_compress_perc;
@@ -1366,6 +1367,60 @@ static void destroy_device(struct ramzswap *rzs)
 	if (rzs->queue)
 		blk_cleanup_queue(rzs->queue);
 }
+static int ramzswap_slot_free_notify(struct notifier_block *self,
+			unsigned long index, void *swap_file)
+{
+	struct ramzswap *rzs;
+
+	rzs = ((struct file *)swap_file)->private_data;
+	ramzswap_free_page(rzs, index);
+	stat_inc(rzs->stats.notify_free);
+	return 0;
+}
+
+static struct notifier_block ramzswap_slot_free_nb = {
+	.notifier_call = ramzswap_slot_free_notify
+};
+
+static int ramzswap_swapon_notify(struct notifier_block *self,
+			unsigned long swap_id, void *swap_file)
+{
+	int ret = 0;
+	struct block_device *bdev;
+	struct file *file;
+	struct inode *inode;
+	struct ramzswap *rzs;
+
+	/* cache ramzswap struct associated with this swap_file */
+	file = (struct file *)swap_file;
+	inode = file->f_mapping->host;
+	bdev = I_BDEV(inode);
+	rzs = bdev->bd_disk->private_data;
+	file->private_data = rzs;
+
+	ret = register_swap_event_notifier(&ramzswap_slot_free_nb,
+				SWAP_EVENT_SLOT_FREE, swap_id);
+	if (ret)
+		pr_err("Error registering swap free notifier\n");
+	return ret;
+}
+
+static int ramzswap_swapoff_notify(struct notifier_block *self,
+			unsigned long swap_id, void *swap_file)
+{
+	unregister_swap_event_notifier(&ramzswap_slot_free_nb,
+				SWAP_EVENT_SLOT_FREE, swap_id);
+	return 0;
+}
+
+
+static struct notifier_block ramzswap_swapon_nb = {
+	.notifier_call = ramzswap_swapon_notify
+};
+
+static struct notifier_block ramzswap_swapoff_nb = {
+	.notifier_call = ramzswap_swapoff_notify
+};
 
 static int __init ramzswap_init(void)
 {
@@ -1399,6 +1454,20 @@ static int __init ramzswap_init(void)
 	for (i = 0; i < num_devices; i++)
 		create_device(&devices[i], i);
 
+	ret = register_swap_event_notifier(&ramzswap_swapon_nb,
+				SWAP_EVENT_SWAPON, 0);
+	if (ret) {
+		pr_err("Error registering swapon notifier\n");
+		goto out;
+	}
+
+	ret = register_swap_event_notifier(&ramzswap_swapoff_nb,
+				SWAP_EVENT_SWAPOFF, 0);
+	if (ret) {
+		pr_err("Error registering swapoff notifier\n");
+		goto out;
+	}
+
 	return 0;
 out:
 	unregister_blkdev(ramzswap_major, "ramzswap");
diff --git a/drivers/staging/ramzswap/ramzswap_drv.h b/drivers/staging/ramzswap/ramzswap_drv.h
index a6ea240..adc841a 100644
--- a/drivers/staging/ramzswap/ramzswap_drv.h
+++ b/drivers/staging/ramzswap/ramzswap_drv.h
@@ -124,6 +124,7 @@ struct ramzswap_stats {
 	u64 failed_reads;	/* can happen when memory is too low */
 	u64 failed_writes;	/* should NEVER! happen */
 	u64 invalid_io;		/* non-swap I/O requests */
+	u64 notify_free;	/* no. of swap slot free notifications */
 	u32 pages_zero;		/* no. of zero filled pages */
 	u32 pages_stored;	/* no. of pages currently stored */
 	u32 good_compress;	/* % of pages with compression ratio<=50% */
diff --git a/drivers/staging/ramzswap/ramzswap_ioctl.h b/drivers/staging/ramzswap/ramzswap_ioctl.h
index c713a09..ec50416 100644
--- a/drivers/staging/ramzswap/ramzswap_ioctl.h
+++ b/drivers/staging/ramzswap/ramzswap_ioctl.h
@@ -27,6 +27,7 @@ struct ramzswap_ioctl_stats {
 	u64 failed_reads;	/* can happen when memory is too low */
 	u64 failed_writes;	/* should NEVER! happen */
 	u64 invalid_io;		/* non-swap I/O requests */
+	u64 notify_free;	/* no. of swap slot free notifications */
 	u32 pages_zero;		/* no. of zero filled pages */
 	u32 good_compress_pct;	/* no. of pages with compression ratio<=50% */
 	u32 pages_expand_pct;	/* no. of incompressible pages */
-- 
1.6.2.5

Re: [PATCH 1/2] [mmotm] Add notifiers for various swap events

[Pekka Enberg]

On Mon, Dec 28, 2009 at 3:19 AM, Nitin Gupta <ngupta@vflare.org> wrote:
> Events:
>  - Swapon
>  - Swapoff
>  - When a swap slot is freed
>
> This is required for ramzswap module which implements RAM based block
> devices to be used as swap disks. These devices require a notification
> on these events to function properly.
>
> Currently, I'm not sure if any of these event notifiers have any other
> users. However, adding ramzswap specific hooks instead of this generic
> approach resulted in a bad/hacky code.
>
> For SWAP_EVENT_SLOT_FREE, callbacks are made under swap_lock. Currently, this
> is not a problem since ramzswap is the only user and the callback it registers
> can be safely made under this lock. However, if this event finds more users,
> we might have to work on reducing contention on this lock (per-swap lock?).
>
> Signed-off-by: Nitin Gupta <ngupta@vflare.org>

This looks good to me. Hugh? Nitin, does this patch cause any
performance hit for the !CONFIG_RAMZSWAP case?

> ---
>  include/linux/swap.h |   12 +++++++++
>  mm/swapfile.c        |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 79 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index a2602a8..5fd2ac7 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -150,6 +150,12 @@ enum {
>        SWP_SCANNING    = (1 << 8),     /* refcount in scan_swap_map */
>  };
>
> +enum swap_event {
> +       SWAP_EVENT_SWAPON,
> +       SWAP_EVENT_SWAPOFF,
> +       SWAP_EVENT_SLOT_FREE,
> +};
> +
>  #define SWAP_CLUSTER_MAX 32
>
>  #define SWAP_MAP_MAX   0x3e    /* Max duplication count, in first swap_map */
> @@ -180,6 +186,7 @@ struct swap_info_struct {
>        struct swap_extent *curr_swap_extent;
>        struct swap_extent first_swap_extent;
>        struct block_device *bdev;      /* swap device or bdev of swap file */
> +       struct atomic_notifier_head slot_free_notify_list;
>        struct file *swap_file;         /* seldom referenced */
>        unsigned int old_block_size;    /* seldom referenced */
>  };
> @@ -329,8 +336,13 @@ extern sector_t map_swap_page(struct page *, struct block_device **);
>  extern sector_t swapdev_block(int, pgoff_t);
>  extern int reuse_swap_page(struct page *);
>  extern int try_to_free_swap(struct page *);
> +extern int register_swap_event_notifier(struct notifier_block *nb,
> +                       enum swap_event event, unsigned long val);
> +extern int unregister_swap_event_notifier(struct notifier_block *nb,
> +                       enum swap_event event, unsigned long val);
>  struct backing_dev_info;
>
> +
>  /* linux/mm/thrash.c */
>  extern struct mm_struct *swap_token_mm;
>  extern void grab_swap_token(struct mm_struct *);
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 6c0585b..301905d 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -57,6 +57,9 @@ static struct swap_list_t swap_list = {-1, -1};
>  static struct swap_info_struct *swap_info[MAX_SWAPFILES];
>
>  static DEFINE_MUTEX(swapon_mutex);
> +static BLOCKING_NOTIFIER_HEAD(swapon_notify_list);
> +static BLOCKING_NOTIFIER_HEAD(swapoff_notify_list);
> +
>
>  static inline unsigned char swap_count(unsigned char ent)
>  {
> @@ -583,6 +586,8 @@ static unsigned char swap_entry_free(struct swap_info_struct *p,
>                        swap_list.next = p->type;
>                nr_swap_pages++;
>                p->inuse_pages--;
> +               atomic_notifier_call_chain(&p->slot_free_notify_list,
> +                                       offset, p->swap_file);
>        }
>
>        return usage;
> @@ -1609,6 +1614,7 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
>        p->swap_map = NULL;
>        p->flags = 0;
>        spin_unlock(&swap_lock);
> +       blocking_notifier_call_chain(&swapoff_notify_list, type, swap_file);
>        mutex_unlock(&swapon_mutex);
>        vfree(swap_map);
>        /* Destroy swap account informatin */
> @@ -2022,7 +2028,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>                swap_list.head = swap_list.next = type;
>        else
>                swap_info[prev]->next = type;
> +       ATOMIC_INIT_NOTIFIER_HEAD(&p->slot_free_notify_list);
>        spin_unlock(&swap_lock);
> +       blocking_notifier_call_chain(&swapon_notify_list, type, swap_file);
>        mutex_unlock(&swapon_mutex);
>        error = 0;
>        goto out;
> @@ -2446,3 +2454,62 @@ static void free_swap_count_continuations(struct swap_info_struct *si)
>                }
>        }
>  }
> +
> +int register_swap_event_notifier(struct notifier_block *nb,
> +                               enum swap_event event, unsigned long val)
> +{
> +       switch (event) {
> +       case SWAP_EVENT_SWAPON:
> +               return blocking_notifier_chain_register(
> +                                       &swapon_notify_list, nb);
> +       case SWAP_EVENT_SWAPOFF:
> +               return blocking_notifier_chain_register(
> +                                       &swapoff_notify_list, nb);
> +       case SWAP_EVENT_SLOT_FREE:
> +               {
> +               struct swap_info_struct *sis;
> +
> +               if (val > nr_swapfiles)
> +                       goto out;
> +               sis = swap_info[val];
> +               return atomic_notifier_chain_register(
> +                               &sis->slot_free_notify_list, nb);
> +               }
> +       default:
> +               pr_err("Invalid swap event: %d\n", event);
> +       };
> +
> +out:
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(register_swap_event_notifier);
> +
> +int unregister_swap_event_notifier(struct notifier_block *nb,
> +                               enum swap_event event, unsigned long val)
> +{
> +       switch (event) {
> +       case SWAP_EVENT_SWAPON:
> +               return blocking_notifier_chain_unregister(
> +                                       &swapon_notify_list, nb);
> +       case SWAP_EVENT_SWAPOFF:
> +               return blocking_notifier_chain_unregister(
> +                                       &swapoff_notify_list, nb);
> +       case SWAP_EVENT_SLOT_FREE:
> +               {
> +               struct swap_info_struct *sis;
> +
> +               if (val > nr_swapfiles)
> +                       goto out;
> +               sis = swap_info[val];
> +               return atomic_notifier_chain_unregister(
> +                               &sis->slot_free_notify_list, nb);
> +               }
> +       default:
> +               pr_err("Invalid swap event: %d\n", event);
> +       };
> +
> +out:
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(unregister_swap_event_notifier);
> +
> --
> 1.6.2.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 1/2] [mmotm] Add notifiers for various swap events

[Nitin Gupta]

On Mon, Dec 28, 2009 at 11:43 AM, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> On Mon, Dec 28, 2009 at 3:19 AM, Nitin Gupta <ngupta@vflare.org> wrote:
>> Events:
>>  - Swapon
>>  - Swapoff
>>  - When a swap slot is freed

<snip>

> This looks good to me. Hugh? Nitin, does this patch cause any
> performance hit for the !CONFIG_RAMZSWAP case?
>

I have not yet collected any data to check overhead in !RAMZSWAP
case but an empty notifier chain is essentially equivalent to:

rcu_read_lock()
        nb = rcu_dereference(*nl);
rcu_read_unlock()

This should not cause any performance problem. Anyway, I will
try to come up with some numbers to confirm this.

Thanks for looking into this.
Nitin

Re: [PATCH 1/2] [mmotm] Add notifiers for various swap events

[Andi Kleen]

Nitin Gupta <ngupta@vflare.org> writes:

> Events:
>  - Swapon
>  - Swapoff
>  - When a swap slot is freed
>
> This is required for ramzswap module which implements RAM based block
> devices to be used as swap disks. These devices require a notification
> on these events to function properly.

The first question to ask is if compressed swap is worth
it. Do you have benchmark numbers showing it to be an improvement?
Are there cases where it is slower than uncompressed swap?


> Currently, I'm not sure if any of these event notifiers have any other
> users. However, adding ramzswap specific hooks instead of this generic
> approach resulted in a bad/hacky code.

If there's only a single user I think it's preferable to call
directly. That makes the code much easier to read and understand.
In the end notifiers are a form of code obfuscation.

The main use for notifiers would be if something is a optional
module, but that's not the case here.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 1/2] [mmotm] Add notifiers for various swap events

[Nitin Gupta]

On 12/28/2009 08:32 PM, Andi Kleen wrote:
> Nitin Gupta <ngupta@vflare.org> writes:
> 
>> Events:
>>  - Swapon
>>  - Swapoff
>>  - When a swap slot is freed
>>
>> This is required for ramzswap module which implements RAM based block
>> devices to be used as swap disks. These devices require a notification
>> on these events to function properly.
> 
> The first question to ask is if compressed swap is worth
> it. Do you have benchmark numbers showing it to be an improvement?
> Are there cases where it is slower than uncompressed swap?
> 

http://code.google.com/p/compcache/wiki/Performance
This contains data for both positive and negative cases.
Also, it is currently being used on (unofficial) Android builds where
it shows noticeable performance gains compared to plain SSD swap.

> 
>> Currently, I'm not sure if any of these event notifiers have any other
>> users. However, adding ramzswap specific hooks instead of this generic
>> approach resulted in a bad/hacky code.
> 
> If there's only a single user I think it's preferable to call
> directly. That makes the code much easier to read and understand.
> In the end notifiers are a form of code obfuscation.
> 

Adding ramzswap code in place of generic swap slot free notification
is bad since ramzswap is just another module and might not be selected
for compilation. So, the code will include some unnecessary #ifdef'ery.

I think SLOT_FREE notifier is the major worry which is surely too
ramzswap specific (who else would want notification so often).

However, SWAPON and SWAPOFF events are fairly generic and (I hope) should
find some more users in future. If its okay to keep just these two notifiers,
I will replace SWAP_NOTIFY notifier with a simple callback function in
swap_info_struct. In future, if this event finds more users, I will revert
back to this generic notifier.

> The main use for notifiers would be if something is a optional
> module, but that's not the case here.


ramzswap is an optional module.

Thanks,
Nitin

Re: [PATCH 2/2] [mmotm] ramzswap: add handlers for various swap events

[Yin Kangkai]

On 2009-12-28, 06:50 +0530, Nitin Gupta wrote:
> +	ret = register_swap_event_notifier(&ramzswap_swapon_nb,
> +				SWAP_EVENT_SWAPON, 0);
> +	if (ret) {
> +		pr_err("Error registering swapon notifier\n");
> +		goto out;
> +	}
> +
> +	ret = register_swap_event_notifier(&ramzswap_swapoff_nb,
> +				SWAP_EVENT_SWAPOFF, 0);
> +	if (ret) {

Sorry, but an unregister here?

> +		pr_err("Error registering swapoff notifier\n");
> +		goto out;
> +	}
> +

Re: [PATCH 1/2] [mmotm] Add notifiers for various swap events

[Andi Kleen]

> > The first question to ask is if compressed swap is worth
> > it. Do you have benchmark numbers showing it to be an improvement?
> > Are there cases where it is slower than uncompressed swap?
> > 
> 
> http://code.google.com/p/compcache/wiki/Performance

That should be included in the changelog of the patches.

> ramzswap is an optional module.

I have some doubts on the wisdom of making swap algorithms modular.
Better compile them in. Then you don't need messy notifiers either.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 1/2] [mmotm] Add notifiers for various swap events

[Pekka Enberg]

Hi Andi,

Andi Kleen wrote:
>>> The first question to ask is if compressed swap is worth
>>> it. Do you have benchmark numbers showing it to be an improvement?
>>> Are there cases where it is slower than uncompressed swap?
>>>
>> http://code.google.com/p/compcache/wiki/Performance
> 
> That should be included in the changelog of the patches.

Which patches? The driver is already in staging and there's a pointer to 
the home page in

   linux/drivers/staging/ramzswap/ramzswap.txt

>> ramzswap is an optional module.
> 
> I have some doubts on the wisdom of making swap algorithms modular.
> Better compile them in. Then you don't need messy notifiers either.

What's so messy about them? The whole point of having the notifiers is 
to avoid CONFIG_RAMZSWAP in core kernel code...

			Pekka

Re: [PATCH 1/2] [mmotm] Add notifiers for various swap events

[Andi Kleen]

>>>> The first question to ask is if compressed swap is worth
>>>> it. Do you have benchmark numbers showing it to be an improvement?
>>>> Are there cases where it is slower than uncompressed swap?
>>>>
>>> http://code.google.com/p/compcache/wiki/Performance
>>
>> That should be included in the changelog of the patches.
>
> Which patches? The driver is already in staging and there's a pointer to 
> the home page in

How can it be in staging if there are no hooks for it yet?

>>> ramzswap is an optional module.
>>
>> I have some doubts on the wisdom of making swap algorithms modular.
>> Better compile them in. Then you don't need messy notifiers either.
>
> What's so messy about them? The whole point of having the notifiers is to 
> avoid CONFIG_RAMZSWAP in core kernel code...

They make the code much harder to read and follow. When you try to follow
the code flow and you find a notifier it's always a complicated operation
to figure out what code will end up being called.

Sometimes they are needed, but they have a high cost in maintainability.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 1/2] [mmotm] Add notifiers for various swap events

[Nitin Gupta]

On 12/29/2009 12:53 AM, Andi Kleen wrote:
>>>>> The first question to ask is if compressed swap is worth
>>>>> it. Do you have benchmark numbers showing it to be an improvement?
>>>>> Are there cases where it is slower than uncompressed swap?
>>>>>
>>>> http://code.google.com/p/compcache/wiki/Performance
>>>
>>> That should be included in the changelog of the patches.
>>
>> Which patches? The driver is already in staging and there's a pointer to 
>> the home page in
> 
> How can it be in staging if there are no hooks for it yet?
> 
>>>> ramzswap is an optional module.
>>>
>>> I have some doubts on the wisdom of making swap algorithms modular.
>>> Better compile them in. Then you don't need messy notifiers either.

These notifiers are the only in-kernel changes ramzswap needs. Giving away
modularity just because of this might not be a good idea. Also, we are
not making any swap algorithm modular. This driver simply creates virtual
block devices to be used as swap disks. This approach allows seamless
integration with existing swap code and 0 overhead when not used.

>>
>> What's so messy about them? The whole point of having the notifiers is to 
>> avoid CONFIG_RAMZSWAP in core kernel code...
> 
> They make the code much harder to read and follow. When you try to follow
> the code flow and you find a notifier it's always a complicated operation
> to figure out what code will end up being called.
> 
> Sometimes they are needed, but they have a high cost in maintainability.
> 

For SWAP_SLOT_FREE notification, I can add a comment mentioning that ramzswap is
currently the only user. Other two notifiers, for swapon and swapoff, are fairly
generic and I hope will find some more users soon.
Maybe I'm missing something but simple cscope query for notifier chain name will
instantly reveal all its users :)

If we remove _any_ of these notifiers, we will end up with lot of ramzswap #ifdef'ery
all over the core kernel code, as Pekka mentioned. This includes swapon/swapoff syscalls(!),
 swap.h, swapfile.c. In fact, some of my initial patches (before mainline merge)
did the same thing but they were obviously too ugly/hacky with CONFIG_RAMZSWAP all over
the place due to which I had to temporarily drop swap notify callback support which is
essential for this driver.

Thanks,
Nitin

Re: [PATCH 2/2] [mmotm] ramzswap: add handlers for various swap events

[Nitin Gupta]

On 12/28/2009 09:56 PM, Yin Kangkai wrote:
> On 2009-12-28, 06:50 +0530, Nitin Gupta wrote:
>> +	ret = register_swap_event_notifier(&ramzswap_swapon_nb,
>> +				SWAP_EVENT_SWAPON, 0);
>> +	if (ret) {
>> +		pr_err("Error registering swapon notifier\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = register_swap_event_notifier(&ramzswap_swapoff_nb,
>> +				SWAP_EVENT_SWAPOFF, 0);
>> +	if (ret) {
> 
> Sorry, but an unregister here?

Oh, I missed it. I will add it in next patch revision.

Thanks,
Nitin