Re: new cache component

[Ben Elliston <bje at redhat dot com>]

>>>>> "Frank" == Frank Ch Eigler <fche@redhat.com> writes:

  Frank> Good stuff.  I like the cache_line/cache_set abstractions.  A
  Frank> few nits with respect to the code:

Thanks for taking the time to review it.

  Frank> - The nomenclature of cache component type names is perhaps
  Frank>   too complicated.  Some of the parameters specified in the
  Frank>   type name could (should?) be treated as attributes instead.
  Frank>   (However, then one would have to address the issue of their
  Frank>   run-time changeability.) 

"I hear you." :-)

After much deliberation, I opted for the component type names to carry
the configuration parameters.  The runtime changeability was just too
onerous.  I tried to keep the naming scheme uniform and intuitive.

  Frank>   In any case, the type name parsing routine in CacheCreate()
  Frank>   could benefit from sidutil::tokenize(),
  Frank>   or even better, from using a fixed list of type-names and 
  Frank>   parameters, like the flash memory components do.

I have made a change to use tokenize(), but I think I like the idea of
a lookup table better instead.  I'll make that change.

  Frank> - In a couple of places, sidutil::parse_attribute() is called
  Frank>   but its return value is not checked.  Naughty!

Ooo, errr ..

  Frank> - Consider adding TODOs for future support for bus snooping, leading
  Frank>   to cache coherency protocols

.. and ..

  Frank> - Consider adding per-cache-line statistics (to detect if accesses
  Frank>   are not distributed nicely amongst the lines).

Both TODOs, I'm afraid. :-(

Ben