> 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?
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. - 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
