linuxcompressed-devel

[lc-devel] Announce: Compressed cache 'pre-alpha-001' release :)

[Nitin G. <nit...@gm...>]

Hi All,

This is the patch for compressed caching feature against 2.6.18-rc2 :)
(Its taken from git branch 'simple_lock_structure')

* What is does
-- compress anon (only anon not page-cache pages now) pages cyclically
using WKdm and WK4x4.
-- Store these pages in compression structure as desc. on Wiki.

Do not expect any performance improvements now...I found it to be just
stable enough after integrating the compression structure. In fact, it
will give perf. heart attack to your system ;)  because of simplified
locking scheme it uses in lookup functions (I'm working on this).

Please let me know if it crashes on you system. I will keep
stabilizing it in 'simple_lock_structure' branch while I'm trying to
get better locking scheme working in 'better_locking' branch. Finally,
I'll merge these two, to get fully functional ccache for anon pages.
(I hope to have support for page cache pages shortly after that).

Cheers,
Nitin Gupta

Re: [lc-devel] [OLPC-devel] Announce: Compressed cache 'pre-alpha-001' release :)

[Nitin G. <nit...@gm...>]

On 7/26/06, Zach Brown <za...@za...> wrote:
> Nitin Gupta wrote:

First, Thanks for taking time to review this patch! :)

> >
> > This is the patch for compressed caching feature against 2.6.18-rc2 :)
>
> Has anyone reviewed this with you yet?

No. Not yet :/

>
> > -EXTRAVERSION = -rc2
> > +EXTRAVERSION = -rc2-cc
>
> You can avoid polluting the diff with this naming change by putting
> '-cc' in ./localversion
>

Ok.

> > +++ b/include/linux/ccache.h
> > @@ -0,0 +1,97 @@
> > +#ifndef _CCACHE_H
> > +#define _CCACHE_H
>
> It's unfortunate that we came up with a name that conflicts with the
> popular compiler cache tool.  Can we come up with a unique name?
>

No problem. __CCACHE_H, _CCACHE_H_, _COMP_CACHE_H_ ... :)


> > +#define ChunkFree(chunk)             \
> > +             test_bit(CHUNK_free, (unsigned long *)(&(chunk)->size))
>
> This is not safe.  By handing the *_bit() interface an unsigned long
> pointer to 'unsigned short size' you're risking corrupting the chunk
> members that are near the size member.  Make 'size' an unsigned long and
> get rid of these casts.
>

I need to keep 'struct chunk' as small as possible. This 'test_bit' usage is
just super blind copying of page-flags.h :)

I think this should do (atomic guarantee not required):

#define ChunkFree(chunk)		\
		(!!(chunk->size & (1 << CHUNK_FREE)))
#define SetChunkFree(chunk)		\
		(chunk->size |= (1 << CHUNK_FREE))
#define ClearChunkFree(chunk)		\
		(chunk->size &= ~(1 << CHUNK_FREE))


> > +/* error codes */
> > +#define CC_ENOMEM    1
> > +/* compression algo actually increased size! */
> > +#define CC_EEXPAND   2
> > +/* someone accessed page when we
> > + * were about to compress it! */
> > +#define CC_EUSED     3
>
> Please don't introduce your own error code namespace.  Typical practice
> is to use -ERRNO to indicate errors, 0 for success, and +ve for
> indicating work done.  Using a convention like that will make your code
> more digestible to other kernel maintainers.
>

I couldn't find equivalent error codes with meaning of CC_EEXPAND and CC_EUSED
so defined them here.	s/CC_ENOMEM/ENOMEM

> > +     atomic_t _count;                /* usage count; free this struct
> > +                                      * when count is 0 */
>
> Lose the '_'.
>

chunk_head is given to page_cache_get() before we know it's 'struct page' or
'struct chunk_head' so need to keep name and its position same as for struct
page. Why the 'struct page' has this '_' for count and mapcount?

>  Heck, maybe just remove it until it is actually used :).
>
I'm _now_ using it when improving locking in find_get_page() and friends.

