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