[U-Boot] [RFC PATCH 1/2] add block device cache

Stephen Warren
2016-03-17T21:16:58Z

On 03/16/2016 03:40 PM, Eric Nelson wrote:
> Signed-off-by: Eric Nelson <eric@nelint.com>

A patch description would be useful here; the cover letter wouldn't be
checked in.

> diff --git a/drivers/block/cache_block.c b/drivers/block/cache_block.c

> +static int cache_iftype = -1;
> +static int cache_devnum = -1;
> +static lbaint_t cache_start = -1;
> +static lbaint_t cache_blkcnt = -1;
> +static unsigned long cache_blksz;
> +static void *cache;

Since variable "cache" is in BSS and hence is 0, which is included in
all checks below, none of the other values need to be initialized.

> +int cache_block_read(int iftype, int dev,
> + lbaint_t start, lbaint_t blkcnt,
> + unsigned long blksz, void *buffer)
> +{
> + if ((iftype == cache_iftype) &&
> + (dev == cache_devnum) &&
> + (start == cache_start) &&
> + (blkcnt <= cache_blkcnt) &&
> + (blksz == cache_blksz) &&
> + (cache != 0)) {

I'd suggest putting (cache != 0) first, since that check indicates
whether any of the others are relevant.

> + memcpy(buffer, cache, blksz*blkcnt);

Nit: space around the * operator. Same comment everywhere.

It's probably hard to fix this given the simple API and lack of MMU
page-tables to remap on a per-page basis, but one deficiency in this
(deliberately) simple implementation is that if someone caches blocks
0..7 then later tries to read 2..10 (or even 2..3) they won't get a
cache hit. While partial hits (the 2..10 case) are hard to deal with,
perhaps it's useful to make subset cases (2..3 with 0..7 cached) work?

> +void cache_block_fill(int iftype, int dev,
> + lbaint_t start, lbaint_t blkcnt,
> + unsigned long blksz, void const *buffer)

> + bytes = blksz*blkcnt;
> + if (cache != 0) {
> + if (bytes != (cache_blksz*cache_blkcnt)) {
> + free(cache);
> + cache = malloc(blksz*blkcnt);
> + if (!cache)
> + return;
> + } /* change in size */
> + } else {
> + cache = malloc(blksz*blkcnt);
> + if (!cache)
> + return;
> + }

Is it better to put the if (!cache) test after the whole if/else block
so it's unified?

> +void cache_block_invalidate(int iftype, int dev)
> +{
> + cache_iftype = -1;

That isn't actually necessary, since assigning 0 to cache below will
prevent anything from using the cache.

> + if (cache) {
> + free(cache);
> + cache = 0;
> + }
> +}

> diff --git a/include/part.h b/include/part.h

> +#ifdef CONFIG_BLOCK_CACHE
> +/**
> + * cache_block_read() - attempt to read a set of blocks from cache
> + *
> + * @param iftype - IF_TYPE_x for type of device
> + * @param dev - device index of particular type
> + * @param start - starting block number
> + * @param blkcnt - number of blocks to read
> + * @param blksz - size in bytes of each block
> + * @param buf - buffer to contain cached data

s/contain/receive/?

> + *
> + * @return - '1' if block returned from cache, '0' otherwise.
> + */
> +int cache_block_read
> + (int iftype, int dev,
> + lbaint_t start, lbaint_t blkcnt,
> + unsigned long blksz, void *buffer);

Nit: ( and probably the first n parameters should be on the same line as
the function name.