> > -      },
> > -      {
> > +     },
> > +     {
>
> Don't mix changes like this in with the rest of the patch.  They just
> add background noise.

Ok :)

>
> > +int anon_cc_started=0;
> > +unsigned long max_anon_cc_size=0, max_fs_backed_cc_size=0;
> > +unsigned long anon_cc_size=0, fs_backed_cc_size=0;   /* current size */
>
> You don't need = 0 here, they'll be put in the BSS and zeroed if you
> don't define an initial value.
>

Ok...But how do you avoid compiler warnings? Though thats not an issue but
they look ugly when compiling :)


> > +const unsigned long ccache_size_limit = MAX_SWAP_OFFSET - 1;
> > +
> > +static struct list_head pages_head, mcl_head, lru_head;
>
> If you defined these with LIST_HEAD() then they would already be
> initialized and you wouldn't need INIT_LIST_HEAD() in init_ccache().
>

I'll do this.

> > +static int WKdm_compress_wrapper (unsigned char *src, unsigned long
src_len,
> > +                             unsigned char *dest, unsigned long *dest_len)
> > +{
> > +     //pr_info("%s called.\n", __FUNCTION__);
> > +     *dest_len=WKdm_compress((WK_word*)src, (WK_word*)dest, 1024);
> > +     return 0;
>
> I guess propagating these return codes will be fixed up eventually.

You mean return *dest_len ? I'll also remove src_len arg since its always
PAGE_SIZE.

> White space around '=', please:
>
>         *dest_len = WK..
>
> > +     if (ret==LZO_E_OK) pr_info("LZO compression done!\n");
> > +     else pr_info("LZO compression failed!\n");
>
> Use newlines after if/else throughout the code, please:
>
>         if (foo)
>                 doit;
>         else
>                 something;
>

Ok :)

> > +repeat:
> > +     spin_lock(&ccache_lock);
> > +     while (free_head) {
> > +             //pr_info("in get_enough_chunks loop: %d\n", tst++);
> > +             chunk = free_head;
> > +             free_head = chunk->next;
> > +             ClearChunkFree(chunk);
> > +             spin_unlock(&ccache_lock);
> > +             if (ChunkMerged(chunk)) {
> > +                     pr_info("merged chunk found: size=%d\n",
> > +                                     ChunkSize(chunk));
> > +                     kfree(chunk);
> > +                     continue;
>
> The ccache_lock needs to be reacquired before continuing so that it
> doesn't double-unlock.  You should perform your tests with lockdep
> enabled in the kernel config :).
>

Great catch!  I'll learn about lockdep :)


> > +int init_ccache(void)
> > +{
> > +     spin_lock(&ccache_lock);
> > +     INIT_LIST_HEAD(&pages_head);
> > +     INIT_LIST_HEAD(&lru_head);
> > +     INIT_LIST_HEAD(&mcl_head);
> > +     free_head = NULL;
> > +     spin_unlock(&ccache_lock);
>
> Hmmm.  What are these locks protecting?  If they're racing with users of
> the lists then they're racing with users who will see uninitialized
> lists :/.
>

I'll define these with LIST_HEAD() as you said.

These were protected since init_ccache() was called _after_ vswap was
activated. But now its called before vswap init, so these locks were not
required anyway :)


> > +     /* collect compressed page from chunks */
> > +     comp_page = alloc_page(GFP_ATOMIC);
> > +     if (!comp_page) {
> > +             pr_info("cc_readpage: comp_page alloc failed!!!\n");
> > +             BUG();
> > +     }
>
> GFP_ATOMIC allocations are expected to fail under load.  I assume this
> is just a temporary BUG() :).

Yes its temporary and I'm not sure how to handle this failed alloc!
If it fails, we lose the page since its copy is not stored in swap disk.
Also, lookup functions (I think) cannot sleep so GFP_KERNEL (WAIT | IO |
FS) can't be used either.

Which flags should be used here for alloc?


