> 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. > > Joel Hestness wrote: > > 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. > > Andreas Hansson wrote: > My comment regarding it being "strange" was merely acknowledging the fact > that the use-cases you mention are not part of the gem5 codebase. Perhaps > unconventional would be a better adjective. > > The reason for the change is simplicity. The memory system is riddled > with assumptions, making it harder than necessary to understand, debug and > extend. This patch is merely one small step towards making the assumptions > more explicit, and making life easier for any developer. It is impossible for > me or anyone else to know what assumptions people make outside the main > codebase. I think the guiding principle should be to firm things up as much > as possible, so if there are unconventional use-cases we need to accommodate, > it would be good to get at least part of that into the main codebase so that > it is visible. > > On the grand scheme of things, if this patch is causing problems, let's > just discard it. There are definitely more fruitful issues to spend our > reviewing efforts on.
Ok. Thanks for the notes. I suspect we might see the fence packet use case with the incoming AMD GPU patches. I'm still not sure if/when we might try to get gem5-gpu code into gem5. - 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
