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

[Zach Brown]

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