> > +int cc_writepage(struct page *page)
> > +{
> > +     int ret = -CC_ENOMEM;
> > +     //swp_entry_t entry;
> > +     int comp_size, algo_idx;
> > +     struct chunk_head *ch=0;
> > +     struct page *tmp_page=0, *new_page=0, **slot;
>
> Set pointers to NULL.  Have you run sparse against this code?  See
> Documentation/sparse.txt in the kernel tree.
>

Having trouble understanding what it is....Will read more on it.

> > +     struct address_space *mapping=page_mapping(page);
> > +
> > +     //ch = kmalloc(sizeof(struct chunk_head), GFP_KERNEL);
> > +     //if (!ch) goto out;
> > +     tmp_page = alloc_pages(GFP_KERNEL, 1);  /* page may expand */
>
> Do you mean to allocate two pages here?

Yes. Since compressed data might be bigger that original data, we need to pass
it extra space (2 pages) when compressing single page.

(If compressed size >= PAGE_SIZE it is just kept in natural form or send to
swap).

> You don't seem to use the
> second one and only free the first.
>

ah..that's a mistake:

- __free_page(tmp_page)
+ __free_pages(tmp_page, 1)


> OK, that's probably enough for now.  Sorry I don't have time to dive
> into the real mechanics of the patch.  I hope this helped.
>

Thanks again for reviewing the code. This was really helpful :)


Regards,
Nitin

Re: [lc-devel] [OLPC-devel] Announce: Compressed cache 'pre-alpha-001' release :)

[Andreas M. <an...@us...>]

Hi,

On Thu, Jul 27, 2006 at 11:10:03AM +0530, Nitin Gupta wrote:
> > It's unfortunate that we came up with a name that conflicts with the
> > popular compiler cache tool.  Can we come up with a unique name?
> >
> 
> No problem. __CCACHE_H, _CCACHE_H_, _COMP_CACHE_H_ ... :)

Better make that _COMPR_CACHE_H_, since it's not a "computer cache". ;)

I reviewed remaining parts, but no comments from me.

Andreas Mohr

Re: [lc-devel] [linux-mm-cc] Re: [OLPC-devel] Announce: Compressed cache 'pre-alpha-001' release :)

[Nitin G. <nit...@gm...>]

Andreas Mohr wrote:
> I reviewed remaining parts, but no comments from me.

That's because code seemed all right to you or was it just unthinkable 
mess? ;)


-- Nitin

Re: [lc-devel] [OLPC-devel] Announce: Compressed cache 'pre-alpha-001' release :)

[Arnd B. <arn...@de...>]

On Thursday 27 July 2006 07:40, Nitin Gupta wrote:

> > > +#define ChunkFree(chunk)             \
> > > +             test_bit(CHUNK_free, (unsigned long *)(&(chunk)->size))

Your code has lots of CamelCase or MIXED_case in it, don't do that,
even if you find other people doing the same.

The two styles that are generally accepted are ALL_CAPS and all_lower_case.

> > This is not safe.  By handing the *_bit() interface an unsigned long
> > pointer to 'unsigned short size' you're risking corrupting the chunk
> > members that are near the size member.  Make 'size' an unsigned long and
> > get rid of these casts.
> >
> 
> I need to keep 'struct chunk' as small as possible. This 'test_bit' usage is
> just super blind copying of page-flags.h :)
> 
> I think this should do (atomic guarantee not required):
> 
> #define ChunkFree(chunk)		\
> 		(!!(chunk->size & (1 << CHUNK_FREE)))
> #define SetChunkFree(chunk)		\
> 		(chunk->size |= (1 << CHUNK_FREE))
> #define ClearChunkFree(chunk)		\
> 		(chunk->size &= ~(1 << CHUNK_FREE))
> 

Or use __set_bit()/__clear_bit() to get the non-atomic versions of these.
test_bit() is always atomic and fast.

