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

[Arnd Bergmann]

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 <><