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

[Nitin Gupta <ngupta@vflare.org>]

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