> > > +/* error codes */
> > > +#define CC_ENOMEM    1
> > > +/* compression algo actually increased size! */
> > > +#define CC_EEXPAND   2
> > > +/* someone accessed page when we
> > > + * were about to compress it! */
> > > +#define CC_EUSED     3
> >
> > Please don't introduce your own error code namespace.  Typical practice
> > is to use -ERRNO to indicate errors, 0 for success, and +ve for
> > indicating work done.  Using a convention like that will make your code
> > more digestible to other kernel maintainers.
> >
> 
> I couldn't find equivalent error codes with meaning of CC_EEXPAND and CC_EUSED
> so defined them here.	s/CC_ENOMEM/ENOMEM

s/CC_EUSED/EBUSY/ ?

> >
> > > +int anon_cc_started=0;
> > > +unsigned long max_anon_cc_size=0, max_fs_backed_cc_size=0;
> > > +unsigned long anon_cc_size=0, fs_backed_cc_size=0;   /* current size */
> >
> > You don't need = 0 here, they'll be put in the BSS and zeroed if you
> > don't define an initial value.
> >
> 
> Ok...But how do you avoid compiler warnings? Though thats not an issue but
> they look ugly when compiling :)

What warning do you get from this?

> > > +int cc_writepage(struct page *page)
> > > +{
> > > +     int ret = -CC_ENOMEM;
> > > +     //swp_entry_t entry;
> > > +     int comp_size, algo_idx;
> > > +     struct chunk_head *ch=0;
> > > +     struct page *tmp_page=0, *new_page=0, **slot;
> >
> > Set pointers to NULL.  Have you run sparse against this code?  See
> > Documentation/sparse.txt in the kernel tree.
> >
> 
> Having trouble understanding what it is....Will read more on it.

Even better, don't initialize any local variables in the declaration.
Normally, you have an assignment before the first use anyway. Putting
an initialization to NULL (or 0) in the declaration will prevent gcc
from warning about broken usage.

One more thing about comments. The common style is to
use either

/* single line comment */

or

/*
 * multi
 * line
 * comment
 */

but not

/* multi
 * line
 * comment */

For disabling code fragments, use

#if 0
	dont_call();
#endif

instead of

	/* dont_call(); */

For the initial code, it also helps to run it through 'indent -kr -i8',
in order to get indentation right.

	Arnd <><

Re: [lc-devel] [OLPC-devel] Announce: Compressed cache 'pre-alpha-001' release :)

[Zach B. <za...@za...>]

> I need to keep 'struct chunk' as small as possible. This 'test_bit'
> usage is
> just super blind copying of page-flags.h :)
> 
> I think this should do (atomic guarantee not required):

Oh, if you don't need atomicity why not just use a bitfield?

	unsigned short	free:1,
			size:15;

or whatever.  Though I think you'll find that most archs are padding it
out to be at least an int anyway to avoid unaligned accesses of
neighbouring members.

> I couldn't find equivalent error codes with meaning of CC_EEXPAND and
> CC_EUSED
> so defined them here.    s/CC_ENOMEM/ENOMEM

So have it return > 0 for the expand case?

> chunk_head is given to page_cache_get() before we know it's 'struct
> page' or
> 'struct chunk_head' so need to keep name and its position same as for
> struct
> page.

Ugh, I missed that detail.  That sounds very dangerous and I don't think
it will be allowed.  You should find a better way.

> Why the 'struct page' has this '_' for count and mapcount?

Because they're not supposed to be used directly, only by helper functions.

> Yes its temporary and I'm not sure how to handle this failed alloc!
> If it fails, we lose the page since its copy is not stored in swap disk.

The swap path can't return an error at this point?  I'd have to look
into the paths here to offer real help :/.

> Having trouble understanding what it is....Will read more on it.

Think of it as a compiler that only parses the code to emit warnings.

- z

Re: [lc-devel] [OLPC-devel] Announce: Compressed cache 'pre-alpha-001' release :)

[Nitin G. <nit...@gm...>]

