[U-Boot] [PATCH V3 1/3] drivers: block: add block device cache

[Tom Rini]

On Fri, Apr 01, 2016 at 04:16:42PM -0700, Eric Nelson wrote:
> Hi Stephen,
> 
> On 04/01/2016 03:57 PM, Stephen Warren wrote:
> > On 03/31/2016 02:24 PM, Eric Nelson wrote:
> >> On 03/30/2016 02:57 PM, Stephen Warren wrote:
> >>> On 03/30/2016 11:34 AM, Eric Nelson wrote:
> >>>> On 03/30/2016 07:36 AM, Stephen Warren wrote:
> >>>>> On 03/28/2016 11:05 AM, Eric Nelson wrote:
> 
> <snip>
> 
> >>>
> >>> We could allocate the data storage for the block cache at the top of RAM
> >>> before relocation, like many other things are allocated, and hence not
> >>> use malloc() for that.
> >>
> >> Hmmm. We seem to have gone from a discussion about data structures to
> >> type of allocation.
> >>
> >> I'm interested in seeing how that works. Can you provide hints about
> >> what's doing this now?
> > 
> > Something like common/board_f.c:reserve_mmu() and many other functions
> > there. relocaddr starts at approximately the top of RAM, continually
> > gets adjusted down as many static allocations are reserved, and
> > eventually becomes the address that U-Boot is relocated to. Simply
> > adding another entry into init_sequence_f[] for the disk cache might work.
> > 
> 
> Thanks for the pointer. I'll review that when time permits.
> 
> This would remove the opportunity to re-configure the cache though, right?
> 
> I'm not sure whether how important this feature is, and I think
> only time and use will tell.
> 
> I'd prefer to keep that ability at least for a cycle or two so that
> I and others can test.
> 
> >>>> While re-working the code, I also thought more about using an array and
> >>>> still don't see how the implementation doesn't get more complex.
> >>>>
> >>>> The key bit is that the list is implemented in MRU order so
> >>>> invalidating the oldest is trivial.
> >>>
> >>> Yes, the MRU logic would make it more complex. Is that particularly
> >>> useful, i.e. is it an intrinsic part of the speedup?
> >>
> >> It's not a question of speed with small numbers of entries. The code
> >> to handle eviction would just be more complex.
> > 
> > My thought was that if the eviction algorithm wasn't important (i.e.
> > most of the speedup comes from have some (any) kind of cache, but the
> > eviction algorithm makes little difference to the gain from having the
> > cache), we could just drop MRU completely. If that's not possible, then
> > indeed a list would make implementing MRU easier.
> > 
> 
> How would we decide which block to discard? I haven't traced enough
> to know what algorithm(s) might be best, but I can say that there's
> a preponderance of repeated accesses to the last-accessed block,
> especially in ext4.
> 
> > You could still do a list with a statically allocated set of list nodes,
> > especially since the length of the list is bounded.
> > 
> 
> Sure. A pooled allocator (pool of free nodes) works well with
> array-based allocation.
> 
> Having a fixed upper limit on the number of blocks would require
> additional checking unless we just sized it for (max entries * max
> blocks/entry).
> 
> >> Given that the command "blkcache configure 0 0" will discard all
> >> cache and since both dfu and ums should properly have the cache
> >> disabled, I'd like to proceed as-is with the list and heap approach.
> >
> > I don't understand "since both dfu and ums should properly have the
> > cache disabled"; I didn't see anything that did that. Perhaps you're
> > referring to the fact that writes invalidate the cache?
> > 
> 
> Yes, but also that the host will cache blocks in the ums case, so
> having the cache enabled will only slow things down slightly by
> lots of memcpy's to cached blocks that won't be helpful.
> 
> I think I was a bit flippant by including dfu in this statement,
> since I haven't used it to access anything except SPI-NOR.
> 
> > Eventually it seems better to keep the cache enabled for at least DFU to
> > a filesystem (rather than raw block device) since presumably parsing the
> > directory structure to write to a file for DFU would benefit from the
> > cache just like anything else.
> 
> I'm not in a position to comment about dfu.

For the record, I think this discussion is good and important, but not a
blocker for getting the initial functionality in.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160401/ee5052f1/attachment.sig>