mem: Add a requestor ID to each request object.

Information
Submitter:	Ali Saidi
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

mem: Add a requestor ID to each request object.

This change adds a requestor id to each request object which can be
used identify every device in the system that is capable of issuing a request. 
This is part of the way to removing the numCpus+1 stats in the cache and
replacing them with the requestor ids. This is one of a series of changes 
that make way for the stats output to be changed to python.

Testing Done

no stats changes on regression tests

Issue Summary

Description	From	Last Updated	Status
Seems like a typedef for RequestID might be in order. I doubt that it really needs to be a uint32_t. ...	Nathan Binkert	Feb. 6, 2012, 8:10 a.m.	Resolved
shouldn't this be a static_cast? We should always know the type. (It could be a safe_cast if you like)	Nathan Binkert	Feb. 6, 2012, 8:10 a.m.	Resolved
What's this?	Nathan Binkert	Feb. 6, 2012, 8:10 a.m.	Resolved
Maybe a little comment on what each of these rids are for? writeback, functional, and ????	Nathan Binkert	Feb. 6, 2012, 8:15 a.m.	Resolved
These changes are somewhat dangerous because of type conversions. A simple wrapper class for the RequestID would be zero code ...	Nathan Binkert	Feb. 6, 2012, 8:16 a.m.	Resolved

src/arch/arm/table_walker.hh (Diff revision 1)

Seems like a typedef for RequestID might be in order.  I doubt that it really needs to be a uint32_t.  Also, might having a -1 value be useful?

Show all issues

Ali Saidi Feb. 6, 2012, 4:40 a.m. (Feb. 6, 2012, 4:40 a.m.)

I want absolutely no option of not setting a requester id and therefor having a value like -1. That is what lead to the mess that this hopes to fix. All requests must have a requester id and thus no magic around indexing by -1 or something need apply.  The typedef is reasonable.

src/arch/x86/pagetable_walker.hh (Diff revision 1)

shouldn't this be a static_cast?  We should always know the type.  (It could be a safe_cast if you like)

Show all issues

Ali Saidi Feb. 6, 2012, 4:40 a.m. (Feb. 6, 2012, 4:40 a.m.)
```
yup
```

src/cpu/testers/rubytest/Check.cc (Diff revision 1)

What's this?

Show all issues

Ali Saidi Feb. 6, 2012, 4:40 a.m. (Feb. 6, 2012, 4:40 a.m.)
```
a problem that needs to be fixed
```

src/mem/request.hh (Diff revision 1)

Maybe a little comment on what each of these rids are for?  writeback, functional, and ????

Show all issues

src/mem/request.hh (Diff revision 1)

These changes are somewhat dangerous because of type conversions.  A simple wrapper class for the RequestID would be zero code overhead, yet it would prevent accidents by preventing implicit casts and strange bugs.  Just a thought.

Show all issues

Ali Saidi Feb. 6, 2012, 4:40 a.m. (Feb. 6, 2012, 4:40 a.m.)

i thought of that, however I was very careful and checked that I didn't perturb anything with the regressions. ohh how I wish c++ supported kwargs at least for readability/verification.

Ali Saidi Feb. 7, 2012, 4 a.m. (Feb. 7, 2012, 4 a.m.)

You mean something like:
struct RequestID 
{
int rid;
explicit RequestID(int _rid) : rid(_rid) {}
};
?

Anyone have any comments?

In general this looks great.  I just have a couple of minor concerns:
1. To me "RequestID" would be an ID for a specific request, where "RequesterID" is what you're doing here.
    a. So if you're going to make a typedef or struct I'd want it named the latter.
    b. Variables, arguments, and functions with "reqId" in the name are ambiguous about whether they're a RequestID or a RequesterID, and I can see that leading to confusion.  Unfortunately I don't have any great ideas here.  Spelling out requesterID all the time is pretty verbose.  What about changing the name to masterID?
2. Is there a way to figure out what ID goes with what object and record that somewhere in the stats file or config.ini, or to assign the IDs a priori in python?  I'm concerned that you'll see that requestor 17 is causing problems and have no idea what that is.  getReqName() provides a basic hook here but I don't see that it gets called from anywhere.

Ali Saidi Feb. 7, 2012, 6:58 a.m. (Feb. 7, 2012, 6:58 a.m.)

Andreas requested masterId as well, so i guess I'll go for that. 

In this change getReqName() isn't called anywhere, however it's used in the next patch to name the stat array indexes. 
What is now 
mshr_misses::0
mshr_misses::1
...

will instead become:
mshr_misses::cpu_dside
mshr_misses::cpu_iside
...

Andreas Hansson Feb. 8, 2012, 6:15 p.m. (Feb. 8, 2012, 6:15 p.m.)

Indeed, I would prefer masterId as it identifies the MasterPort where the transaction request is created. We should also clearly state if/how it is allowed to be changed, i.e. if it is forwarded by a bridge, must it always retain its original masterId, or is the bridge (acting as a master on another bus) allowed to change it?

A more long term name change that I would like to suggest is to rename the Request class Transaction, since it lives over the span of both a request, forwarded request, forwarded response, and response etc (irrespective of the length of the message chain). A Transaction is then persistent over a number of phases, and comprises a number of messages that are exchanged. Ultimately, the current Packet class would be split into a "transaction layer" Message class (one for each phase of a transaction), and an interconnect "transport layer" with a Packet class that wraps messages and adds interconnect specific information. To start with a Packet would always correspond 1-1 to a Message, but that could change over time for other (future) interconnects. I'm going slightly astray here, so please provide any feedback in a separate thread.

src/sim/system.cc (Diff revision 1)

Is there (or should there be) a flag that gets set when stats are enabled?

Ali Saidi Feb. 7, 2012, 6:58 a.m. (Feb. 7, 2012, 6:58 a.m.)

i need to find the appropriate incantation in swig to add it. If you happen to know it, that would be great.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Status: Closed (submitted)