Zach Brown wrote:
>> I need to keep 'struct chunk' as small as possible. This 'test_bit'
>> usage is
>> just super blind copying of page-flags.h :)
>>
>> I think this should do (atomic guarantee not required):
>
> Oh, if you don't need atomicity why not just use a bitfield?
>
> 	unsigned short	free:1,
> 			size:15;
>
> or whatever.  Though I think you'll find that most archs are padding it
> out to be at least an int anyway to avoid unaligned accesses of
> neighbouring members.
Never used 'bitfield' before. I'll surely look into this.
>
>> I couldn't find equivalent error codes with meaning of CC_EEXPAND and
>> CC_EUSED
>> so defined them here.    s/CC_ENOMEM/ENOMEM
>
> So have it return > 0 for the expand case?
>
I think, its better to have a consistent interface: -ve is error always, 
+ve, 0 is success.
>> chunk_head is given to page_cache_get() before we know it's 'struct
>> page' or
>> 'struct chunk_head' so need to keep name and its position same as for
>> struct
>> page.
>
> Ugh, I missed that detail.  That sounds very dangerous and I don't think
> it will be allowed.  You should find a better way.
>
Why do you think its 'dangerous'? In very early patches I used a 'struct 
page' in place of 'struct chunk_head'. This would have been 'safe' but 
that isn't good as sizeof(struct page) is just too big -- metadata size 
overhead will explode.


Regards,
Nitin

Re: [lc-devel] [OLPC-devel] Announce: Compressed cache 'pre-alpha-001' release :)

[Nitin G. <nit...@gm...>]

Arnd Bergmann wrote:
> On Thursday 27 July 2006 07:40, Nitin Gupta wrote:
>
>>>> +#define ChunkFree(chunk)             \
>>>> +             test_bit(CHUNK_free, (unsigned long *)(&(chunk)->size))
>
> Your code has lots of CamelCase or MIXED_case in it, don't do that,
> even if you find other people doing the same.
>
> The two styles that are generally accepted are ALL_CAPS and all_lower_case.
>
Done s/CHUNK_free/CHUNK_FREE and like. 'Chunk' operations are used at my 
places where they are done on 'struct page' too which uses CamelCase. 
So, it may look uglier to have ALL_CAPS for chunk ops. like:

if (TestPageLocked(page))
    SET_CHUNK_LOCKED(page);

instead of...

if (TestPageLocked(page))
    SetChunkLocked(page);

>>> This is not safe.  By handing the *_bit() interface an unsigned long
>>> pointer to 'unsigned short size' you're risking corrupting the chunk
>>> members that are near the size member.  Make 'size' an unsigned long and
>>> get rid of these casts.
>>>
>> I need to keep 'struct chunk' as small as possible. This 'test_bit' usage is
>> just super blind copying of page-flags.h :)
>>
>> I think this should do (atomic guarantee not required):
>>
>> #define ChunkFree(chunk)		\
>> 		(!!(chunk->size & (1 << CHUNK_FREE)))
>> #define SetChunkFree(chunk)		\
>> 		(chunk->size |= (1 << CHUNK_FREE))
>> #define ClearChunkFree(chunk)		\
>> 		(chunk->size &= ~(1 << CHUNK_FREE))
>>
>
> Or use __set_bit()/__clear_bit() to get the non-atomic versions of these.
> test_bit() is always atomic and fast.
>
Again __set_bit()/__clear_bit() take unsigned long while chunk->size is 
unsigned short. Can it cause problem if I cast this chunk->size to 
unsigned long and pass to them? I think they just require start address 
to count bit from and hence shouldn't hurt.
>>>> +/* error codes */
>>>> +#define CC_ENOMEM    1
>>>> +/* compression algo actually increased size! */
>>>> +#define CC_EEXPAND   2
>>>> +/* someone accessed page when we
>>>> + * were about to compress it! */
>>>> +#define CC_EUSED     3
>>> Please don't introduce your own error code namespace.  Typical practice
>>> is to use -ERRNO to indicate errors, 0 for success, and +ve for
>>> indicating work done.  Using a convention like that will make your code
>>> more digestible to other kernel maintainers.
>>>
>> I couldn't find equivalent error codes with meaning of CC_EEXPAND and CC_EUSED
>> so defined them here.	s/CC_ENOMEM/ENOMEM
>
> s/CC_EUSED/EBUSY/ ?
>
Looks fine. I'll get some replacement for CC_EEXPAND too and get rid of 
these :)

