Re: [PATCH 2/6] compcache: block device - internal defs

[Nitin Gupta]

On Mon, Mar 24, 2008 at 9:35 PM, Will Newton <will.newton@xxxxxxxxx> wrote:
> On Mon, Mar 24, 2008 at 3:03 PM, Nitin Gupta <nitingupta910@xxxxxxxxx> wrote:
>
>  Hi Nitin,
>
>
>
>  > This contains header to be used internally by block device code.
>  >  It contains flags to enable/disable debugging, stats collection and also
>  >  defines default disk size (25% of total RAM).
>  >
>  >  Signed-off-by: Nitin Gupta <nitingupta910 at gmail dot com>
>  >  ---
>  >   drivers/block/compcache.h |  147 +++++++++++++++++++++++++++++++++++++++++++++
>  >   1 files changed, 147 insertions(+), 0 deletions(-)
>  >
>  >  diff --git a/drivers/block/compcache.h b/drivers/block/compcache.h
>  >  new file mode 100644
>  >  index 0000000..b84b5d3
>  >  --- /dev/null
>  >  +++ b/drivers/block/compcache.h
>  >  @@ -0,0 +1,147 @@
>  >  +/*
>  >  + * Compressed RAM based swap device
>  >  + *
>  >  + * (C) Nitin Gupta
>  >  + *
>  >  + * This RAM based block device acts as swap disk.
>  >  + * Pages swapped to this device are compressed and
>  >  + * stored in memory.
>  >  + *
>  >  + * Project home: http://code.google.com/p/compcache
>  >  + */
>  >  +
>  >  +#ifndef _COMPCACHE_H_
>  >  +#define _COMPCACHE_H_
>  >  +
>  >  +#define K(x)   ((x) >> 10)
>  >  +#define KB(x)  ((x) << 10)
>  >  +
>  >  +#define SECTOR_SHIFT           9
>  >  +#define SECTOR_SIZE            (1 << SECTOR_SHIFT)
>  >  +#define SECTORS_PER_PAGE_SHIFT (PAGE_SHIFT - SECTOR_SHIFT)
>  >  +#define SECTORS_PER_PAGE       (1 << SECTORS_PER_PAGE_SHIFT)
>  >  +
>  >  +/*-- Configurable parameters */
>  >  +/* Default compcache size: 25% of total RAM */
>  >  +#define DEFAULT_COMPCACHE_PERCENT      25
>  >  +#define INIT_SIZE                      KB(16)
>  >  +#define GROW_SIZE                      INIT_SIZE
>
>  Maybe these could be renamed to INIT_SIZE_BYTES/GROW_SIZE_BYTES to
>  make the units clearer?
>


Sounds better. I will rename these.


>
>  >  +/*-- */
>  >  +
>  >  +/* Message prefix */
>  >  +#define C "compcache: "
>  >  +
>  >  +/* Debugging and Stats */
>  >  +#define NOP    do { } while(0)
>  >  +
>  >  +#if (1 || defined(CONFIG_DEBUG_COMPCACHE))
>  >  +#define DEBUG  1
>  >  +#define STATS  1
>  >  +#else
>  >  +#define DEBUG  0
>  >  +#define STATS  0
>  >  +#endif
>
>  If DEBUG is defined unconditionally what is the point of CONFIG_DEBUG_COMPCACHE?
>
>

For now I have not added DEBUG_COMPCACHE as make option (menuconfig)
so its unconditional for now. I am now adding this option and then it
will not be unconditional.

>  >  +
>  >  +/* Create /proc/compcache? */
>  >  +/* If STATS is disabled, this will give minimal compcache info */
>  >  +#define CONFIG_COMPCACHE_PROC
>  >  +
>  >  +#if DEBUG
>  >  +#define CC_DEBUG(fmt,arg...) \
>  >  +       printk(KERN_DEBUG C fmt,##arg)
>  >  +#else
>  >  +#define CC_DEBUG(fmt,arg...) NOP
>  >  +#endif
>
>  Have you thought about using pr_debug() for this? It looks like it
>  would simplify this file at the cost of a little flexibility.
>

I want to enable/disable this debugging based on DEBUG_COMPCACHE flag.
Thats why I added these macros. I will do 'printk(KERN_DEBUG' ->
pr_debug


>
>  >  +
>  >  +/*
>  >  + * Verbose debugging:
>  >  + * Enable basic debugging + verbose messages spread all over code
>  >  + */
>  >  +#define DEBUG2 0
>  >  +
>  >  +#if DEBUG2
>  >  +#define DEBUG  1
>  >  +#define STATS  1
>  >  +#define CONFIG_COMPCACHE_PROC  1
>  >  +#define CC_DEBUG2((fmt,arg...) \
>  >  +       printk(KERN_DEBUG C fmt,##arg)
>  >  +#else /* DEBUG2 */
>  >  +#define CC_DEBUG2(fmt,arg...) NOP
>  >  +#endif
>  >  +
>  >  +/* Its useless to collect stats if there is no way to export it */
>  >  +#if (STATS && !defined(CONFIG_COMPCACHE_PROC))
>  >  +#error "compcache stats is enabled but not /proc/compcache."
>  >  +#endif
>
>  So it appears that if we want DEBUG we also get STATS, which requires
>  /proc support enabled, so it is impossible to have just DEBUG and no
>  STATS or /proc support?
>

I will decouple debug and stats so each can be enabled/disabled individually.

Thanks,
Nitin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/