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

[Nitin Gupta <ngupta@vflare.org>]

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