>>>> +int anon_cc_started=0;
>>>> +unsigned long max_anon_cc_size=0, max_fs_backed_cc_size=0;
>>>> +unsigned long anon_cc_size=0, fs_backed_cc_size=0;   /* current size */
>>> You don't need = 0 here, they'll be put in the BSS and zeroed if you
>>> don't define an initial value.
>>>
>> Ok...But how do you avoid compiler warnings? Though thats not an issue but
>> they look ugly when compiling :)
>
> What warning do you get from this?
>
None. These are global variables, didn't see that carefully. I get 
'variable can be used uninitialized' for local vars. (when they indeed 
can be used uninitialized).
>>>> +int cc_writepage(struct page *page)
>>>> +{
>>>> +     int ret = -CC_ENOMEM;
>>>> +     //swp_entry_t entry;
>>>> +     int comp_size, algo_idx;
>>>> +     struct chunk_head *ch=0;
>>>> +     struct page *tmp_page=0, *new_page=0, **slot;
>>> Set pointers to NULL.  Have you run sparse against this code?  See
>>> Documentation/sparse.txt in the kernel tree.
>>>
>> Having trouble understanding what it is....Will read more on it.
>
> Even better, don't initialize any local variables in the declaration.
> Normally, you have an assignment before the first use anyway. 
Almost every time, case is this:

some_func()
{
    void *a, *b;
    a = alloc()
    if (!a) goto out:
    b = alloc();
    ...
out:
    if (a) free(a)
    if (b) free(b)
    ...
}

Here you (correctly) get 'possible uninitialized usage for b' warning. 
That's why I initialize those local variables.

> One more thing about comments. The common style is to
> use either
>
> /* single line comment */
>
> or
>
> /*
>  * multi
>  * line
>  * comment
>  */
>
> but not
>
> /* multi
>  * line
>  * comment */
>
> For disabling code fragments, use
>
> #if 0
> 	dont_call();
> #endif
>
> instead of
>
> 	/* dont_call(); */
>
/*
 * Ok, then
 * I'll do this :)
 */

> For the initial code, it also helps to run it through 'indent -kr -i8',
> in order to get indentation right.
>
> 	Arnd <><




Regards,
Nitin

Re: [lc-devel] [OLPC-devel] Announce: Compressed cache 'pre-alpha-001' release :)

[Arnd B. <ar...@ar...>]

On Thursday 27 July 2006 09:29, Nitin Gupta wrote:
> some_func()
> {
> =A0 =A0 void *a, *b;
> =A0 =A0 a =3D alloc()
> =A0 =A0 if (!a) goto out:
> =A0 =A0 b =3D alloc();
> =A0 =A0 ...
> out:
> =A0 =A0 if (a) free(a)
> =A0 =A0 if (b) free(b)
> =A0 =A0 ...
> }
>=20
> Here you (correctly) get 'possible uninitialized usage for b' warning.=20
> That's why I initialize those local variables.

Well, I'd prefer to write this as=20

some_func()
{
    void *a, *b;
    int ret;
    ret =3D -ENOMEM;
    a =3D alloc();
    if (!a)
	goto out_a;
    b =3D alloc();
    if (!b)
	goto out_b;
    ...
    ret =3D 0;
    free(b);   =20
out_b:
    free(a);
out_a:
    return ret;
}

Note:
 - statement after if() on following line
 - no variables initialized on declaration (ret could be)
 - only clean up what you created.
 - kfree(NULL); is ok

	Arnd <><

Re: [lc-devel] [OLPC-devel] Announce: Compressed cache 'pre-alpha-001' release :)

[Zach B. <za...@za...>]

> Again __set_bit()/__clear_bit() take unsigned long while chunk->size is
> unsigned short. Can it cause problem if I cast this chunk->size to
> unsigned long and pass to them?

_Yes_.  That's what I said in the first mail and why I brought it up to
begin with!  Maybe architectures (like i386!) implement it with assembly
that will write to the entire long when setting a given bit.

- z

