> On Aug. 23, 2015, 7:12 p.m., Joel Hestness wrote: > > Hi Andreas. I'm wondering why this patch was checked in > > (http://repo.gem5.org/gem5/rev/842f56345a42). It doesn't have a 'Ship It', > > and the review request is only 4 days old. > > > > This change also breaks some packet use cases we have in gem5-gpu. > > Specifically, this change now requires that Requests be translated or > > otherwise find a valid physical address before a packet can be constructed. > > I'm not convinced that this is a reasonable requirement, especially since > > packets can be used between components that operate completely on virtual > > addresses. > > > > Can you please revert this change? > > Andreas Hansson wrote: > Hi Joel, > > I called out for feedback, and Steve was the only one to respond. Since > the patch was fairly trivial and the functionality was never used I opted to > not gate other changes on this minor modification. > > Could you explain your use-case a bit better? Packets _need_ a physical > address. Previously there was an option to create a va-only request, then > create the packet, then do the va->pa. Note that the packet always had to > hava physical address in both cases. The late setting was _never_ used in the > main gem5 codebase and opened up for nasty bugs (trust me on this one). > > How is the gem5-gpu use-case different from what is happening in the main > codebase? Why not first create the request (do any translation), and then > create the packet? > > Joel Hestness wrote: > Thank you for reverting the change. I would have liked more than 2 days > to provide feedback on this change, especially since it lacked a 'Ship It'. > > There are a number of use cases where this change causes strange > requirements. The clearest in gem5-gpu is in the GPU where we communicate > virtual addresses from the memory pipeline dispatch to the load-store queue > (which does coalescing first). Coalescing occurs before address translation, > so Requests to the LSQ can only contain a virtual address. This patch would > require calling req->setPaddr(<something>) just to create the packet to > communicate to the load-store queue. Given there isn't a physical address > yet, including setPaddr() in the code is strange and misleading both in > writing and reading the code. > > Other use cases involve communicating commands other than loads/store, > such as control messages or fence commands. I don't see why we should require > that a request have a valid physical address to create a packet. These packet > commands don't even require a virtual address but do require communicating > things like the command, request flags, master ID, thread ID, sender states, > etc. > > Finally, there is a use case in which we extend packet to store GPU > access decoalescing state, and we don't have a single *virtual* address > before instantiating the state and packet. Other recent gem5 changes dictate > that we're going to need to change this, but this is another instance that > indicates the desire to be able to create packets without valid physical > addresses. > > Andreas Hansson wrote: > I'd say it's your use of packets that is "strange" (or at least not in > line with the rest of gem5) :-) > > Why do you not coalesce/split the requests, as done in other places, > before you create the packets? > > The address is fundamentally needed by the memory system (caches, > crossbars etc), and throughout the entire gem5 codebase you can always assume > that a packet has a valid (physical) address once created. Note that this > requirement is also in line with SystemC TLM, and I'd argue there is reason > to keep gem5's assumptions as similar as possible.
> Why do you not coalesce/split the requests, as done in other places, before > you create the packets? This route only addresses one of our use cases, and as I stated, we're going to need to change this particular case. I gave 2-3 other use cases for which it doesn't make sense to require a valid physical address. Do you have recommendations for addressing those? I don't see what is 'strange' about them, or even how they can be any different. > The address is fundamentally needed by the memory system (caches, crossbars > etc), and throughout the entire gem5 codebase you can always assume that a > packet has a valid (physical) address once created. How would one model virtually-addressed caches? This patch would disallow using packets to send to a virtually-addressed cache. I apologize if I'm missing something obvious, but I'm hoping you can clarify your overall argument. I agree that packets to the memory system require addresses (obvious if needed for routing). I can also acknowledge that for all current gem5 uses of packets within memory systems, it may make sense to require the physical address (there are no virtually-addressed memory system components currently). However, I don't understand why having a physical address is an appropriate requirement for packets that do not access memory locations (i.e. are not loads/stores) and do not need to be routed based on address in the memory system. I also don't understand why a physical address should be required if communicating between components outside a cache hierarchy where addresses may not be required (i.e. communicating through ports). This latter use of packets has always been allowed in gem5. Separate from the semantics of this specific patch, this sort of change is something we've been grappling with as a collaborative community: Trying to remove components/functionality and increase interface strictness can be exceptionally painful for anyone that has internal patches derived from the code being removed/restricted. AMD has vetoed a handful of such patches recently. I feel this case is very similar to those vetoes, and especially without more clarity on how existing packet use should be changed. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3044/#review7028 ----------------------------------------------------------- On Aug. 24, 2015, 9:04 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3044/ > ----------------------------------------------------------- > > (Updated Aug. 24, 2015, 9:04 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11058:2c40795e9a88 > --------------------------- > mem: Reflect that packet address and size are always valid > > This patch simplifies the packet, and removes the possibility of > creating a packet without a valid address and/or size. Under no > circumstances are these fields set at a later point, and thus they > really have to be provided at construction time. > > > Diffs > ----- > > src/mem/packet.hh 842f56345a42 > > Diff: http://reviews.gem5.org/r/3044/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
