Re: new cache component

Re: new cache component

[Frank Ch. Eigler]

bje wrote:

: I have just committed a new memory cache component.  [...]

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

- The nomenclature of cache component type names is perhaps
  too complicated.  Some of the parameters specified in the
  type name could (should?) be treated as attributes instead.
  (However, then one would have to address the issue of their
  run-time changeability.)  In any case, the type name parsing
  routine in CacheCreate() could benefit from sidutil::tokenize(),
  or even better, from using a fixed list of type-names and 
  parameters, like the flash memory components do.

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

- Your use of various static objects is fine w.r.t. C++ standards, 
  but is possibly fragile on platforms with crappy C++ shared library
  implementations.  You should check the code on non-Linux hosts; may
  need to go for heap allocation instead. :-(

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

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


- FChE

Re: new cache component

[Ben Elliston]

>>>>> "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

new cache component

[Ben Elliston]

I have just committed a new memory cache component.  It should be
sufficiently configurable for most applications.  For more
information, please refer to the siddoc documentation file
(hw-cache.txt).

Ben