Hi Stephen, Thanks again for the detailed review.
On 03/23/2016 10:22 AM, Stephen Warren wrote: > On 03/20/2016 07:45 PM, Eric Nelson wrote: >> Add a block device cache to speed up repeated reads of block devices by >> various filesystems. >> >> This small amount of cache can dramatically speed up filesystem >> operations by skipping repeated reads of common areas of a block >> device (typically directory structures). >> >> This has shown to have some benefit on FAT filesystem operations of >> loading a kernel and RAM disk, but more dramatic benefits on ext4 >> filesystems when the kernel and/or RAM disk are spread across >> multiple extent header structures as described in commit fc0fc50. >> >> The cache is implemented through a minimal list (block_cache) maintained >> in most-recently-used order and count of the current number of entries >> (cache_count). It uses a maximum block count setting to prevent copies >> of large block reads and an upper bound on the number of cached areas. >> >> The maximum number of entries in the cache defaults to 4 and the maximum >> number of blocks per cache entry has a default of 1, which matches the >> current implementation of at least FAT and ext4 that only read a single >> block at a time. > > If the maximum size of the cache is fixed and small (which judging by > the description it is), why not use an array rather than a linked list. > That would be far simpler to manage. > You seem to agree with Marek, and I must be dain-bramaged because I just don't see it. >> The 'blkcache' command (enabled through CONFIG_CMD_BLOCK_CACHE) allows >> changing these values and can be used to tune for a particular filesystem >> layout. > > Even with this dynamic adjustment, using an array still feels simpler, > although I have't looked at the code yet. > >> diff --git a/drivers/block/cache_block.c b/drivers/block/cache_block.c > >> +/* >> + * search for a set of blocks in the cache >> + * >> + * remove and return the node if found so it can be >> + * added back at the start of the list and maintain MRU order) >> + */ > > Splitting the remove/add feels a bit dangerous, since forgetting to > re-do the add (or e.g. some error causing that to be skipped) could > cause cache_count to become inconsistent. > > The function name "find" is a bit misleading. cache_find_and_remove() > would make its semantics more obvious. Or, simply put the list_del() > somewhere else; it doesn't feel like part of a "find" operation to me. > Or, put the entire move-to-front operation inside this function so it > isn't split up - if so, cache_find_and_move_to_head(). > You're right. I'll look at that for a V3 (hopefully non-RFC) patch set. >> +static struct block_cache_node *cache_find >> + (int iftype, int devnum, > > Nit: the ( and first n arguments shouldn't be wrapped. > >> +int cache_block_read(int iftype, int devnum, >> + lbaint_t start, lbaint_t blkcnt, >> + unsigned long blksz, void *buffer) > >> + memcpy(buffer, src, blksz*blkcnt); > > Nit: Space around operators. checkpatch should catch this. > Thanks. >> + if (trace) >> + printf("hit: start " LBAF ", count " LBAFU "\n", >> + start, blkcnt); > > I guess I'll find that trace can be adjusted at run-time somewhere later > in this patch, but it's more typical to use debug() without the if > statement for this. It would be awesome if debug() could be adjusted at > run-time... > I wanted to keep this as a run-time thing because it's useful in tuning things and I'm not yet certain if/when users might need to update the sizes. I could wrap these with some #ifdef stuff but I'm not certain which is the right thing to do. >> +void cache_block_fill(int iftype, int devnum, > ... >> + if (node->cache == 0) { >> + node->cache = malloc(bytes); >> + if (!node->cache) { > > Nit: may as well be consistent with those NULL checks. > Yep. >> +void cache_block_invalidate(int iftype, int devnum) > ... >> +void cache_block_invalidate_all(void) > > There's no invalidate_blocks(); I imagine that writing-to/erasing (some > blocks of) a device must call cache_block_invalidate() which will wipe > out even data that wasn't written. > Right. I figured that this wasn't worth extra code. The 99.99% use case for U-Boot is read-only, so optimizing this just seems like bloat. >> +static void dump_block_cache(void) > ... >> + list_for_each_safe(entry, n, &block_cache) { > > Nit: This doesn't need to be _safe since the list is not being modified. > Good catch. >> +static int do_blkcache(cmd_tbl_t *cmdtp, int flag, >> + int argc, char * const argv[]) >> +{ >> + if ((argc == 1) || !strcmp("show", argv[1])) { >> + printf("block cache:\n" >> + "%u\thits\n" >> + "%u\tmisses\n" >> + "%u\tentries in cache\n" >> + "trace %s\n" >> + "max blocks/entry %lu\n" >> + "max entries %lu\n", >> + block_cache_hits, block_cache_misses, cache_count, >> + trace ? "on" : "off", >> + max_blocks_per_entry, max_cache_entries); > > Nit: Let's print the value and "label" in a consistent order. I would > suggest everything being formatted as: > > " description: %u" > > so that it's indented, has a delimiter between description and value, > and so that irrespective of the length of the converted value, the > indentation of the other parts of the string don't change (\t doesn't > guarantee this after a certain number length). > Thanks. > I would rather have expected "show" to dump the entries too, but I > suppose it's fine that they're separate. > While testing, I found myself mostly using 'show' to see results. I added dump as a debug tool and almost removed it, but figured this was still an RFC, so I left it in. >> + } else if ((argc == 2) && ('c' == argv[1][0])) { > > Nit: the value of 'c' is always 'c', so it's pointless to test whether > it's equal to something. The value being compared is arv[1][0], so the > parameters to == should be swapped. I'm aware that == is mathematically > commutative, but typical English reading of the operator is "is equal > to" which has non-commutative implications re: what is being tested. I'm > also aware that this construct is used to trigger compiler warnings if > someone types = instead of ==. However, (a) you didn't and (b) compilers > warn about this these days, so there's no need to write unusual code to > get that feature anymore. > :) muscle-memory again. It's usually Marek that catches my yoda-expressions, which are usually tests against zero. > I worry that being imprecise in command-line parsing (i.e. treating both > "crud" and "clear" as the same thing) will lead to problems expanding > the command-line format in the future; we won't be able to ever add > another option starting with "c" and maintain the same abbreviation > semantics. I'd suggest requiring the full option name always. > >> + } else if ((argc == 3) && isdigit(argv[1][0]) && >> isdigit(argv[2][0])) { > > Let's make this "blkcache set" or "blkcache configure" so that other > sub-commands can take numeric parameters in the future, and have > consistent command-line syntax. > Sounds good to me. >> +U_BOOT_CMD( >> + blkcache, 3, 0, do_blkcache, >> + "block cache control/statistics", >> + "[show] - show statistics\n" >> + "blkcache c[lear] - clear statistics\n" >> + "blkcache d[ump] - dump cache entries\n" >> + "blkcache i[nvalidate] - invalidate cache\n" >> + "blkcache #maxblocks #maxentries\n" >> + "\tset maximum blocks per cache entry, maximum entries\n" >> + "blkcache t[race] [off] - enable (disable) tracing" >> +); > > BTW, isn't there some support in U-Boot for sub-commands already, so you > can write a separate function per sub-command and skip writing all the > strcmp logic to select between them? I'm pretty sure I saw that somewhere. > There is, but I haven't (yet) used it. I'll take a look at this in V3. >> diff --git a/include/part.h b/include/part.h > >> +static inline void cache_block_invalidate_all(void){} > > Is that useful outside of the debug commands? > Not at the moment, and it probably won't be needed when I figure out how this glues to the block layer and/or drivers in a cleaner way. Regards, Eric _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot