* Re: new cache component
[not found] <15145.36560.815865.750859.cygnus.project.sid@scooby.apac.redhat.com>
@ 2001-06-15 8:46 ` Frank Ch. Eigler
2001-06-20 23:30 ` Ben Elliston
0 siblings, 1 reply; 3+ messages in thread
From: Frank Ch. Eigler @ 2001-06-15 8:46 UTC (permalink / raw)
To: Ben Elliston; +Cc: sid
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: new cache component
2001-06-15 8:46 ` new cache component Frank Ch. Eigler
@ 2001-06-20 23:30 ` Ben Elliston
0 siblings, 0 replies; 3+ messages in thread
From: Ben Elliston @ 2001-06-20 23:30 UTC (permalink / raw)
To: Frank Ch. Eigler; +Cc: sid
>>>>> "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
^ permalink raw reply [flat|nested] 3+ messages in thread
* new cache component
@ 2001-06-14 21:28 Ben Elliston
0 siblings, 0 replies; 3+ messages in thread
From: Ben Elliston @ 2001-06-14 21:28 UTC (permalink / raw)
To: sid
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2001-06-20 23:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <15145.36560.815865.750859.cygnus.project.sid@scooby.apac.redhat.com>
2001-06-15 8:46 ` new cache component Frank Ch. Eigler
2001-06-20 23:30 ` Ben Elliston
2001-06-14 21:28 Ben Elliston
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).