Re: [lc-devel] [OLPC-devel] Announce: Compressed cache 'pre-alpha-001' release :)

[Nitin G. <nit...@gm...>]

Zach Brown wrote:
>> Again __set_bit()/__clear_bit() take unsigned long while chunk->size is
>> unsigned short. Can it cause problem if I cast this chunk->size to
>> unsigned long and pass to them?
>
> _Yes_.  That's what I said in the first mail and why I brought it up to
> begin with!  Maybe architectures (like i386!) implement it with assembly
> that will write to the entire long when setting a given bit.
>
> - z
>
Ok, then I will do with this (as posted earlier):

#define ChunkFree(chunk)        \
        (!!(chunk->size & (1 << CHUNK_FREE)))
#define SetChunkFree(chunk)        \
        (chunk->size |= (1 << CHUNK_FREE))
#define ClearChunkFree(chunk)        \
        (chunk->size &= ~(1 << CHUNK_FREE))


-- Nitin

Re: [lc-devel] [OLPC-devel] Announce: Compressed cache 'pre-alpha-001' release :)

[Zach B. <za...@za...>]

Nitin Gupta wrote:
> Hi All,
> 
> This is the patch for compressed caching feature against 2.6.18-rc2 :)

Has anyone reviewed this with you yet?  I'll share the comments that
come to mind while lightly reading the patch.  I won't go into too much
depth.

> -EXTRAVERSION = -rc2
> +EXTRAVERSION = -rc2-cc

You can avoid polluting the diff with this naming change by putting
'-cc' in ./localversion

> +++ b/include/linux/ccache.h
> @@ -0,0 +1,97 @@
> +#ifndef _CCACHE_H
> +#define _CCACHE_H

It's unfortunate that we came up with a name that conflicts with the
popular compiler cache tool.  Can we come up with a unique name?

> +/* chunk flags: only 3 MSBs available [13-15] */
> +#define CHUNK_free	15
> +#define CHUNK_merged	14

The general style is to use all caps for constants.  The page-flags.h
constructs are a bit of an anomaly and shouldn't be duplicated :)

> +#define ChunkFree(chunk)		\
> +		test_bit(CHUNK_free, (unsigned long *)(&(chunk)->size))

This is not safe.  By handing the *_bit() interface an unsigned long
pointer to 'unsigned short size' you're risking corrupting the chunk
members that are near the size member.  Make 'size' an unsigned long and
get rid of these casts.

Really, I'd just get rid of these wrappers entirely and use the native
*_bit() interface.

> +/* error codes */
> +#define CC_ENOMEM	1
> +/* compression algo actually increased size! */
> +#define CC_EEXPAND	2
> +/* someone accessed page when we
> + * were about to compress it! */
> +#define CC_EUSED	3

Please don't introduce your own error code namespace.  Typical practice
is to use -ERRNO to indicate errors, 0 for success, and +ve for
indicating work done.  Using a convention like that will make your code
more digestible to other kernel maintainers.

> +	atomic_t _count;		/* usage count; free this struct
> +					 * when count is 0 */

Lose the '_'.  Heck, maybe just remove it until it is actually used :).

