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

[Nitin Gupta]

On 7/26/06, Zach Brown <zab at zabbo.net> 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