> -	 },
> -	 {
> +	},
> +	{

Don't mix changes like this in with the rest of the patch.  They just
add background noise.

> +int anon_cc_started=0;
> +unsigned long max_anon_cc_size=0, max_fs_backed_cc_size=0;
> +unsigned long anon_cc_size=0, fs_backed_cc_size=0;	/* current size */

You don't need = 0 here, they'll be put in the BSS and zeroed if you
don't define an initial value.

> +const unsigned long ccache_size_limit = MAX_SWAP_OFFSET - 1;
> +
> +static struct list_head pages_head, mcl_head, lru_head;

If you defined these with LIST_HEAD() then they would already be
initialized and you wouldn't need INIT_LIST_HEAD() in init_ccache().

> +	//pr_info("==== flags1=0x%08lx\n", ch->flags);

If you used pr_debug() instead you could at least enable and disable the
output on a per-file bases by defining DEBUG.  That might save you from
having to comment them in and out all the time.

> +static int WKdm_compress_wrapper (unsigned char *src, unsigned long src_len,
> +				unsigned char *dest, unsigned long *dest_len)
> +{
> +	//pr_info("%s called.\n", __FUNCTION__);
> +	*dest_len=WKdm_compress((WK_word*)src, (WK_word*)dest, 1024);
> +	return 0;

I guess propagating these return codes will be fixed up eventually.
White space around '=', please:

	*dest_len = WK..

> +	if (ret==LZO_E_OK) pr_info("LZO compression done!\n");
> +	else pr_info("LZO compression failed!\n");

Use newlines after if/else throughout the code, please:

	if (foo)
		doit;
	else
		something;

> +repeat:
> +	spin_lock(&ccache_lock);
> +	while (free_head) {
> +		//pr_info("in get_enough_chunks loop: %d\n", tst++);
> +		chunk = free_head;
> +		free_head = chunk->next;
> +		ClearChunkFree(chunk);
> +		spin_unlock(&ccache_lock);
> +		if (ChunkMerged(chunk)) {
> +			pr_info("merged chunk found: size=%d\n",
> +					ChunkSize(chunk));
> +			kfree(chunk);
> +			continue;

The ccache_lock needs to be reacquired before continuing so that it
doesn't double-unlock.  You should perform your tests with lockdep
enabled in the kernel config :).

> +	if (ret) goto out;
> +	goto repeat;
> +out:

This sort of thing is a warning sign that the loop is getting too weird.
 Hopefully some more natural constructs can be found to simplify things.

> +int init_ccache(void)
> +{
> +	spin_lock(&ccache_lock);
> +	INIT_LIST_HEAD(&pages_head);
> +	INIT_LIST_HEAD(&lru_head);
> +	INIT_LIST_HEAD(&mcl_head);
> +	free_head = NULL;
> +	spin_unlock(&ccache_lock);

Hmmm.  What are these locks protecting?  If they're racing with users of
the lists then they're racing with users who will see uninitialized
lists :/.

> +struct page* cc_readpage(struct chunk_head *ch)
> +{
> +	int ret = -CC_ENOMEM, algo_idx;
> +	unsigned int comp_size=0;
> +	struct page *decomp_page, *comp_page, *tmp_page;
> +	void *comp_data;
> +	struct chunk *chunk, *tmp;
> +	unsigned long flags;

'unsigned long flags' on the stack in the kernel usually means that
'flags' is going to be an argument to locking constructs that save irq
state flags.  This particular 'flags' only seems to be a temporary to
store a trivial mask so you might as well remove it and avoid surprising
people.

> +	/* collect compressed page from chunks */
> +	comp_page = alloc_page(GFP_ATOMIC);
> +	if (!comp_page) {
> +		pr_info("cc_readpage: comp_page alloc failed!!!\n");
> +		BUG();
> +	}

GFP_ATOMIC allocations are expected to fail under load.  I assume this
is just a temporary BUG() :).

> +	spin_lock(&ccache_lock);
> +	list_del(&ch->lru);
> +	spin_unlock(&ccache_lock);

You almost always want to use list_del_init() so that you don't leave
stale pointers in the list_head that is being removed.

> +int cc_writepage(struct page *page)
> +{
> +	int ret = -CC_ENOMEM;
> +	//swp_entry_t entry;
> +	int comp_size, algo_idx;
> +	struct chunk_head *ch=0;
> +	struct page *tmp_page=0, *new_page=0, **slot;

Set pointers to NULL.  Have you run sparse against this code?  See
Documentation/sparse.txt in the kernel tree.

> +	struct address_space *mapping=page_mapping(page);
> +
> +	//ch = kmalloc(sizeof(struct chunk_head), GFP_KERNEL);
> +	//if (!ch) goto out;
> +	tmp_page = alloc_pages(GFP_KERNEL, 1);	/* page may expand */

Do you mean to allocate two pages here?  You don't seem to use the
second one and only free the first.

OK, that's probably enough for now.  Sorry I don't have time to dive
into the real mechanics of the patch.  I hope this helped.

- z